u01jmg3 / ics-parser

Parser for iCalendar Events • PHP 8+, 7 (≥ 7.4), 5 (≥ 5.6)
MIT License
450 stars 144 forks source link

Modify key value from string #291

Closed noec764 closed 2 years ago

noec764 commented 2 years ago

I change this function because of a unexpected behaviour.

Indeed, I have this line in my ics file :

ATTENDEE;CN="Lieu : Laboratoire Dép. d'Analyse, Bâtiment : Laboratoire Dép. d'Analyse, Étage : 0, Salle : Grande salle";CUTYPE=RESOURCE;ROLE=NON-PARTICIPANT;PARTSTAT=ACCEPTED;RSVP=TRUE:mailto:xxx@xxx.fr

but when it was parsed by keyValueFromString function I get this result:

Array (size=2) 0 => string 'ATTENDEE' (length=8) 1 => array (size=2) 0 => string 'mailto:xxx@xxx.fr (length=40) 1 => array (size=1) 'CN' => string 'Lieu: Laboratoire Dép. d'Analyse, Bâtiment: Laboratoire Dép. d'Analyse, Étage: 0, Salle: Grande salle;CUTYPE' (length=112)

We can see that there is no more CUTYPE , ROLE, PARTSTAT and RSVP parameters.

I found out that this function doesn't parse ics line as recommended in RFC5545 especially when using str_getcsv($text, ':') which explodes strings on colon even inside double quote.

I finally decides to re code this function entirely in order respect the RFC5545.

The parseLine() function analyses the line characters by characters and return a array of strings and separators.

Then the keyValueFromString() create the object corresponding to what is expected in RFC5545 and matching with old function template.

u01jmg3 commented 2 years ago

@s0600204: would you have time to cast your eyes over this PR? Thanks

s0600204 commented 2 years ago

@noec764:

Firstly, thank you for your submission. It is clear from trying this myself that our parsing of key-parameter-values encapsulated within double-quotes is flawed.

Secondly, your assessment is mildly incorrect. This issue does appear to be centred around our use of str_getcsv(), but not for the reason you state above. str_getcsv() doesn't split on the colons encapsulated within the double-quotes, but it does remove the double-quotes from the values it returns, e.g.:

"a : b ; c";d=D;e=E:f:g
// Becomes
Array(
    [0] => a : b ; c;d=D;e=E
    [1] => f
    [2] => g
)

We later naïvely slap double quotes around the entire thing to make up for this (e.g. "a : b ; c;d=D;e=E"), and this causes problems when we later want to split on the semi-colons, as the regex can't find any outside the double quotes.

To make it even more fun (!) the regex splitting on the = also removes double-quotes it finds, and the first one it encounters is always unquoted:

CN="a : b ; c;d=D;e=E"
// Becomes
Array(
    [0] => CN
    [1] => a : b ; c;d
    [2] => D;e
    [3] => E
)

Anyway, the take away here is that our approach is flawed. So once again thank you for providing a better solution (and one that doesn't use regex, which is always a plus!).

s0600204 commented 2 years ago

@u01jmg3: FYI, this PR incidentally also adds support for key-parameters to have multiple values (e.g. the DELEGATED-TO parameter of ATTENDEE), which we didn't have previously.

For instance, the following line (taken from the RFC5545 spec, top of page 18):

ATTENDEE;DELEGATED-TO="mailto:jdoe@example.com","mailto:jqpublic@example.com":mailto:jsmith@example.com

Used to result in:

[ATTENDEE_array] => Array(
    [0] => Array(
        [DELEGATED-TO] => mailto:jdoe@example.com,
    )
    [1] => jqpublic@example.com":mailto:jsmith@example.com
)

And with this PR:

[ATTENDEE_array] => Array(
    [0] => Array (
        [DELEGATED-TO] => Array (
            [0] => mailto:jdoe@example.com
            [1] => mailto:jqpublic@example.com
        )
    )
    [1] => mailto:jsmith@example.com
)
u01jmg3 commented 2 years ago

@noec764: would you be able to make the list of changes @s0600204 has kindly put together?

noec764 commented 2 years ago

@noec764: would you be able to make the list of changes @s0600204 has kindly put together? Thank you for answer ! Yes, no problem, but I will do it in January.

u01jmg3 commented 2 years ago

@noec764: is this ready to review again?

noec764 commented 2 years ago

Yes it is ready

u01jmg3 commented 2 years ago

@s0600204: can I ask for one final review? I think there may be one outstanding test to add.

That parameters-of-keys may have multiple values (that a key may have multiple values is tested for already)