Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Form] Added Form view parent method #19492

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

Closed
wants to merge 7 commits into from
Closed

[Form] Added Form view parent method #19492

wants to merge 7 commits into from

Conversation

bhavin-nakrani
Copy link
Contributor

Q A
Branch? "master" for new features
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #18882
License MIT
Doc PR reference to the documentation PR, if any

Adding FormView::hasParent() which should be hardcoded in twig to prevent accessing the property: form.hasParent.

@ogizanagi
Copy link
Contributor

Just asking, but what about isRoot() instead, as in FormInterface::isRoot() ?

@bhavin-nakrani
Copy link
Contributor Author

@ogizanagi FormInterface::isRoot() is not accessible from twig and FormView does not implement this interface.

@bhavin-nakrani
Copy link
Contributor Author

Does anyone knows why i am getting this error in build AppVeyor build failed

@HeahDude
Copy link
Contributor

HeahDude commented Jul 31, 2016

The problem here is that it does not make such sense to add methods like isRoot or hasParent on a value object with public properties as FormView, and using form.parent or form.root has the same effect in Twig that checking the public property so adding it to hardcode it looks weird somehow.

The real issue I guess come from Twig and how it tries to access an attributes because FormView has both public properties and array access, but the last has a precedence so it will first look for named children.

Maybe we should just better document how to access children via offset get when they have the same name as a view property (e.g form['parent'], form['children'], form['vars']) and suggest to avoid such collision?

@@ -44,6 +44,9 @@ public function testPassedCallbackIsExecuted()
$this->assertTrue($executed);
}

/**
* @return array
*/
public function provideCommandsAndOutput()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your IDE can already determine the return type, in this state it doesn't really add a value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix does not belong to the PR's scope anyway.

@ogizanagi
Copy link
Contributor

ogizanagi commented Jul 31, 2016

@ogizanagi FormInterface::isRoot() is not accessible from twig and FormView does not implement this interface.

@bhavin-nakrani : I was asking about naming the method isRoot instead of hasParent and reverse the logic inside 😅

The real issue I guess come from Twig and how it tries to access an attributes because FormView has both public properties and array access, but the last has a precedence so it will first look for named children.

Maybe we should just better document how to access children via offset get when they have the same name as a view property (e.g form['parent'], form['children'], form['vars']) and suggest to avoid such collision?

@HeahDude : IIUC, this won't solve the mentioned issue, where form.parent will always return something in case the form has a child named parent. Thus, no simple way to know if it's a child or a parent :/

@HeahDude
Copy link
Contributor

@ogizanagi Yes what I implicitly suggest here is to think about the precedence in Twig, checking object (properties and methods) in an early return before checking for array and ArrayAccess, but that sounds like a big BC break (maybe doable with the upcoming 2.0?).

The opposite is not working either because if one wants to test if a form has a child named "parent" it may return the actual parent even using form['parent'] because [] has the same behavior for objects since it is the only way to make the call dynamic using a twig variable to determine the property/method.

So it may worth to say a word in the doc about it anyway. Although hardcoding hasMethod might help to ensure the checks both ways but IMO it keeps looking like a hack adding this method in FormView to work around this.

@ro0NL
Copy link
Contributor

ro0NL commented Oct 30, 2016

Imo. it's worth reserving form.parent as is. A child named liked like that can be accessed via form.children['parent'], same for having form.root as proposed in #20369

@fabpot
Copy link
Member

fabpot commented Mar 22, 2017

Closing this PR as there does not seem to have any more activity and the PR cannot be merged as is. Feel free to reopen though.

fabpot added a commit that referenced this pull request Dec 4, 2017
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.