Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Validator][ConstraintValidator] Safe fail on invalid timezones#34904
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cebb5ac to97b9745CompareTests 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. |
fancyweb commentedDec 9, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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? |
9871424 to7fbfa3cCompare
Looks like we can. It simplifies everything. I just pushed it. |
rebase needed |
7fbfa3c toafc851aCompareCo-authored-by: Scott Dawson <scott@loyaltycorp.com.au>
afc851a to3b1b994CompareRebased + tests pass. I modified the tests to strenghten them. |
Thank you@fancyweb. |
…zones (fancyweb)This PR was merged into the 3.4 branch.Discussion----------[Validator][ConstraintValidator] Safe fail on invalid timezonesCo-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
Co-authored-by: Scott Dawsonscott@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.