Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] [TwigBridge] Added option to disable usage of default themes when rendering a form#22610
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| class FormThemeNodeextends \Twig_Node | ||
| { | ||
| publicfunction__construct(\Twig_Node$form,\Twig_Node$resources,$lineno,$tag =null) | ||
| publicfunction__construct(\Twig_Node$form,\Twig_Node$resources,$only,$lineno,$tag =null) |
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.
Should be added as last argument with defaultfalse to keep BC.
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.
Isn't this class considered an internal detail?
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.
Actually it's not marked as is, and anyone could extend the parser and the node to use custom implementation.
I don't know if it's a a doable/valid use case, but this API is public for now anyway, so BC should be preserved.
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.
@HeahDude Will do! As I am currently on vacation, give me couple of weeks and I'll do it.
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.
@HeahDude Placed the$only argument at the end and rebased on top of3.4.
bdd1ffc to0d3da35Compareemodric commentedSep 25, 2017
Hi, what is the status PR? Do you think it could end up in 3.4? |
731de67 to627eeecCompare| * default themes to render the view | ||
| */ | ||
| publicfunctionsetTheme(FormView$view,$themes); | ||
| publicfunctionsetTheme(FormView$view,$themes,$useDefaultThemes =true); |
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.
adding the argument in the interface is a BC break. So this cannot be done before 4.0 (and 3.4 should trigger a deprecation warning in case it finds an implementation of the interface without the extra optional attribute)
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.
Okay, makes sense. How should I proceed then?
| * default themes to render the view | ||
| */ | ||
| publicfunctionsetTheme(FormView$view,$themes); | ||
| publicfunctionsetTheme(FormView$view,$themes,$useDefaultThemes =true); |
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.
same here
fabpot commentedOct 1, 2017
emodric commentedOct 1, 2017
@fabpot Sure, but how do you want me to handle it? Open a new PR for deprecations only against 3.4 and target this one for master or did you have something else in mind? |
fabpot commentedOct 1, 2017
You need to remove the argument and use |
emodric commentedOct 1, 2017
Ah okay, I figured you wanted the feature to go in only in 4.0. I'll do it first thing tomorrow morning. |
emodric commentedOct 2, 2017
@fabpot I've updated the code to include requested deprecations. I updated the tests too in order not to trigger deprecation notices in them. There is a test failure in Twig bridge tests, but I don't see how it is related to this change. |
nicolas-grekas left a comment
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.
(failure unrelated)
| $this->renderer->setTheme($view,$themes); | ||
| $args =func_get_args(); | ||
| if (!isset($args[2])) { | ||
| @trigger_error(sprintf('Calling setTheme method of %s without the third boolean argument $useDefaultThemes is deprecated since 3.4 and will be removed in 4.0. Call the method with the third argument included and use boolean true to keep current behaviour.',self::class),E_USER_DEPRECATED); |
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.
I don't understand why we would want to force people to pass a third argument, which should betrue by default.
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.
Hm.. I'm not sure what exactly deprecation message should be then if the third argument is optional? Can't we just use it if it's there, and if not, default totrue?
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.
I think this is the right approach.
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.
Okay, will fix it ASAP. Where do you want me to document the fact that in 4.0 the interface is going to receive a third argument? I've seen somewhere in the codebase the usage of an inline comment in the style ofpublic function setTheme(FormView $view, $themes /* , $useDefaultThemes = true */ );
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.
Yes, that's what we are doing.
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.
@fabpot Alright, I've updated the code now as per instructions.
3bed88b to1d89877Compare| * in the engine, will be added to the interface in 4.0 | ||
| */ | ||
| publicfunctionsetTheme(FormView$view,$themes); | ||
| publicfunctionsetTheme(FormView$view,$themes/*, $useDefaultThemes = true */); |
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.
in all places where this is added, the code should throw a deprecation when the argument is not provided (using func_num_args, see other places in the code base).
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.
@nicolas-grekas As per@fabpot, deprecation notice is not needed if the argument is optional?
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.
I think the current code is good as is :)
…hen rendering a form
fabpot commentedOct 13, 2017
Thank you@emodric. |
…efault themes when rendering a form (emodric)This PR was merged into the 3.4 branch.Discussion----------[Form] [TwigBridge] Added option to disable usage of default themes when rendering a form| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR |symfony/symfony-docs#8495This adds a possibility to use `only` keyword in `form_theme` tag to disable usage of globally defined form themes, e.g.`{% form_theme form with ['common.html.twig', 'form/fields.html.twig'] only %}`Otherwise, in order to completely control the rendering of the forms (for example in custom admin interfaces), one would need to use a form theme which has all the possible twig blocks defined to prevent globally defined themes to interfere with the rendering.`only` keyword is already used when including a Twig template to transfer only the variables which are explicitly defined in the `include` tag, so it seemed natural to use it here too.This, of course, means that the user will need to manually `use` all of the templates that are required to render the form, including `form_div_layout.html.twig`This issue is described in details over at Symfony Demo repo:symfony/demo#515TODO:- [x] submit changes to the documentationCommits-------e0681f9 [Form] [TwigBridge] Added option to disable usage of default themes when rendering a form
emodric commentedOct 13, 2017
Thanks everyone for review :) |
…dric)This PR was merged into the 4.0-dev branch.Discussion----------[Form] Add $useDefaultThemes flag to the interfaces| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR | N/AFollowup to#22610 to add `$useDefaultThemes` to the interfaces as discussed in the issue.Commits-------c22d783 [Form] Add useDefaultThemes flag to the interfaces
… form themes (emodric, javiereguiluz)This PR was merged into the 3.4 branch.Discussion----------[Form] Document disabling the usage of globally defined form themesThis PR documents the option to disable the usage of globally defined form themes. This is suggested as a feature insymfony/symfony#22610 (reviewed, but not yet merged)Commits-------14ef7ae Minor reword8d45f83 Document disabling the usage of globally defined form themes
Uh oh!
There was an error while loading.Please reload this page.
This adds a possibility to use
onlykeyword inform_themetag to disable usage of globally defined form themes, e.g.{% form_theme form with ['common.html.twig', 'form/fields.html.twig'] only %}Otherwise, in order to completely control the rendering of the forms (for example in custom admin interfaces), one would need to use a form theme which has all the possible twig blocks defined to prevent globally defined themes to interfere with the rendering.
onlykeyword is already used when including a Twig template to transfer only the variables which are explicitly defined in theincludetag, so it seemed natural to use it here too.This, of course, means that the user will need to manually
useall of the templates that are required to render the form, includingform_div_layout.html.twigThis issue is described in details over at Symfony Demo repo:symfony/demo#515
TODO: