[Form] form_theme now sets the theme to all children of the specified form #6025

Closed
wants to merge 2 commits into
from

Projects

None yet

8 participants

@mvrhov
Contributor
mvrhov commented Nov 15, 2012

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: N/A
Todo: -
License of the code: MIT
Documentation PR: N/A

@bamarni bamarni and 1 other commented on an outdated diff Nov 16, 2012
src/Symfony/Component/Form/AbstractRendererEngine.php
// Do not cast, as casting turns objects into arrays of properties
- $this->themes[$cacheKey] = is_array($themes) ? $themes : array($themes);
-
- // Unset instead of resetting to an empty array, in order to allow
- // implementations (like TwigRendererEngine) to check whether $cacheKey
- // is set at all.
- unset($this->resources[$cacheKey]);
- unset($this->resourceHierarchyLevels[$cacheKey]);
+ $themes = is_array($themes) ? $themes : array($themes);
+
+ $set = function($key) use (&$themes) {
+ $this->themes[$key] = $themes;
@bamarni
bamarni Nov 16, 2012 Contributor

this should fail under php 5.4

@mvrhov
mvrhov Nov 16, 2012 Contributor

No this fails under 5.3 And works as expected under 5.4. I'm running on 5.4 and thus didn't catch it. I've seen the reports from travis a couple of hours ago and I plan to extract this into helper method... But I really can't find the proper name for it.

@bamarni
bamarni Nov 16, 2012 Contributor

You could also do like this :

$renderer = $this;
$set = function($key) use ($renderer, &$themes) {
// ...
}
mvrhov added some commits Nov 15, 2012
@mvrhov mvrhov form_theme now sets the theme to all children of the specified form 3051c8c
@mvrhov mvrhov Handle theme changes on deeper levels
0eb1c31
@fabpot
Member
fabpot commented Dec 11, 2012

Can you add some unit tests please?

@vicb
Contributor
vicb commented Dec 11, 2012

@bschussek Have you reviewed the code ? It used to work the other way around (ie walk up the parents to find the theme)

@mvrhov
Contributor
mvrhov commented Dec 11, 2012

@fabpot gladly, but I don't know how to in this case.. the AbstractDivLayoutTest class has annotations like @ dataProvider themeInheritanceProvider, but I cannot find either class or method with that name in any of the files inside Form/Tests ..

@vicb
Contributor
vicb commented Dec 11, 2012

Should be a public method of the tc

----- Reply message -----
De : "Miha Vrhovnik" notifications@github.com
Pour : "symfony/symfony" symfony@noreply.github.com
Cc : "Victor Berchet" victor@suumit.com
Objet : [symfony] form_theme now sets the theme to all children of the specified form (#6025)
Date : mar., déc. 11, 2012 21:15
@fabpot gladly, but I don't know how to in this case.. the AbstractDivLayoutTest class has annotations like @ dataProvider themeInheritanceProvider, but I cannot find this class.

Reply to this email directly or view it on GitHub.

@mvrhov
Contributor
mvrhov commented Dec 12, 2012

@vicb: Should be yeah, but really I can't find it. and I have no idea how those tests pass.

@mvrhov
Contributor
mvrhov commented Dec 12, 2012

Ah completely different namespace.... Thanks.

@webmozart
Member

@vicb is right. Theme inheritance works for me. What is the motivation for this PR?

@mvrhov
Contributor
mvrhov commented Dec 13, 2012

Theme inheritance doesn't work more than one level. Here is the screenshot of the same form rendered with and without patch.
PR6062

@webmozart
Member

Could you please provide a fork of symfony-standard that reproduces the problem?

@mvrhov
Contributor
mvrhov commented Dec 13, 2012

@vicb The subform on the screenshot is added inside PRE_SET_DATA event :)

@vicb
Contributor
vicb commented Dec 13, 2012

yep, please forget what I said !
On 12/13/2012 05:23 PM, Miha Vrhovnik wrote:

@vicb https://github.com/vicb The subform on the screenshot is added
inside PRE_SET_DATA event :)


Reply to this email directly or view it on GitHub
#6025 (comment).

@mvrhov
Contributor
mvrhov commented Dec 13, 2012

