[Validator][ConstraintValidator] Safe fail on invalid timezones #34904
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fancyweb
force-pushed
the
validator-zulu-fix-
branch
2 times, most recently
from
cebb5ac
to
97b9745
Compare
|
Tests pass on my machine with both implem (polyfill and native) but fails on CI 😕 Looks like the good value is not returned when using UTC. |
|
Also we have to think of what is the goal here: display the passed date relatively to its timezone (the same datetime in all timezones must display the same thing basically). Can't we just always set the formatter to UTC, copy the passed value in a temp var and set the timezone to UTC, so we don't have to deal with this timezone stuff? |
fancyweb
force-pushed
the
validator-zulu-fix-
branch
2 times, most recently
from
9871424
to
7fbfa3c
Compare
Looks like we can. It simplifies everything. I just pushed it. |
|
rebase needed |
fancyweb
force-pushed
the
validator-zulu-fix-
branch
from
7fbfa3c
to
afc851a
Compare
Co-authored-by: Scott Dawson <scott@loyaltycorp.com.au>
fancyweb
force-pushed
the
validator-zulu-fix-
branch
from
afc851a
to
3b1b994
Compare
|
Rebased + tests pass. I modified the tests to strenghten them. |
nicolas-grekas
approved these changes
Dec 13, 2019
|
Thank you @fancyweb. |
nicolas-grekas
added a commit
that referenced
this pull request
Dec 13, 2019
…zones (fancyweb) This PR was merged into the 3.4 branch. Discussion ---------- [Validator][ConstraintValidator] Safe fail on invalid timezones Co-authored-by: Scott Dawson <scott@loyaltycorp.com.au> | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | #33901 | License | MIT | Doc PR | Alternative to #33902. I will explain why I think it is better this way: 1. We set the timezone with the setter because it's 100% safe, it never fails. It fall backs to the default timezone if the provided timezone is not supported (as if we passed null, so the same behavior that always existed). We are therefore compatible with all edge cases. 2. We don't validate the timezone with `\DateTimeZone::listIdentifiers()`. It only returns full identifiers like "Europe/Paris" but it doesn't take into account "numeric" identifiers such as "+08:00" which are perfectly valid. I added a test case to ensure we stay valid with this case. + some invalid identifiers for the native `\IntlDateFormatter` are valid with the polyfill that uses `\DateTimeZone` (eg : `X`). I don't think we can validate anything safely that will work reliably on both implementations. Commits ------- 3b1b994 [Validator][ConstraintValidator] Safe fail on invalid timezones
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Co-authored-by: Scott Dawson scott@loyaltycorp.com.au
Alternative to #33902.
I will explain why I think it is better this way:
\DateTimeZone::listIdentifiers(). It only returns full identifiers like "Europe/Paris" but it doesn't take into account "numeric" identifiers such as "+08:00" which are perfectly valid. I added a test case to ensure we stay valid with this case. + some invalid identifiers for the native\IntlDateFormatterare valid with the polyfill that uses\DateTimeZone(eg :X). I don't think we can validate anything safely that will work reliably on both implementations.