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] Added option to disable usage of default themes when rendering a form #22610

Merged
merged 1 commit into from Oct 13, 2017

Conversation

@emodric
Contributor

emodric commented May 2, 2017

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#8495

This 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#515

TODO:

  • submit changes to the documentation
@emodric

This comment has been minimized.

Show comment
Hide comment
@emodric

emodric Sep 25, 2017

Contributor

Hi, what is the status PR? Do you think it could end up in 3.4?

Contributor

emodric commented Sep 25, 2017

Hi, what is the status PR? Do you think it could end up in 3.4?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 1, 2017

Member

@emodric I like this feature, can you implement the suggestions made by @stof?

Member

fabpot commented Oct 1, 2017

@emodric I like this feature, can you implement the suggestions made by @stof?

@emodric

This comment has been minimized.

Show comment
Hide comment
@emodric

emodric Oct 1, 2017

Contributor

@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?

Contributor

emodric commented Oct 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

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 1, 2017

Member

You need to remove the argument and use get_func_args() to determine how it was called to handle the new arg and trigger a dep notice if needed. You can find quite a few examples in the code base doing exactly this.

Member

fabpot commented Oct 1, 2017

You need to remove the argument and use get_func_args() to determine how it was called to handle the new arg and trigger a dep notice if needed. You can find quite a few examples in the code base doing exactly this.

@emodric

This comment has been minimized.

Show comment
Hide comment
@emodric

emodric Oct 1, 2017

Contributor

Ah okay, I figured you wanted the feature to go in only in 4.0. I'll do it first thing tomorrow morning.

Contributor

emodric commented Oct 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

This comment has been minimized.

Show comment
Hide comment
@emodric

emodric Oct 2, 2017

Contributor

@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.

Contributor

emodric commented Oct 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

(failure unrelated)

@xabbuh

xabbuh approved these changes Oct 8, 2017

*/
public function setTheme(FormView $view, $themes);
public function setTheme(FormView $view, $themes /*, $useDefaultThemes = true */);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 13, 2017

Member

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).

@nicolas-grekas

nicolas-grekas Oct 13, 2017

Member

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).

This comment has been minimized.

@emodric

emodric Oct 13, 2017

Contributor

@nicolas-grekas As per @fabpot, deprecation notice is not needed if the argument is optional?

@emodric

emodric Oct 13, 2017

Contributor

@nicolas-grekas As per @fabpot, deprecation notice is not needed if the argument is optional?

This comment has been minimized.

@fabpot

fabpot Oct 13, 2017

Member

I think the current code is good as is :)

@fabpot

fabpot Oct 13, 2017

Member

I think the current code is good as is :)

@fabpot

fabpot approved these changes Oct 13, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 13, 2017

Member

Thank you @emodric.

Member

fabpot commented Oct 13, 2017

Thank you @emodric.

@fabpot fabpot merged commit e0681f9 into symfony:3.4 Oct 13, 2017

1 of 3 checks passed

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

fabpot added a commit that referenced this pull request Oct 13, 2017

feature #22610 [Form] [TwigBridge] Added option to disable usage of d…
…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#8495

This 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#515

TODO:

- [x] submit changes to the documentation

Commits
-------

e0681f9 [Form] [TwigBridge] Added option to disable usage of default themes when rendering a form
@emodric

This comment has been minimized.

Show comment
Hide comment
@emodric

emodric Oct 13, 2017

Contributor

Thanks everyone for review :)

Contributor

emodric commented Oct 13, 2017

Thanks everyone for review :)

This was referenced Oct 18, 2017

fabpot added a commit that referenced this pull request Oct 19, 2017

minor #24628 [Form] Add $useDefaultThemes flag to the interfaces (emo…
…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/A

Followup to #22610 to add `$useDefaultThemes` to the interfaces as discussed in the issue.

Commits
-------

c22d783 [Form] Add useDefaultThemes flag to the interfaces

weaverryan added a commit to symfony/symfony-docs that referenced this pull request Nov 10, 2017

feature #8495 [Form] Document disabling the usage of globally defined…
… form themes (emodric, javiereguiluz)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] Document disabling the usage of globally defined form themes

This PR documents the option to disable the usage of globally defined form themes. This is suggested as a feature in symfony/symfony#22610 (reviewed, but not yet merged)

Commits
-------

14ef7ae Minor reword
8d45f83 Document disabling the usage of globally defined form themes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment