Skip to content

[Yaml] 🐛 throw ParseException on invalid date #57968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 12, 2024
Merged

[Yaml] 🐛 throw ParseException on invalid date #57968

merged 1 commit into from
Aug 12, 2024

Conversation

homersimpsons
Copy link
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

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
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
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
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
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
Contributor Author

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
Member

5.4

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

5.4

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

@derrabus
Copy link
Member

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
Contributor Author

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
Member

xabbuh commented Aug 12, 2024

Thank you @homersimpsons.

@xabbuh xabbuh merged commit d5b0f63 into symfony:5.4 Aug 12, 2024
9 of 12 checks passed
@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