[Form] reverse transform RFC 3339 formatted dates #28712

xabbuh · 2018-10-03T17:41:53Z

Q	A
Branch?	2.8
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#28699
License	MIT
Doc PR

Technically, dates formatted according to the HTML specifications do not
contain any timezone information. But since our DateTimeType used to
contain this information in the passed, users had configure their JS
libraries to accept (and create) dates in that format.

To not break BC we should accept these dates and silently ignore the
additional timezone information.

darles · 2018-10-04T06:21:27Z

...ny/Component/Form/Extension/Core/DataTransformer/DateTimeToHtml5LocalDateTimeTransformer.php

@@ -81,7 +81,7 @@ public function reverseTransform($dateTimeLocal)
            return;
        }

-        if (!preg_match('/^(\d{4})-(\d{2})-(\d{2})[T ]\d{2}:\d{2}(?::\d{2})?$/', $dateTimeLocal, $matches)) {
+        if (!preg_match('/^(\d{4})-(\d{2})-(\d{2})[T ]\d{2}:\d{2}(?::\d{2})?(?:\.\d)?(?:Z|(?:(?:\+|-)\d{2}:\d{2}))?$/', $dateTimeLocal, $matches)) {


Maybe it would be possible to also support timezone without ":"?
if (!preg_match('/^(\d{4})-(\d{2})-(\d{2})[T ]\d{2}:\d{2}(?::\d{2})?(?:\.\d)?(?:Z|(?:(?:\+|-)\d{2}:\d{2})|(?:\+|-)\d{4})?$/', $dateTimeLocal, $matches)) {

Can you give an example string of what you have in mind so I can add it to the tests?

2018-09-15T10:00:00+0200

done, thank you for the feedback

Technically, dates formatted according to the HTML specifications do not contain any timezone information. But since our DateTimeType used to contain this information in the passed, users had configure their JS libraries to accept (and create) dates in that format. To not break BC we should accept these dates and silently ignore the additional timezone information.

HeahDude

Thanks for fixing this!
Shouldn't we deprecate this behavior in master to be able to use a strict check in Symfony 5?

magnetik · 2018-10-04T10:30:15Z

+1 for being clearer about what should work and not to be able to be stricter in the future

xabbuh · 2018-10-04T10:33:51Z

@HeahDude Yeah, I am already preparing some PRs to improve DX around this.

xabbuh · 2018-10-04T11:43:08Z

@HeahDude see #28724

danrot · 2018-10-04T14:08:07Z

@xabbuh Do you already know if this will be accepted? Because if it is not we have to fix our JS, otherwise we would only have to add this version as conflict, and we could release a new working version immediately.

xabbuh · 2018-10-04T14:48:27Z

@danrot Adding the conflict rule will be enough.

danrot · 2018-10-10T12:00:28Z

I've added the conflict for symfony 3.4.16 and 3.4.17 in our composer.json, but if you don't set our latest version (1.6.22) as minimum in the project composer.json, composer will install Sulu 1.6.21, so that it can use Symfony 3.4.17, which then still breaks the installation (hope you understand what I mean, otherwise I can elaborate further 🙈).

So people are really struggling with the installation of our product... Is there any other thing we can do, apart from kindly asking you to get this merged and release it? 😊

YetiCGN · 2018-10-10T12:05:01Z

I've added the conflict for symfony 3.4.16 and 3.4.17 in our composer.json, but if you don't set our latest version (1.6.22) as minimum in the project composer.json, composer will install Sulu 1.6.21, so that it can use Symfony 3.4.17, which then still breaks the installation (hope you understand what I mean, otherwise I can elaborate further 🙈).

Huh? I have "sulu/sulu": "~1.6.0", as a dependency in several projects (all of our Sulu projects, actually) and composer update correctly installs Sulu 1.6.22. The only time this would fail is if people did the update last week when Sulu 1.6.22 wasn't out yet but Symfony 3.4.17 was already - like we did. But even then an Update after the release of Sulu 1.6.22 fixed it. Why would you think people are locked to Sulu 1.6.21?

danrot · 2018-10-10T12:19:23Z

Just tested it again, had a project with "sulu/sulu": "~1.6.22", where 1.6.22 was installed, no surpirse so far. Then I changed my composer.json in the following way (I assume that's what most Sulu users have in their composer.json):

-        "sulu/sulu": "~1.6.22",
+        "sulu/sulu": "~1.6.0",

Then I executed another composer update, and that's the output:

daniel@macbook ~/D/s/sulu-standard $ composer update
Loading composer repositories with package information                                                                                                                                                             
Updating dependencies (including require-dev)                                                                                                                                                                      
Package operations: 0 installs, 3 updates, 0 removals                                                                                                                                                              
  - Updating symfony/symfony (v3.4.15 => v3.4.17): Loading from cache                                                                                                                                              
  - Downgrading sulu/sulu (1.6.22 => 1.6.21):  Checking out c0c04cd95e                                                                                                                                             
  - Updating dantleech/phpcr-migrations-bundle (1.0.0 => 1.1.0): Loading from cache

In addition to that composer even tells me that Symfony is the reason for not installing 1.6.22:

daniel@macbook ~/D/s/sulu-standard $ composer why-not sulu/sulu 1.6.22
sulu/sulu  1.6.22  conflicts  symfony/symfony (2.8.10 || 3.4.12 || 3.4.16 || 3.4.17)

Just realized that it works, if the "sulu/sulu" require is before the "symfony/symfony" require 😕 Feels a bit weird, but we can't fix that for existing projects 😞

YetiCGN · 2018-10-10T12:34:43Z

Just realized that it works, if the "sulu/sulu" require is before the "symfony/symfony" require 😕 Feels a bit weird, but we can't fix that for existing projects 😞

Ah! See, we have "config": { "sort-packages": true }, in all our projects, which puts Sulu before Symfony. 🙂

danrot · 2018-10-10T12:54:32Z

And since sort-packages will be the default with symfony flex, that doesn't make me feel more comfortable 🙈 That means that if we have a conflict with a package that is ordered after sulu alphabetically there is no way to fix this 😕

YetiCGN · 2018-10-10T12:58:38Z

Anyhow, please somebody merge this to the LTS branches 2.8 and 3.4 so we can safely rely on backwards compatibility again. 🙂

nicolas-grekas · 2018-10-14T18:31:23Z

Thank you @xabbuh.

This PR was merged into the 2.8 branch. Discussion ---------- [Form] reverse transform RFC 3339 formatted dates | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28699 | License | MIT | Doc PR | Technically, dates formatted according to the HTML specifications do not contain any timezone information. But since our DateTimeType used to contain this information in the passed, users had configure their JS libraries to accept (and create) dates in that format. To not break BC we should accept these dates and silently ignore the additional timezone information. Commits ------- 503239f reverse transform RFC 3339 formatted dates

xabbuh added this to the 2.8 milestone Oct 3, 2018

carsonbot added Status: Needs Review Form Bug labels Oct 3, 2018

xabbuh mentioned this pull request Oct 3, 2018

[Form] TransformationFailedException when submitting a ISO8601 date to a DateTimeType field #28699

Closed

darles reviewed Oct 4, 2018

View reviewed changes

xabbuh mentioned this pull request Oct 4, 2018

[Form] Do not fail reverse transforming relative dates #28703

Closed

xabbuh force-pushed the issue-28699-html5 branch from 36f9c8f to 503239f Compare October 4, 2018 10:00

HeahDude approved these changes Oct 4, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 4, 2018

xabbuh mentioned this pull request Oct 4, 2018

[Form] deprecate submitting non HTML5 formatted dates #28724

Closed

nicolas-grekas merged commit 503239f into symfony:2.8 Oct 14, 2018

xabbuh deleted the issue-28699-html5 branch October 14, 2018 18:37

This was referenced Nov 3, 2018

Release v2.8.47 #29069

Merged

Release v3.4.18 #29070

Merged

fabpot mentioned this pull request Nov 3, 2018

Release v4.1.7 #29071

Merged

This was referenced Mar 12, 2021

Bump symfony/symfony from 2.8.27 to 2.8.52 lajoskvcs/symfony-final#2

Open

Bump symfony/symfony from 2.8.3 to 2.8.52 lajoskvcs/symfony-course#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] reverse transform RFC 3339 formatted dates #28712

[Form] reverse transform RFC 3339 formatted dates #28712

xabbuh commented Oct 3, 2018

darles Oct 4, 2018

xabbuh Oct 4, 2018

darles Oct 4, 2018

xabbuh Oct 4, 2018

HeahDude left a comment

magnetik commented Oct 4, 2018

xabbuh commented Oct 4, 2018

xabbuh commented Oct 4, 2018

danrot commented Oct 4, 2018

xabbuh commented Oct 4, 2018

danrot commented Oct 10, 2018 •

edited

YetiCGN commented Oct 10, 2018

danrot commented Oct 10, 2018 •

edited

YetiCGN commented Oct 10, 2018

danrot commented Oct 10, 2018

YetiCGN commented Oct 10, 2018

nicolas-grekas commented Oct 14, 2018

[Form] reverse transform RFC 3339 formatted dates #28712

[Form] reverse transform RFC 3339 formatted dates #28712

Conversation

xabbuh commented Oct 3, 2018

darles Oct 4, 2018

Choose a reason for hiding this comment

xabbuh Oct 4, 2018

Choose a reason for hiding this comment

darles Oct 4, 2018

Choose a reason for hiding this comment

xabbuh Oct 4, 2018

Choose a reason for hiding this comment

HeahDude left a comment

Choose a reason for hiding this comment

magnetik commented Oct 4, 2018

xabbuh commented Oct 4, 2018

xabbuh commented Oct 4, 2018

danrot commented Oct 4, 2018

xabbuh commented Oct 4, 2018

danrot commented Oct 10, 2018 • edited

YetiCGN commented Oct 10, 2018

danrot commented Oct 10, 2018 • edited

YetiCGN commented Oct 10, 2018

danrot commented Oct 10, 2018

YetiCGN commented Oct 10, 2018

nicolas-grekas commented Oct 14, 2018

danrot commented Oct 10, 2018 •

edited

danrot commented Oct 10, 2018 •

edited