[Form][TwigBridge] Don't render _method in form_rest() for a child form #23649

fmarchalemisys · 2017-07-24T16:54:36Z

The hidden _method must only be generated if the form is the top most form.

Always generating the hidden _method breaks forms using the POST method when they have children using the PUT method. If _method is generated for such a child form, it overrides the parent method and the form fails to validate.

See issue #23254

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

Always generating the hidden _method breaks forms using the POST method when they have children using the PUT method. The _method generated for a child form overrides the parent method and the form fails to validate. The hidden _method must only be generated if the form is the top most form. See issue #23254

xabbuh · 2017-07-24T17:41:30Z

src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig

@@ -303,7 +303,7 @@
        {% endif %}
    {%- endfor %}

-    {% if not form.methodRendered %}
+    {% if not form.methodRendered and form.parent is empty %}


Maybe use is null instead?

I copied the code from form_widget_compound() at line 18 of that very file.

xabbuh · 2017-07-24T17:41:44Z

ping @HeahDude @ogizanagi

ogizanagi · 2017-07-24T17:59:45Z

This looks good to me appart from @xabbuh's minor suggestion which I think should be fixed despite form_widget_compound. Congrats on your first PR @fmarchalemisys !

As requested, use "is null" instead of "is empty" to check if the form is a child or the top parent form. Only the top most form must output the hidden "_method".

fmarchalemisys · 2017-07-25T06:31:41Z

I made the requested change i.e. replace "is empty" with "is null".

BTW, I ran the test suite against Symfony 3.3.5, not against Symfony 2.7 which is the target of this PR. Should I change the "Test pass?" entry in the PR?

xabbuh · 2017-07-25T06:38:09Z

Thanks for your nice first contribution! And no, changing that line is not necessary. As you can see tests on AppVeyor (for Windows) and Travis CI (testing different PHP and dependency versions) all pass. So that's still true. :)

HeahDude · 2017-07-25T08:39:31Z

src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig

@@ -303,7 +303,7 @@
        {% endif %}
    {%- endfor %}

-    {% if not form.methodRendered %}
+    {% if not form.methodRendered and form.parent is null %}


It might break, if a field is named "parent". As discussed in #19492, I think we should be able to hardcode:

{% if not form.methodRendered and not form.hasParent %}

to know if a form view is a root.

I hesitated suggesting to add a FormView::needsMethodToBeRendered() method instead, which will do this check inside, as rendering the method input does only makes sense for a root form anyway and would avoid this issue.

Besides that, we already use form.parent in multiple places, so I don't think the related issue should be considered here.

Agreed, your solution sounds good though.

I'll suggest it in another PR then.

ogizanagi · 2017-07-25T09:25:06Z

Thanks for fixing this bug @fmarchalemisys.

… a child form (fmarchalemisys) This PR was squashed before being merged into the 2.7 branch (closes #23649). Discussion ---------- [Form][TwigBridge] Don't render _method in form_rest() for a child form The hidden `_method` must only be generated if the form is the top most form. Always generating the hidden `_method` breaks forms using the POST method when they have children using the PUT method. If `_method` is generated for such a child form, it overrides the parent method and the form fails to validate. See issue #23254 | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14261 | License | MIT | Doc PR | Commits ------- 973b2d3 [Form][TwigBridge] Don't render _method in form_rest() for a child form

…nd form fields (yceruto) This PR was merged into the 2.7 branch. Discussion ---------- [Form][TwigBridge] Fix collision between view properties and form fields | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18882 | License | MIT | Doc PR | TODO This introduce a new Twig test function `rootform` that guarantee the right access to the `parent` property of the form view. The rest of the properties (`vars` and `children`) are not used at least inside Symfony repo. I've chosen this solution because it doesn't [affect the design of the form view class/interface](https://github.com/symfony/symfony/pull/19492/files#diff-f60b55ea46e40b9c4475a1bd361f6940R168) and because [the problem happen only on Twig](https://github.com/twigphp/Twig/blob/fd98722d15996561f598f9181dbcef8432e9ff85/lib/Twig/Extension/Core.php#L1439-L1447). More details about the problem here: * #24892 * #19492 * #23649 (comment) _if this is approved_ we should update also: * [`foundation_5_layout.html.twig`](https://github.com/symfony/symfony/blob/336600857b8bb47d5a7ba3d1924a0e7a3e2af7a8/src/Symfony/Bridge/Twig/Resources/views/Form/foundation_5_layout.html.twig#L321-L326) in `3.3` (done in #25305) * [`bootstrap_4_layout.html.twig`](https://github.com/symfony/symfony/blob/76d356f36a692dd8ad796de363484c97d6731d1f/src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig#L176) in `3.4` (done in #25306) Commits ------- 8505894 Fix collision between view properties and form fields

carsonbot added Status: Needs Review Form TwigBridge Bug labels Jul 24, 2017

xabbuh reviewed Jul 24, 2017

View reviewed changes

xabbuh added this to the 2.7 milestone Jul 24, 2017

[Form][TwigBridge] Use null to check for top most form

be4eb82

As requested, use "is null" instead of "is empty" to check if the form is a child or the top parent form. Only the top most form must output the hidden "_method".

xabbuh approved these changes Jul 25, 2017

View reviewed changes

ogizanagi approved these changes Jul 25, 2017

View reviewed changes

HeahDude reviewed Jul 25, 2017

View reviewed changes

HeahDude approved these changes Jul 25, 2017

View reviewed changes

ogizanagi closed this Jul 25, 2017

This was referenced Aug 1, 2017

Release v2.7.33 #23734

Merged

Release v2.8.26 #23735

Merged

Release v3.2.13 #23736

Merged

Release v3.3.6 #23738

Merged

yceruto mentioned this pull request Dec 1, 2017

[Form][TwigBridge] Fix collision between view properties and form fields #25236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form][TwigBridge] Don't render _method in form_rest() for a child form #23649

[Form][TwigBridge] Don't render _method in form_rest() for a child form #23649

fmarchalemisys commented Jul 24, 2017

xabbuh Jul 24, 2017

fmarchalemisys Jul 24, 2017

xabbuh commented Jul 24, 2017

ogizanagi commented Jul 24, 2017

fmarchalemisys commented Jul 25, 2017

xabbuh commented Jul 25, 2017

HeahDude Jul 25, 2017

ogizanagi Jul 25, 2017 •

edited

Loading

HeahDude Jul 25, 2017

ogizanagi Jul 25, 2017

ogizanagi commented Jul 25, 2017

[Form][TwigBridge] Don't render _method in form_rest() for a child form #23649

[Form][TwigBridge] Don't render _method in form_rest() for a child form #23649

Conversation

fmarchalemisys commented Jul 24, 2017

xabbuh Jul 24, 2017

Choose a reason for hiding this comment

fmarchalemisys Jul 24, 2017

Choose a reason for hiding this comment

xabbuh commented Jul 24, 2017

ogizanagi commented Jul 24, 2017

fmarchalemisys commented Jul 25, 2017

xabbuh commented Jul 25, 2017

HeahDude Jul 25, 2017

Choose a reason for hiding this comment

ogizanagi Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

HeahDude Jul 25, 2017

Choose a reason for hiding this comment

ogizanagi Jul 25, 2017

Choose a reason for hiding this comment

ogizanagi commented Jul 25, 2017

ogizanagi Jul 25, 2017 •

edited

Loading