[TwigBundle] Deprecating "false" in favor of "kernel.debug" as default value of "strict_variable" #25780

yceruto · 2018-01-13T00:48:18Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

http://symfony.com/doc/current/reference/configuration/twig.html#strict-variables
strict_variables
type: boolean default: '%kernel.debug%'

Nope, really it's false by default:

symfony/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php

Line 130 in 1df45e4

->booleanNode('strict_variables')->end()

Fixing it in symfony/symfony-docs#9050, but yes '%kernel.debug%' is a better default value, the TwigBundle recipe affirms that:

twig:
    # ...
    strict_variables: '%kernel.debug%'

So yeah, it definitely looks like it should be the default value, wdyt?

curry684 · 2018-01-13T02:25:23Z

👍

Status: reviewed

nicolas-grekas · 2018-01-16T21:41:50Z

BC break? Should be in a recipe instead?

nicolas-grekas · 2018-01-16T21:43:19Z

OH, it's already in a recipe, and the doc was accurate when starting with the standard edition, isn't it?
Because of the BC break cannot happen as is IMHO.

curry684 · 2018-01-16T22:39:28Z

Standard Edition has always explicitly defined it, see 2.7 @ https://github.com/symfony/symfony-standard/blob/2.7/app/config/config.yml#L36. So given that all "supported" standard editions and the Flex recipe default it correctly we're only "BC breaking" people that explicitly removed it to fall back to a value that was documented to be identical to the default. Even if somebody actually did that (why?!?) they were expecting it to do this as documented (%kernel.debug%), so absolute worst possible BC break that could happen is that we're fixing their dev environment to be a bit stricter.

Imho that's a very hard case to call a "BC break".

yceruto · 2018-01-16T23:07:29Z

Agree with @curry684, this BC break is very weird, however we can do something to solve it, right?

nicolas-grekas · 2018-01-17T05:31:02Z

Yes we can: we just need to deprecate not setting the option.

yceruto · 2018-01-21T23:35:32Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php