@bschussek: While trying to provide a testcase with symfony-standard fork I've discovered why we need the code in this PR.
I'm using a GenemuFormBundle. The fields in this particular bundle can have the css or javascript associated to it. Now when outputting either the js or css FormRendererInterface->searchAndRenderBlock method gets called. This method will cache the whole form inheritance tree.
IMO this patch should also fix the case you presented in #5438

@vicb
Contributor
vicb commented Dec 14, 2012

@mvrhov from what you say (sorry I haven't checked your code neither I am very familiar with the "new" rendering code) this looks like a bad idea to me. If there is a problem with the current theme inheritance, it should be patched but we should not introduce a new mechanism to hide a "problem" (if it is one).

@webmozart
Member

@mvrhov Once again, could you please provide a failing test case so that I can verify the implementation?

@mvrhov mvrhov added a commit to mvrhov/symfony-standard that referenced this pull request Dec 11, 2013
@mvrhov mvrhov example for symfony/symfony#6025 5e21726
@mvrhov
Contributor
mvrhov commented Dec 11, 2013

@bschussek: Here you go

@mvrhov mvrhov added a commit to mvrhov/symfony-standard that referenced this pull request Dec 11, 2013
@mvrhov mvrhov example for symfony/symfony#6025 3c62148
@cordoval
Contributor

thanks @mvrhov , this is a great help 👶

@webmozart
Member

Thank you @mvrhov for the test project! Sorry also for the late response.

I just tried the project and what I see is this:

screenshot

Am I supposed to see this or did you forget to include some styles? How can I spot the bug?

@mvrhov
Contributor
mvrhov commented Jul 30, 2014

Yes, the stylesheet is missing as it's not important. Take a look at Welcome/index.html.twig

@webmozart
Member

I would really appreciate if you could simplify the form and describe exactly 1) what you expect to see and 2) what you actually see. It's really hard to figure that out from a big junk of HTML without any detailed explanation.

@mvrhov
Contributor
mvrhov commented Jul 30, 2014

I've thought that what I've written as a comment is descriptive enough.
{# when commented the correct result is displayed e.g. the rows in table don't have labels #} (That's what's changed in the 2nd form template.)

@webmozart
Member

I don't have enough information to reproduce and fix this problem. Closing.

@webmozart webmozart closed this Jun 16, 2015
@mvrhov
Contributor
mvrhov commented Jun 16, 2015

@webmozart: Would you please explain what exactly is it missing? You have the fork. The code is documented.

@webmozart
Member

To quote myself:

I would really appreciate if you could simplify the form and describe exactly 1) what you expect to see and 2) what you actually see. It's really hard to figure that out from a big junk of HTML without any detailed explanation.

I know long explanations can be tedious, but I can't fix something that i don't understand.

@mvrhov
Contributor
mvrhov commented Aug 6, 2015

@webmozart:
IMO there is no need to simplify the html. If you don't notice the difference (after following the instructions inside src\Acme\DemoBundle\Resources\views\Welcome\index.html) it's in the labels in this particular case.

As you are probably aware the default form theme associates a label with each input.
In 2nd level the form is formated inside a table, so the labels on each input are unnecessary. And the theme removes them for the 2nd level. However due to a bug this seems not to work. The patch fixes that.

How to achieve this with the demo is written in a comment inside as I have said before.

@mvrhov
Contributor
mvrhov commented Aug 6, 2015

P.S. This really needs to get into the core as this will be impossible to fix in 3.0 as the class parameters are gone. And there is no way for me to ship the workaround in my projects.

Please re-open

@sstok
Contributor
sstok commented Aug 6, 2015

I think I understand what your problem is, you render the form and then you load an additional the theme into a subform. But as the previous theme (in the parent) is already cached the new one is never loaded.

This seems like an edge case and I'm not sure if its a good idea to support this, theming subforms by loading the theme into the subform looks wrong and edgy.

@mvrhov
Contributor
mvrhov commented Aug 6, 2015

Yes, sstok, you understand correctly. I don't think that this is such edge case. Or it is as there would be more reports like this.
I'd say that changing a subform theme is no different than changing the theme for a couple of inputs.

@mvrhov
Contributor
mvrhov commented Nov 26, 2015

@webmozart: Please reconsider this.. sstok described perfectly. and this is now impossible to achieve in sf 3

@mvrhov
Contributor
mvrhov commented Feb 27, 2016

@webmozart: I really need this in core, because now that 3.0 is out I'm unable to style my forms properly.

@webmozart
Member

