[Form] Fix Date\TimeType marked as invalid on request with single_text and zero seconds #20307

LuisDeimos · 2016-10-26T10:00:05Z

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

Fix: When using a form with an Time type with option 'widget' => 'single_text', and 0 is selected in the seconds, we obtain an TransformationFailedException "Unable to reverse value for property path "[time]": Data missing". Check ticket #20304

ro0NL · 2016-10-26T18:20:13Z

src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToStringTransformer.php

        }

+        // handle seconds ignored by user's browser when seconds as single_text is 0
+        if ($this->parseFormat === 'H:i:s|') {


This should also work without the pipe character set.

ro0NL · 2016-10-26T18:20:54Z

src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToStringTransformer.php

+        // handle seconds ignored by user's browser when seconds as single_text is 0
+        if ($this->parseFormat === 'H:i:s|') {
+            if (!preg_match('((?:(?:[0-1][0-9])|(?:[2][0-3])|(?:[0-9])):(?:[0-5][0-9])(?::[0-5][0-9]))', $value) &&
+                preg_match('((?:(?:[0-1][0-9])|(?:[2][0-3])|(?:[0-9])):(?:[0-5][0-9]))', $value)) {


Why not only the 2nd preg_match call and use ^..$? And should we consider missing hours and/or minutes as well on this?

I think the first run, and we should see only seconds, a lack of hours or minutes (without 'with_minutes' => false) itself should make a validation error it indicates an erroneous entry by the user. Here just we managed the browser omission.

Here just we managed the browser omission.

So this doesnt count for minutes and hours then? Sorry no real time to test this. But i proposed are more or less generic approach for "adding sensible defaults to the value". Ie.

format="H", value="", new_value="00" format="H:i", value="", new_value="00:00" format="H:i", value="12", new_value="12:00" etc.

Yes, that is why the condition $ this-> parseFormat === 'H: i: s |'.

And, ok, these default values would apply then sent the form (within the SUBMIT events) ?, if so, would work.

The ultimate goal of this is to correct the omission of the latter by the browser when the field requires seconds ('with_seconds' => true), and the user enters 0 seconds, in this case the browser sends '02: 05', in instead of '02: 05: 00 'causing a validation error.

For now, I fix this with an new DateTimeType implementing:

# To fix bug in Symfony. On choice 00 seconds in an DateTime with seconds, # is checked as invalid, because browser send the request as '01:00' instead of '01:00:00'. # we catch the submission data, and, where appropriate, hand add the missing zeros $builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $e) { $data = $e->getData(); if ($time = $data['time']) { if (!preg_match('((?:(?:[0-1][0-9])|(?:[2][0-3])|(?:[0-9])):(?:[0-5][0-9])(?::[0-5][0-9]))', $time) && preg_match('((?:(?:[0-1][0-9])|(?:[2][0-3])|(?:[0-9])):(?:[0-5][0-9]))', $time)) { $data['time'] = $time . ":00"; $e->setData($data); } } });`

And perhaps at this point even \d{2}:\d{2}

Ok, sounds great, it would be something like:

preg_match('/\d{2}:\d{2}$/', $time)

the above was to eliminate things like '99: 78 'but still has to go through validation.

testing in TimeType...

Using ^$ is key here ;-)

Of course, error finger, thanks

Well, he's looking good, you think?

ro0NL · 2016-10-26T21:34:38Z

src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTimeTypeTest.php

        $this->assertDateTimeEquals(new \DateTime('2010-06-02 03:04:05 UTC'), $form->getData());
    }

+    public function testSubmitWithSecondsAndBrowserOmissionSeconds()


Perhaps add this to TimeTypeTest.

This test may be removed now imo.

I think so, is covered by Time Type

Ok unit test only in TimeType ;)

ro0NL · 2016-10-26T21:35:22Z

src/Symfony/Component/Form/Extension/Core/Type/TimeType.php

