$ php main.phpPHP Fatal error:  Uncaught Exception: Failed to parse time string (6418-75-51) at position 6 (5): Unexpected character in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php:714Stack trace:#0 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(714): DateTimeImmutable->__construct()#1 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(312): Symfony\Component\Yaml\Inline::evaluateScalar()#2 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(80): Symfony\Component\Yaml\Inline::parseScalar()#3 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(790): Symfony\Component\Yaml\Inline::parse()#4 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(341): Symfony\Component\Yaml\Parser->parseValue()#5 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(86): Symfony\Component\Yaml\Parser->doParse()#6 /tmp/symfony-yaml/vendor/symfony/yaml/Yaml.php(77): Symfony\Component\Yaml\Parser->parse()#7 /tmp/symfony-yaml/main.php(8): Symfony\Component\Yaml\Yaml::parse()#8 {main}  thrown in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php on line 714

This is because the "month" is invalid. Fixing the "month" will trigger about the same issue because the "day" would be invalid.

With the current change it will throw aParseException.

homersimpsons requested a review fromxabbuh as acode owner

August 9, 2024 17:11

carsonbot added Status: Needs Review Bug labels

Aug 9, 2024

carsonbot added this to the7.2 milestone

Aug 9, 2024

Copy link

carsonbot commentedAug 9, 2024

Hey!

Thanks for your PR. You are targeting branch "7.2" but it seems your PR description refers to branch "7.2 for features / 5.4, 6.4, and 7.1 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

stof reviewed

Aug 9, 2024

View reviewed changes

Copy link

Member

stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This needs some tests to prevent regressions

Copy link

Member

derrabus commentedAug 9, 2024

What does the YAML spec say about invalid date strings? Is the fallback to a plain string the expected behavior?

Copy link

ContributorAuthor

homersimpsons commentedAug 9, 2024•
edited
Loading

This needs some tests to prevent regressions

I added a test. I do not know how far should I go with the "invalid dates"

What does the YAML spec say about invalid date strings? Is the fallback to a plain string the expected behavior?

I cannot seem to find anything searchinghttps://yaml.org/spec/1.2.2/ for "date", "invalid", "incorrect".

I also searched for "date" inhttps://github.com/yaml/yaml-test-suite without success.

I tried withhttps://github.com/yaml/yaml-reference-parser/blob/main/parser-1.2/javascript (I had to hack a bit around to make it work) but it looks like this is not checking explicitly for dates:

xabbuh added the Yaml label

Aug 11, 2024

xabbuh modified the milestones:7.2,5.4

Aug 11, 2024

carsonbot changed the title~~🐛 Gracefully handle invalid dates~~[Yaml] 🐛 Gracefully handle invalid dates

Aug 11, 2024

xabbuh requested changes

Aug 11, 2024

View reviewed changes

Copy link

Member

xabbuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

https://yaml-online-parser.appspot.com/?yaml=date%3A+2024-50-50&type=python yields an error for invalid date and I think we should do the same (i.e. throw aParsException).

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Aug 11, 2024

homersimpsons force-pushed thepatch-1 branch from726d28c to4c8cae7Compare

August 11, 2024 13:46

Copy link

ContributorAuthor

homersimpsons commentedAug 11, 2024

https://yaml-online-parser.appspot.com/?yaml=date%3A+2024-50-50&type=python yields an error for invalid date and I think we should do the same (i.e. throw aParsException).

I changed the code to an exception.

Which branch should I target? 5.4? 6.4? 7.1?