@mvrhov Thanks for bringing this up again Miha. If somebody else can reproduce this and write a corresponding test case, I'm happy to reconsider.

@webmozart webmozart reopened this Feb 29, 2016
@backbone87
Contributor

If i get this correctly, then the cause for this problems lies with a potential missuse of the FormView.

FormViews are stateful (easily spotted by the "isRendered" method), but besides the "rendered" flag, also some caching for the form theme is done in the form view.

But because form_javascript and form_stylesheet (both from the custom bundle), traverse the whole form view tree to collect the styles and scripts, the form theme gets also processed and cached for each node in the form view tree.

Both functions are called at https://github.com/mvrhov/symfony-standard/blob/testcase_6025/src/Acme/DemoBundle/Resources/views/Welcome/index.html.twig#L15-L16 before the actual form rendering is done at https://github.com/mvrhov/symfony-standard/blob/testcase_6025/src/Acme/DemoBundle/Resources/views/Welcome/index.html.twig#L19-L27 . So the form theme set for the form twig-variable at the point of the first tree traversal at https://github.com/mvrhov/symfony-standard/blob/testcase_6025/src/Acme/DemoBundle/Resources/views/Welcome/index.html.twig#L15 is used for each node in the form view tree (and therefore cached in each one).

However at https://github.com/mvrhov/symfony-standard/blob/testcase_6025/src/Acme/DemoBundle/Resources/views/Welcome/index.html.twig#L26 the form theme gets overridden for a single node form.prices (a collection form type). But since the entry (children of form.prices) form views were already visited, when form_javascript was called at https://github.com/mvrhov/symfony-standard/blob/testcase_6025/src/Acme/DemoBundle/Resources/views/Welcome/index.html.twig#L15 at which time the only form theme set (implicitly via default form theme) was already used and cached for every descendant of the form.prices form view tree node.

@webmozart i hope this helps to get a grasp on the problem. (and i also agree that the presented demo is too complex to be easily scanned for the concrete problem)

@sstok i think subform theming is fine in general. the problem is that form theme "resolution" is done only once for each node not having an explicit theme set in the form view tree and since one doesnt know if, for example, form_start(myForm) already visited the whole form view tree internally, and therefore triggered the form resolution on every node, one must explicitly recursively overwrite the form theme for a node in the form view tree.

@webmozart
Member

Thanks a lot for investigating @backbone87, the issue is much clearer now. Does that mean this could be fixed by moving the {% form_theme ... %} statement before the custom form_javascript() call?

@backbone87
Contributor

from my understanding: yes

@webmozart
Member

@mvrhov Could you verify please?

@mvrhov
Contributor
mvrhov commented Mar 10, 2016

That "won't" work, when your forms are more than 1 level deep.
What I'm trying to say is that it would be cumbersome/weird to set all the themes up top and not at the place you actually need it.

@webmozart
Member

@mvrhov What about moving the form_stylesheet() and form_javascript() calls after the form rendering code?

@backbone87
Contributor

@mvrhov the "where you need it" is before you call form_stylesheet and/or form_javascript because they internally traverse the whole form view tree to collect the styles the scripts. you need "a priori" knowledge about the inner workings of the functions/methods working on form views. thats the problem which i already mentioned:

i think subform theming is fine in general. the problem is that form theme "resolution" is done only once for each node not having an explicit theme set in the form view tree and since one doesnt know if, for example, form_start(myForm) already visited the whole form view tree internally, and therefore triggered the form resolution on every node, one must explicitly recursively overwrite the form theme for a node in the form view tree.

even if the concrete block to use, doesnt get cached: in your example the collected styles and scripts from your form_stylesheet and form_javascript calls base on the application of the default form theme to the whole form view tree, which could emit very different styles and scripts compared to (recursively) using another theme for a particular form view subtree. either you must set all form themes within the form view tree before calling any rendering method or you must know what parts of the form view tree the called rendering method is using.

@mvrhov
Contributor
mvrhov commented Mar 18, 2016

Putting form_stylesheet and form_javascript before form_end tag seems to work at least in the demo I provided.
I don't work on the project this was a problem initially anymore. So I cannot check there.
I also don't know why they were at the top i might be that that was requirement back then.

@mvrhov mvrhov closed this Mar 18, 2016
@webmozart
Member

Thanks a lot for verifying @mvrhov! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment