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

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

Merged
merged 1 commit into from Feb 6, 2018

Conversation

Projects
None yet
7 participants
@yceruto
Contributor

yceruto commented Jan 13, 2018

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:

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

This comment has been minimized.

Contributor

curry684 commented Jan 13, 2018

👍

Status: reviewed

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jan 16, 2018

BC break? Should be in a recipe instead?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jan 16, 2018

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

This comment has been minimized.

Contributor

curry684 commented Jan 16, 2018

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

This comment has been minimized.

Contributor

yceruto commented Jan 16, 2018

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

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jan 17, 2018

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

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

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

This comment has been minimized.

@yceruto

yceruto Jan 21, 2018

Contributor

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

This comment has been minimized.

@yceruto

yceruto Jan 21, 2018

Contributor

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

This comment has been minimized.

@xabbuh

xabbuh Jan 23, 2018

Member

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

This comment has been minimized.

@yceruto

yceruto Jan 24, 2018

Contributor

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?

This comment has been minimized.

@xabbuh

xabbuh Jan 24, 2018

Member

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)

This comment has been minimized.

@yceruto

yceruto Jan 24, 2018

Contributor

Inline comments added, thanks.

This comment has been minimized.

@fabpot

fabpot Jan 24, 2018

Member

We are now using is deprecated as of Symfony 4.1

@yceruto

This comment has been minimized.

Contributor

yceruto commented Jan 24, 2018

we just need to deprecate not setting the option.

Well, first step completed!

Status: Needs Review

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

This comment has been minimized.

@fabpot

fabpot Jan 24, 2018

Member

We are now using is deprecated as of Symfony 4.1

@yceruto

This comment has been minimized.

Contributor

yceruto commented Jan 24, 2018

@fabpot done.

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 24, 2018

Member

Not setting (missing "t")
this also misses some context IMHO.
Not setting the ... Twig configuration option ... ?

This comment has been minimized.

@yceruto

yceruto Jan 24, 2018

Contributor

Done, thanks!

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

This comment has been minimized.

@xabbuh

xabbuh Jan 25, 2018

Member

IIRC we use to include the full path of the config option like this:

Not setting the "twig.strict_variables" option to false [...]

This comment has been minimized.

@xabbuh

xabbuh Jan 25, 2018

Member

By the way, mentioning false here is IMO wrong. What is deprecated is not setting the option at all. So it should be:

Not setting the "twig.strict_variables" option is deprecated [...]

This comment has been minimized.

@yceruto

yceruto Jan 25, 2018

Contributor

What about:

Not setting the "strict_variables" option to "false" under "twig" configuration is deprecated as of Symfony 4.1 ...

better?

By the way, mentioning false here is IMO wrong. What is deprecated is not setting the option at all.

Yep, it could be a bit confusing, but IIUC not setting the option at all still will be possible in 5.0. We're deprecating the default value/behaviour caused by "not setting the option explicitly" at the same time we're saying to the people (the "false" part) what they need to do to keep the current behaviour in 5.0 and remove the deprecation notice during 4.1+

This comment has been minimized.

@xabbuh

xabbuh Jan 25, 2018

Member

My concern with the current wording is that it seems that you cannot set the option to true anymore.

This comment has been minimized.

@yceruto

yceruto Jan 25, 2018

Contributor

Yep, I see, so we need to reformulate the phrase to be clearer... 🤔

This comment has been minimized.

@yceruto

yceruto Jan 25, 2018

Contributor

Now changing the point of view:

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.

Wdyt? other suggestions?

@yceruto

This comment has been minimized.

Contributor

yceruto commented Jan 25, 2018

Modified deprecation message.

Status: Needs Review

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

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.

This comment has been minimized.

@Tobion

Tobion Feb 4, 2018

Member

Agree with nicolas

This comment has been minimized.

@yceruto

yceruto Feb 4, 2018

Contributor

Much better, done, thanks!

@Tobion

Tobion approved these changes Feb 5, 2018

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

This comment has been minimized.

@xabbuh

xabbuh Feb 5, 2018

Member

The indentation of this line needs to be fixed.

This comment has been minimized.

@yceruto

yceruto Feb 5, 2018

Contributor

Fixed, thanks.

@xabbuh

xabbuh approved these changes Feb 5, 2018

@xabbuh

This comment has been minimized.

Member

xabbuh commented Feb 5, 2018

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

@yceruto

This comment has been minimized.

Contributor

yceruto commented Feb 5, 2018

Should be ready now.

@fabpot

fabpot approved these changes Feb 6, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 6, 2018

Thank you @yceruto.

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

3 checks passed

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

fabpot added a commit that referenced this pull request Feb 6, 2018

feature #25780 [TwigBundle] Deprecating "false" in favor of "kernel.d…
…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

@yceruto yceruto deleted the yceruto:default_strict_variables branch Feb 6, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

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