-                ->booleanNode('strict_variables')->end()
+                ->booleanNode('strict_variables')
+                    ->defaultValue(function () {
+                        @trigger_error('No setting "strict_variables" to false is deprecated as of 4.1, it will be the value of the "kernel.debug" parameter by default in 5.0.', E_USER_DEPRECATED);


@nicolas-grekas done, but I'm not completely sure if the numbers are correct: 4.1 ... 5.0 ?

Btw, is it the right way to do it? How can I avoid the deprecation notices in tests?

You would need to configure a value explicitly instead of relying on the default value.

Thanks @xabbuh, I've made the changes, but in some places it seems out of place, so we need to remember remove it relying on the default value again in 5.0. The question is: how to remember that? i.e. what is the common practice in this case? adding a direct comment to the code?

in the past, we often added inline comments like "to be removed when ..." (at least, I used this phrase a lot to search for code that could be removed when we started the development on 4.0)

Inline comments added, thanks.

We are now using is deprecated as of Symfony 4.1

yceruto · 2018-01-24T16:29:55Z

we just need to deprecate not setting the option.

Well, first step completed!

Status: Needs Review

yceruto · 2018-01-25T15:55:06Z

Modified deprecation message.

Status: Needs Review

nicolas-grekas · 2018-02-04T10:52:23Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php

-                ->booleanNode('strict_variables')->end()
+                ->booleanNode('strict_variables')
+                    ->defaultValue(function () {
+                        @trigger_error('Setting by default the "strict_variables" option to "false" under "twig" configuration is deprecated as of Symfony 4.1, it will be by default the value of the "kernel.debug" parameter in 5.0.', E_USER_DEPRECATED);


What about:
Relying on the default value ("false") of the "twig.strict_variables" configuration option is deprecated since Symfony 4.1. You should use "%kernel.debug%" explicitly instead, which will be the new default in 5.0.

Agree with nicolas

Much better, done, thanks!

xabbuh · 2018-02-05T05:50:36Z

src/Symfony/Bundle/TwigBundle/Tests/DependencyInjection/Fixtures/php/extra.php

-         'namespaced_path3' => 'namespace3',
-      ),
+    'paths' => array(
+     'namespaced_path3' => 'namespace3',


The indentation of this line needs to be fixed.

Fixed, thanks.

xabbuh · 2018-02-05T14:07:30Z

@yceruto Can you please also update the UPGRADE-4.1.md and UPGRADE-5.0.md files as well as the changelog file of TwigBundle?

…configuration

yceruto · 2018-02-05T14:57:48Z

Should be ready now.

fabpot · 2018-02-06T06:27:16Z

Thank you @yceruto.

…ebug" as default value of "strict_variable" (yceruto) This PR was merged into the 4.1-dev branch. Discussion ---------- [TwigBundle] Deprecating "false" in favor of "kernel.debug" as default value of "strict_variable" | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - > http://symfony.com/doc/current/reference/configuration/twig.html#strict-variables >**strict_variables** > **type**: boolean **default**: `'%kernel.debug%'` Nope, really it's `false` by default: https://github.com/symfony/symfony/blob/1df45e43563a37633b50d4a36478090361a0b9de/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php#L130 Fixing it in symfony/symfony-docs#9050, but yes `'%kernel.debug%'` is a better default value, the [TwigBundle recipe](https://github.com/symfony/recipes/blob/bf2148f9f1fe5af7e19c3145b6f7246c6cabb3a5/symfony/twig-bundle/3.3/config/packages/twig.yaml#L4:) affirms that: ```yaml twig: # ... strict_variables: '%kernel.debug%' ``` So yeah, it definitely looks like it should be the default value, wdyt? Commits ------- 922878e Deprecating "false" as default value of "strict_variable" under Twig configuration

…trict_variables option (yceruto) This PR was merged into the 5.0-dev branch. Discussion ---------- [TwigBundle] Remove default value deprecation for twig.strict_variables option | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - See previous PR in 4.1 symfony/symfony#25780 Commits ------- 83207370cb Remove default value deprecation for twig.strict_variables

…trict_variables option (yceruto) This PR was merged into the 5.0-dev branch. Discussion ---------- [TwigBundle] Remove default value deprecation for twig.strict_variables option | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - See previous PR in 4.1 #25780 Commits ------- 8320737 Remove default value deprecation for twig.strict_variables

carsonbot added Status: Needs Review TwigBundle Feature labels Jan 13, 2018

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 13, 2018

yceruto mentioned this pull request Jan 13, 2018

[TwigBundle] Set the parameter kernel.debug as default value of strict_variables option symfony/symfony-docs#9049

Closed

xabbuh added this to the 4.1 milestone Jan 15, 2018

nicolas-grekas added the BC Break label Jan 16, 2018

nicolas-grekas added Status: Needs Work Deprecation and removed Status: Reviewed BC Break Feature labels Jan 18, 2018

yceruto force-pushed the default_strict_variables branch from 4bdf431 to 85b6115 Compare January 21, 2018 23:29

yceruto changed the title ~~[TwigBundle] Set the parameter kernel.debug as default value of strict_variables option~~ [TwigBundle] Deprecating "false" in favor of "kernel.debug" as default value of "strict_variable" Jan 21, 2018

yceruto commented Jan 21, 2018

View reviewed changes

yceruto force-pushed the default_strict_variables branch 6 times, most recently from 34b158c to 9cd4637 Compare January 24, 2018 16:19

carsonbot removed the Status: Needs Work label Jan 24, 2018

carsonbot added the Status: Needs Work label Jan 25, 2018

yceruto force-pushed the default_strict_variables branch from 5509b63 to 1141a8a Compare January 25, 2018 15:46

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 25, 2018

nicolas-grekas reviewed Feb 4, 2018

View reviewed changes

yceruto force-pushed the default_strict_variables branch from 1141a8a to 4f74801 Compare February 4, 2018 20:33

Tobion approved these changes Feb 5, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 5, 2018

xabbuh reviewed Feb 5, 2018

View reviewed changes

yceruto force-pushed the default_strict_variables branch from 4f74801 to 19f30e2 Compare February 5, 2018 13:27

xabbuh approved these changes Feb 5, 2018

View reviewed changes

yceruto force-pushed the default_strict_variables branch 2 times, most recently from 4dd428e to 9572ebd Compare February 5, 2018 14:51

Deprecating "false" as default value of "strict_variable" under Twig …

922878e

…configuration

yceruto force-pushed the default_strict_variables branch 2 times, most recently from 65db960 to 922878e Compare February 5, 2018 14:55

fabpot approved these changes Feb 6, 2018

View reviewed changes

fabpot merged commit 922878e into symfony:master Feb 6, 2018

yceruto deleted the default_strict_variables branch February 6, 2018 12:13

javiereguiluz mentioned this pull request Feb 18, 2018

[TwigBundle] Deprecating "false" in favor of "kernel.debug" as default value of "strict_variable" symfony/symfony-docs#9291

Closed

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

yceruto mentioned this pull request May 28, 2019

[TwigBundle] Remove default value deprecation for twig.strict_variables option #31664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TwigBundle] Deprecating "false" in favor of "kernel.debug" as default value of "strict_variable" #25780

[TwigBundle] Deprecating "false" in favor of "kernel.debug" as default value of "strict_variable" #25780

yceruto commented Jan 13, 2018 •

edited

Loading

curry684 commented Jan 13, 2018

nicolas-grekas commented Jan 16, 2018

nicolas-grekas commented Jan 16, 2018 •

edited

Loading

curry684 commented Jan 16, 2018

yceruto commented Jan 16, 2018

nicolas-grekas commented Jan 17, 2018 •

edited

Loading

yceruto Jan 21, 2018

yceruto Jan 21, 2018 •

edited

Loading

xabbuh Jan 23, 2018

yceruto Jan 24, 2018 •

edited

Loading

xabbuh Jan 24, 2018

yceruto Jan 24, 2018

fabpot Jan 24, 2018

yceruto commented Jan 24, 2018

yceruto commented Jan 25, 2018

nicolas-grekas Feb 4, 2018 •

edited

Loading

Tobion Feb 4, 2018

yceruto Feb 4, 2018

xabbuh Feb 5, 2018

yceruto Feb 5, 2018

xabbuh commented Feb 5, 2018

yceruto commented Feb 5, 2018

fabpot commented Feb 6, 2018

[TwigBundle] Deprecating "false" in favor of "kernel.debug" as default value of "strict_variable" #25780

[TwigBundle] Deprecating "false" in favor of "kernel.debug" as default value of "strict_variable" #25780

Conversation

yceruto commented Jan 13, 2018 • edited Loading

curry684 commented Jan 13, 2018

nicolas-grekas commented Jan 16, 2018

nicolas-grekas commented Jan 16, 2018 • edited Loading

curry684 commented Jan 16, 2018

yceruto commented Jan 16, 2018

nicolas-grekas commented Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

yceruto Jan 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto commented Jan 24, 2018

yceruto commented Jan 25, 2018

nicolas-grekas Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Feb 5, 2018

yceruto commented Feb 5, 2018

fabpot commented Feb 6, 2018

yceruto commented Jan 13, 2018 •

edited

Loading

nicolas-grekas commented Jan 16, 2018 •

edited

Loading

nicolas-grekas commented Jan 17, 2018 •

edited

Loading

yceruto Jan 21, 2018 •

edited

Loading

yceruto Jan 24, 2018 •

edited

Loading

nicolas-grekas Feb 4, 2018 •

edited

Loading