Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] deprecate submitting non HTML5 formatted dates#28724
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
xabbuh commentedOct 4, 2018
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | #28712 (review) |
| License | MIT |
| Doc PR |
Technically, dates formatted according to the HTML specifications do notcontain any timezone information. But since our DateTimeType used tocontain this information in the passed, users had configure their JSlibraries to accept (and create) dates in that format.To not break BC we should accept these dates and silently ignore theadditional timezone information.
xabbuh commentedOct 4, 2018
needs to be rebased once#28712 is merged up to master |
nicolas-grekas commentedOct 4, 2018 • 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.
I'm really unsure about this. Having an option means asking devs to make a choice, and making a choice is hard. Here, enabling strict mode (Yay, strict all the things!) means lowering the compatibility with the ecosystem. That's contra the robustness principle. |
xabbuh commentedOct 4, 2018
To me, this goes along with#28753. If one wants to use HTML 5 features, they should follow the standards IMO. Custom formats are okay, but then you need to be explicit about this (i.e. set |
f545150 tod0dd6c9Compare...ny/Component/Form/Extension/Core/DataTransformer/DateTimeToHtml5LocalDateTimeTransformer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| // to maintain backwards compatibility we do not strictly validate the submitted date | ||
| // see https://github.com/symfony/symfony/issues/28699 | ||
| if (!$this->strict) { | ||
| $regex ='/^(\d{4})-(\d{2})-(\d{2})[T ]\d{2}:\d{2}(?::\d{2})?/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
to maintain BC, shouldn't it use the old transformer instead (which did not have that regex either, so you are only partially fixing the BC break)
stof commentedOct 4, 2018
@nicolas-grekas if you have a form type configured as HTML5_FORMAT, supporting other formats looks weird. The option is here to opt-in for the new behavior at the time you want (needed in case your JS currently generates a different format for instance) |
stof commentedOct 4, 2018
We may also want to harden DateTimeToRfc3339Transformer to actually support only RFC 3339 (using |
d0dd6c9 to3132819Comparenicolas-grekas commentedOct 5, 2018 • 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.
I'm 👎 here: data transformers are not where strictness should apply. Strictness is for validators. |
nicolas-grekas commentedOct 20, 2018
@xabbuh ok with my main objection: not worth adding a new option as DX may suffer? |
xabbuh commentedFeb 16, 2019
I am closing here for now. We can consider to reopen when we notice that a less strict date format handling leads to avoidable bugs. |