u01jmg3 / ics-parser

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

Add the information that states that the datas were successfully parsed #337

Closed axi closed 7 months ago

axi commented 7 months ago

Similar to #336 but just validating that some content has been parsed and no error has been thrown. Without this, there's no way to distinguish the state of the parser before and after parsing happened. Another idea could be to set $cal property to null before parsing but this would be a BC.

s0600204 commented 7 months ago

Without this, there's no way to distinguish the state of the parser before and after parsing happened.

As per one of your own comments on the previous iteration, with this there's still no way to distinguish between the state of the parser before and after parsing a second (or third, or fourth, or...) input file/string/url. Whilst setting $cal to null could indeed be considered a breaking change, setting your new variable to false at the start of initLines() would not.

axi commented 7 months ago

@s0600204 this makes sense, I pushed an update

u01jmg3 commented 7 months ago

I must admit I'm still not happy with merging this code. It does not indicate why content wasn't parsed which, if I was making use of this code, would be my next question. For me, I feel the implementation is too vague.

@s0600204: I'd be keen to hear your thoughts.

axi commented 7 months ago

In my opinion, knowing that a stream/file/content is "invalid" in this context, even if I don't know why it's invalid, is more valuable that not having a way to get that information.

s0600204 commented 7 months ago

I'd say it depends on the intended objective.

If the objective is to determine whether a provided file passes through the parser without error, then

$parsedSuccessfully = false;
try {
    $ical = new ICal('./example_ical.ics');
    $parsedSuccessfully = true;
} catch (Exception $except) {}

would probably work just as well.

If the objective is to determine if the passed file/string/url is valid ICal data, then this PR fails as - like its predecessor - it only really checks for the existence of BEGIN:VCALENDAR.

Just because it passes through our parser without error does not necessarily mean that it's valid ICal. The following currently pass through the parser without error:

BEGIN:VCALENDAR
BEGIN:VCALENDAR
BEGIN:EVENT
DTSTART:20240101

Are they "valid"? No. Are they successfully parsed? Yes.

u01jmg3 commented 7 months ago

Thanks for your assessment, @s0600204.

I agree and still don't think there's enough merit in merging this code.