[Form][FrameworkBundle][Bridge] Add a DateInterval form type #16809

MisatoTremor · 2015-12-02T15:22:18Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#13389
License	MIT
Doc PR	symfony/symfony-docs#4817

Replaces #15030

MisatoTremor · 2016-03-11T12:40:18Z

For the upcoming hack day and if you find the time to review this for any issues I should fix, a short notice about that would be nice.

xabbuh · 2016-03-11T19:59:45Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml

@@ -73,6 +73,9 @@
        <service id="form.type.datetime" class="Symfony\Component\Form\Extension\Core\Type\DateTimeType">
            <tag name="form.type" />
        </service>
+        <service id="form.type.dateinterval" class="Symfony\Component\Form\Extension\Core\Type\DateIntervalType">
+            <tag name="form.type" alias="dateinterval" />


The alias attribute is now not needed anymore.

MisatoTremor · 2016-03-13T17:48:06Z

Type alias removed and rebased.

@xabbuh Sorry for being late...

fabpot · 2016-06-16T16:57:07Z

👍

HeahDude · 2016-06-17T00:22:45Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml

@@ -73,6 +73,9 @@
        <service id="form.type.datetime" class="Symfony\Component\Form\Extension\Core\Type\DateTimeType">
            <tag name="form.type" />
        </service>
+        <service id="form.type.dateinterval" class="Symfony\Component\Form\Extension\Core\Type\DateIntervalType">


A service for this type is not needed as it has no dependency, we just use FQCN since 2.8. Btw almost all the others have been deprecated in 3.1.

HeahDude · 2016-06-17T00:58:36Z

Apart some very minor comments, this PR looks awesome!

MisatoTremor · 2016-06-17T09:25:54Z

@HeahDude Thanks for your comments. Some of these stem from the fact that this PR was originally based on 2.7. I have updated it accordingly for 3.1+ now.

The in-linings would break the soft limit, so I left them out.

fabpot · 2016-06-17T09:28:54Z

Can you inline even if it breaks the soft limit? We are not following any limits for line length.

MisatoTremor · 2016-06-17T10:15:39Z

@fabpot OK, done :)

xabbuh · 2016-06-18T06:43:05Z

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

+    private $fields;
+
+    /**
+     * Constructor.


should be removed

MisatoTremor · 2016-06-18T11:29:50Z

@xabbuh I did this in sync with the existing classes, i.e. DateTimeToArrayTransformer. Should I really change it only for this one (now)?

fabpot · 2016-06-21T14:30:02Z

@MisatoTremor Let's do the change for this one for now.

MisatoTremor · 2016-06-21T14:41:06Z

@fabpot Done :)

fabpot · 2016-06-21T16:02:49Z

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

+            throw new TransformationFailedException(sprintf('The fields "%s" should not be empty', implode('", "', $emptyFields)));
+        }
+        if (isset($value['invert']) && !is_bool($value['invert'])) {
+            throw new UnexpectedTypeException($value['invert'], 'boolean');


The resulting error message won't help the end user as 'invert' won't be anywhere, right?

I don't quite understand what you mean with "won't be anywhere".

The error message is going to be Expected argument of type "boolean", xxx" given. I don't think there will be anything in the context mentioning that the error comes from the invert value and not the whole value for the form.

You are right, I oversaw this when applying HeahDudes comments. Fixed

Also add dateinterval widget to twig templates.

fabpot · 2016-06-22T12:02:53Z

Thank you @MisatoTremor.

…m type (MisatoTremor) This PR was merged into the 3.2-dev branch. Discussion ---------- [Form][FrameworkBundle][Bridge] Add a DateInterval form type | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13389 | License | MIT | Doc PR | symfony/symfony-docs#4817 Replaces #15030 Commits ------- f7669be [Form] Add a DateInterval form type Also add dateinterval widget to twig templates.

Taluu · 2016-06-22T12:03:28Z

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

+    public function __construct(array $fields = null, $pad = false)
+    {
+        if (null === $fields) {
+            $fields = array('years', 'months', 'days', 'hours', 'minutes', 'seconds', 'invert');


I would use an associative array here instead of plain array (because you often use array_flip, and isset($array[$key]) is faster than in_array($key, $array))

MisatoTremor · 2016-06-22T12:29:57Z

Thanks all for your help improving this and thanks for merging.

weaverryan · 2016-07-10T16:04:05Z

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

+            $fields = array('years', 'months', 'days', 'hours', 'minutes', 'seconds', 'invert');
+        }
+        $this->fields = $fields;
+        $this->pad = (bool) $pad;


@MisatoTremor We're missing the actual pad property at the top of this class

This PR was merged into the 3.2-dev branch. Discussion ---------- [Form][#16809] add missing pad property | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16809 (comment) | License | MIT | Doc PR | Commits ------- 647e6ba [#16809] add missing pad property

carsonbot added the Status: Needs Review label Dec 2, 2015

javiereguiluz added Form Feature FrameworkBundle labels Mar 3, 2016

xabbuh reviewed Mar 11, 2016
View reviewed changes

MisatoTremor force-pushed the add_dateinterval_type_3_1 branch from 95037ca to 3586785 Compare March 13, 2016 17:46

HeahDude reviewed Jun 17, 2016
View reviewed changes

MisatoTremor force-pushed the add_dateinterval_type_3_1 branch from 3586785 to 77eff03 Compare June 17, 2016 09:23

MisatoTremor force-pushed the add_dateinterval_type_3_1 branch from 77eff03 to 39ac6de Compare June 17, 2016 10:15

xabbuh reviewed Jun 18, 2016
View reviewed changes

MisatoTremor force-pushed the add_dateinterval_type_3_1 branch from 39ac6de to 2f8f79d Compare June 21, 2016 14:40

fabpot reviewed Jun 21, 2016
View reviewed changes

MisatoTremor force-pushed the add_dateinterval_type_3_1 branch from 2f8f79d to 43ad054 Compare June 22, 2016 11:46

[Form] Add a DateInterval form type

f7669be

Also add dateinterval widget to twig templates.

MisatoTremor force-pushed the add_dateinterval_type_3_1 branch from 43ad054 to f7669be Compare June 22, 2016 11:52

fabpot merged commit f7669be into symfony:master Jun 22, 2016

Taluu reviewed Jun 22, 2016
View reviewed changes

HeahDude mentioned this pull request Jun 22, 2016

Add dateinterval type reference symfony/symfony-docs#4817

Merged

weaverryan reviewed Jul 10, 2016
View reviewed changes

xabbuh added a commit to xabbuh/symfony that referenced this pull request Jul 10, 2016

[symfony#16809] add missing pad property

647e6ba

xabbuh mentioned this pull request Jul 10, 2016

[Form][#16809] add missing pad property #19331

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form][FrameworkBundle][Bridge] Add a DateInterval form type #16809

[Form][FrameworkBundle][Bridge] Add a DateInterval form type #16809

MisatoTremor commented Dec 2, 2015

MisatoTremor commented Mar 11, 2016

xabbuh Mar 11, 2016

MisatoTremor commented Mar 13, 2016

fabpot commented Jun 16, 2016

HeahDude Jun 17, 2016

HeahDude commented Jun 17, 2016

MisatoTremor commented Jun 17, 2016

fabpot commented Jun 17, 2016

MisatoTremor commented Jun 17, 2016

xabbuh Jun 18, 2016

MisatoTremor commented Jun 18, 2016

fabpot commented Jun 21, 2016

MisatoTremor commented Jun 21, 2016

fabpot Jun 21, 2016

MisatoTremor Jun 22, 2016

fabpot Jun 22, 2016

MisatoTremor Jun 22, 2016

fabpot commented Jun 22, 2016

Taluu Jun 22, 2016

MisatoTremor commented Jun 22, 2016

weaverryan Jul 10, 2016

[Form][FrameworkBundle][Bridge] Add a DateInterval form type #16809

[Form][FrameworkBundle][Bridge] Add a DateInterval form type #16809

Conversation

MisatoTremor commented Dec 2, 2015

MisatoTremor commented Mar 11, 2016

Choose a reason for hiding this comment

MisatoTremor commented Mar 13, 2016

fabpot commented Jun 16, 2016

Choose a reason for hiding this comment

HeahDude commented Jun 17, 2016

MisatoTremor commented Jun 17, 2016

fabpot commented Jun 17, 2016

MisatoTremor commented Jun 17, 2016

Choose a reason for hiding this comment

MisatoTremor commented Jun 18, 2016

fabpot commented Jun 21, 2016

MisatoTremor commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jun 22, 2016

Choose a reason for hiding this comment

MisatoTremor commented Jun 22, 2016

Choose a reason for hiding this comment