[Validator] Validate timezone before passing to IntlDateFormatter #33902
Conversation
This file contains hidden or 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
reviewed
Oct 8, 2019
xabbuh
requested changes
Oct 15, 2019
xabbuh
approved these changes
Oct 17, 2019
fabpot
reviewed
Nov 9, 2019
nicolas-grekas
approved these changes
Nov 28, 2019
|
any news on this ? since my 4.3.5 update i have the same error while validator datetime string with |
fancyweb
approved these changes
Dec 4, 2019
There was a problem hiding this comment.
@nicolas-grekas Did you remove your comment about perf?
My bad I just found it in the hidden conversation
|
My perf comment is here, and is not addressed for now: |
|
@sjdaws Do you mind if I finish this PR? |
|
@fancyweb sure, I just need it fixed so I can update 4.3. I’m not sure what else I need to do for this PR |
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.
This change validates timezones before passing them to the IntlDateFormatter to prevent fatal errors where an invalid timezone is used. This wasn't previously passed in 4.3.4 and was added in 4.3.5 which causes a break when using valid ISO8601 zulu timestamps. If an invalid timezone is provided it will fall back to system default, which was used for all versions < 4.3.4.