-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
Just asking, but what about |
@ogizanagi |
Does anyone knows why i am getting this error in build |
The problem here is that it does not make such sense to add methods like The real issue I guess come from Twig and how it tries to access an attributes because 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 |
@@ -44,6 +44,9 @@ public function testPassedCallbackIsExecuted() | ||
$this->assertTrue($executed); | ||
} | ||
|
||
/** | ||
* @return array | ||
*/ | ||
public function provideCommandsAndOutput() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@bhavin-nakrani : I was asking about naming the method
@HeahDude : IIUC, this won't solve the mentioned issue, where |
@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 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 So it may worth to say a word in the doc about it anyway. Although hardcoding |
Imo. it's worth reserving |
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. |
…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
Adding FormView::hasParent() which should be hardcoded in twig to prevent accessing the property: form.hasParent.