+            if ($options['with_seconds']) {
+                $builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $e) {
+                    if ($data = $e->getData()) {
+                        if (preg_match('/^\d{2}:\d{2}$/', $data)) {


Can be a single if()

if ($options['with_seconds']) { $builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $e) { $data = $e->getData(); if ($data && preg_match('/^\d{2}:\d{2}$/', $data)) { $e->setData($data.':00'); } }); }`

Implemented suggestions, has not simplified the condition 'if ($ options [' with_seconds '])' to avoid adding an not necessary listener

ro0NL · 2016-10-26T21:36:16Z

src/Symfony/Component/Form/Extension/Core/Type/TimeType.php

        if ('single_text' === $options['widget']) {
            $builder->addViewTransformer(new DateTimeToStringTransformer($options['model_timezone'], $options['view_timezone'], $format));
+
+            // handle seconds ignored by user's browser when with_seconds enabled and seconds is 00


Perhaps add a link to some issue(s) here, ie the chromium one.

I think you can leave out and seconds is 00 (actually seconds are omitted which you already mentioned).

…ext and zero seconds (LuisDeimos)

xabbuh · 2016-11-12T11:01:03Z

👍

Status: Reviewed

fabpot · 2016-11-12T15:33:25Z

Thank you @LuisDeimos.

… single_text and zero seconds (LuisDeimos) This PR was squashed before being merged into the 2.7 branch (closes #20307). Discussion ---------- [Form] Fix Date\TimeType marked as invalid on request with single_text and zero seconds | Q | A | | --- | --- | | Branch? | 2.7 | | Bug fix? | yes | | New feature? | no | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | 20304 | | License | MIT | | Doc PR | | Fix: When using a form with an Time type with option 'widget' => 'single_text', and 0 is selected in the seconds, we obtain an TransformationFailedException "Unable to reverse value for property path "[time]": Data missing". Check ticket #20304 Commits ------- bcb03e0 [Form] Fix Date\TimeType marked as invalid on request with single_text and zero seconds

carsonbot added Status: Needs Review Form Bug labels Oct 26, 2016

LuisDeimos force-pushed the ticket_20304 branch from 7da4162 to aad154e Compare October 26, 2016 10:12

ro0NL reviewed Oct 26, 2016

View reviewed changes

LuisDeimos force-pushed the ticket_20304 branch from aad154e to 4b71d52 Compare October 26, 2016 21:11

ro0NL reviewed Oct 26, 2016

View reviewed changes

LuisDeimos force-pushed the ticket_20304 branch from d1fac6e to cb1a42a Compare October 26, 2016 22:12

LuisDeimos added 6 commits October 27, 2016 02:38

[Form] fixed Date\TimeType marked as invalid on request with single_t…

bf6979b

…ext and zero seconds (LuisDeimos)

[Form] fixed Date\TimeType marked as invalid on request with single_t…

6f26d21

…ext and zero seconds (LuisDeimos)

[Form] fixed Date\TimeType marked as invalid on request with single_t…

11f6502

…ext and zero seconds (LuisDeimos)

[Form] fixed Date\TimeType marked as invalid on request with single_t…

4adaa2a

…ext and zero seconds (LuisDeimos)

[Form] fixed Date\TimeType marked as invalid on request with single_t…

006f236

…ext and zero seconds (LuisDeimos)

[Form] fixed Date\TimeType marked as invalid on request with single_t…

ea9567e

…ext and zero seconds (LuisDeimos)

LuisDeimos force-pushed the ticket_20304 branch from cb1a42a to ea9567e Compare October 27, 2016 07:39

LuisDeimos changed the title ~~[WIP][Form] Fix Date\TimeType marked as invalid on request with single_text and zero seconds~~ [Form] Fix Date\TimeType marked as invalid on request with single_text and zero seconds Oct 29, 2016

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 12, 2016

fabpot closed this Nov 12, 2016

This was referenced Nov 17, 2016

Release v3.2.0-RC1 #20552

Merged

Release v2.7.21 #20573

Merged

Release v2.8.14 #20574

Merged

Release v3.1.7 #20575

Merged

Uh oh!

[Form] Fix Date\TimeType marked as invalid on request with single_text and zero seconds #20307

[Form] Fix Date\TimeType marked as invalid on request with single_text and zero seconds #20307

Uh oh!

Conversation

LuisDeimos commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuisDeimos Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Nov 12, 2016

Uh oh!

fabpot commented Nov 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LuisDeimos commented Oct 26, 2016 •

edited

Loading

ro0NL Oct 26, 2016 •

edited

Loading

LuisDeimos Oct 27, 2016 •

edited

Loading