Skip to content

[Yaml] 🐛 throw ParseException on invalid date#57968

Merged
xabbuh merged 1 commit intosymfony:5.4from
homersimpsons:patch-1
Aug 12, 2024
Merged

[Yaml] 🐛 throw ParseException on invalid date#57968
xabbuh merged 1 commit intosymfony:5.4from
homersimpsons:patch-1

Conversation

@homersimpsons
Copy link
Copy Markdown
Contributor

@homersimpsons homersimpsons commented Aug 9, 2024 •
edited
Loading

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues None
License MIT

(found in symfony-tools/docs-builder#179)

When parsing the following yaml:

date: 6418-75-51

symfony/yaml will throw an exception:

$ php main.php
PHP 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:714
Stack 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 a ParseException.

@homersimpsons homersimpsons requested a review from xabbuh as a code owner August 9, 2024 17:11
@carsonbot carsonbot added this to the 7.2 milestone Aug 9, 2024
@carsonbot
Copy link
Copy Markdown

carsonbot commented Aug 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
stof reviewed Aug 9, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@stof 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

@derrabus
Copy link
Copy Markdown
Member

derrabus commented Aug 9, 2024

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

@homersimpsons
Copy link
Copy Markdown
Contributor Author

homersimpsons commented Aug 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 searching https://yaml.org/spec/1.2.2/ for "date", "invalid", "incorrect".

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

I tried with https://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:
image

@xabbuh xabbuh added the Yaml label Aug 11, 2024
@xabbuh xabbuh modified the milestones: 7.2, 5.4 Aug 11, 2024
@carsonbot carsonbot changed the title 🐛 Gracefully handle invalid dates [Yaml] 🐛 Gracefully handle invalid dates Aug 11, 2024
xabbuh
xabbuh requested changes Aug 11, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@xabbuh 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 a ParsException).

@homersimpsons
Copy link
Copy Markdown
Contributor Author

homersimpsons commented Aug 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 a ParsException).

I changed the code to an exception.

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

@derrabus
Copy link
Copy Markdown
Member

derrabus commented Aug 11, 2024

5.4

@homersimpsons homersimpsons changed the base branch from 7.2 to 5.4 August 11, 2024 16:52
@homersimpsons
Copy link
Copy Markdown
Contributor Author

homersimpsons commented Aug 11, 2024

5.4

Done, There was a conflict on src/Symfony/Component/Yaml/Inline.php: \DateTime becomes \DateTimeImmutable in later versions

@derrabus
Copy link
Copy Markdown
Member

derrabus commented Aug 11, 2024

Can you please adjust the PR title and description? They don't match the implemented logic anymore.

@homersimpsons homersimpsons changed the title [Yaml] 🐛 Gracefully handle invalid dates [Yaml] 🐛 throw ParseException on invalid date Aug 11, 2024
@homersimpsons
Copy link
Copy Markdown
Contributor Author

homersimpsons commented Aug 11, 2024

Can you please adjust the PR title and description? They don't match the implemented logic anymore.

Done. Tests are failing on twig but I didn't change anything there.

@xabbuh
Copy link
Copy Markdown
Member

xabbuh commented Aug 12, 2024

Thank you @homersimpsons.

@xabbuh xabbuh merged commit d5b0f63 into symfony:5.4 Aug 12, 2024
@homersimpsons homersimpsons deleted the patch-1 branch August 12, 2024 08:41
This was referenced Aug 30, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stof stof stof left review comments

@derrabus derrabus derrabus approved these changes

@xabbuh xabbuh xabbuh approved these changes

@chalasr chalasr chalasr approved these changes

@yceruto yceruto Awaiting requested review from yceruto

Assignees

No one assigned

Labels

Bug Status: Reviewed Yaml

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

6 participants

@homersimpsons @carsonbot @derrabus @xabbuh @stof @chalasr