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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Dec 4, 2017

Conversation

Projects
None yet
8 participants
@yceruto
Contributor

yceruto commented Dec 1, 2017

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 and because the problem happen only on Twig.

More details about the problem here:

if this is approved we should update also:

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Dec 1, 2017

Contributor

I tried something similar with form.root (#20369). I agree a twig solution for this is much better IMHO 👍

But is a new feature :)

Should we add a method to get the root from, along with the test.

Contributor

ro0NL commented Dec 1, 2017

I tried something similar with form.root (#20369). I agree a twig solution for this is much better IMHO 👍

But is a new feature :)

Should we add a method to get the root from, along with the test.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

OK as bug fix to me.

Member

nicolas-grekas commented Dec 1, 2017

OK as bug fix to me.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Dec 1, 2017

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Dec 1, 2017

Contributor

But is a new feature :)

@ro0NL I don't think so :) this fix a bug that affects all previous versions.

Should we add a method to get the root from, along with the test.

This is a new feature that will be great to have, a lot of third-party bundle check for form.parent.vars incurring the same problem.

Contributor

yceruto commented Dec 1, 2017

But is a new feature :)

@ro0NL I don't think so :) this fix a bug that affects all previous versions.

Should we add a method to get the root from, along with the test.

This is a new feature that will be great to have, a lot of third-party bundle check for form.parent.vars incurring the same problem.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Dec 4, 2017

Contributor

(AppVeyor failure unrelated)

/cc @ogizanagi @HeahDude @xabbuh

Contributor

yceruto commented Dec 4, 2017

(AppVeyor failure unrelated)

/cc @ogizanagi @HeahDude @xabbuh

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Dec 4, 2017

Member

LGTM 👍

Member

xabbuh commented Dec 4, 2017

LGTM 👍

@xabbuh

xabbuh approved these changes Dec 4, 2017

@HeahDude

LGTM, thanks for fixing this!

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Dec 4, 2017

Contributor

I've created #25305 for foundation_5_layout.html.twig and #25306 for bootstrap_4_layout.html.twig.

Contributor

yceruto commented Dec 4, 2017

I've created #25305 for foundation_5_layout.html.twig and #25306 for bootstrap_4_layout.html.twig.

@fabpot

fabpot approved these changes Dec 4, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Dec 4, 2017

Member

Thank you @yceruto.

Member

fabpot commented Dec 4, 2017

Thank you @yceruto.

@fabpot fabpot merged commit 8505894 into symfony:2.7 Dec 4, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Dec 4, 2017

bug #25236 [Form][TwigBridge] Fix collision between view properties a…
…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

fabpot added a commit that referenced this pull request Dec 4, 2017

bug #25305 [Form][TwigBridge] Fix collision between view properties a…
…nd form fields (yceruto)

This PR was merged into the 3.3 branch.

Discussion
----------

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

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

Require #25236 merged in 3.3

Commits
-------

888b48a Fix collision between view properties and form fields

symfony-splitter pushed a commit to symfony/twig-bridge that referenced this pull request Dec 4, 2017

bug #25305 [Form][TwigBridge] Fix collision between view properties a…
…nd form fields (yceruto)

This PR was merged into the 3.3 branch.

Discussion
----------

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

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

Require symfony/symfony#25236 merged in 3.3

Commits
-------

888b48a89c Fix collision between view properties and form fields
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Dec 4, 2017

Member

@yceruto Can you submit a PR on the docs to make the needed changes? Thanks.

Member

fabpot commented Dec 4, 2017

@yceruto Can you submit a PR on the docs to make the needed changes? Thanks.

symfony-splitter pushed a commit to symfony/twig-bridge that referenced this pull request Dec 4, 2017

bug #25306 [Form][TwigBridge] Fix collision between view properties a…
…nd form fields (yceruto)

This PR was merged into the 3.4 branch.

Discussion
----------

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

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

Require symfony/symfony#25236 merged in 3.4

Commits
-------

c330965cfb Fix collision between view properties and form fields

fabpot added a commit that referenced this pull request Dec 4, 2017

bug #25306 [Form][TwigBridge] Fix collision between view properties a…
…nd form fields (yceruto)

This PR was merged into the 3.4 branch.

Discussion
----------

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

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

Require #25236 merged in 3.4

Commits
-------

c330965 Fix collision between view properties and form fields

@yceruto yceruto deleted the yceruto:root_form_27 branch Dec 4, 2017

javiereguiluz added a commit to symfony/demo that referenced this pull request Dec 7, 2017

bug #726 Use rootform test to check by root form view (yceruto)
This PR was merged into the master branch.

Discussion
----------

Use rootform test to check by root form view

This is consistent with the latest changes in the form themes and avoids any future collision between form view properties and form fields.

See symfony/symfony#25236

Commits
-------

bb11ddf Use rootform test to check by root form view

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jul 6, 2018

minor #10022 Document Twig test "rootform" (yceruto, javiereguiluz)
This PR was merged into the 2.8 branch.

Discussion
----------

Document Twig test "rootform"

Documents PR symfony/symfony#25236 and closes #8829, sorry for the delay.

Commits
-------

cf355c9 Reworded the code comments
423bfae Minor reword
740803e Reword
fd433a1 Document Twig test "rootform"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment