Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Merged
fabpot merged 1 commit intosymfony:masterfromyceruto:default_strict_variables
Feb 6, 2018

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedJan 13, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

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

Nope, really it'sfalse by default:

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

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

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

@curry684
Copy link
Contributor

👍

Status: reviewed

@nicolas-grekas
Copy link
Member

BC break? Should be in a recipe instead?

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJan 16, 2018•
edited
Loading

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
Copy link
Contributor

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 reacted with thumbs up emoji

@yceruto
Copy link
MemberAuthor

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

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJan 17, 2018•
edited
Loading

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

@ycerutoycerutoforce-pushed thedefault_strict_variables branch from4bdf431 to85b6115CompareJanuary 21, 2018 23:29
@ycerutoyceruto 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
->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);
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
MemberAuthor

@ycerutoycerutoJan 21, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
MemberAuthor

@ycerutoycerutoJan 24, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Inline comments added, thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We are now usingis deprecated as of Symfony 4.1

@ycerutoycerutoforce-pushed thedefault_strict_variables branch 6 times, most recently from34b158c to9cd4637CompareJanuary 24, 2018 16:19
@yceruto
Copy link
MemberAuthor

we just need to deprecate not setting the option.

Well, first step completed!

Status: Needs Review

@yceruto
Copy link
MemberAuthor

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);
Copy link
Member

@nicolas-grekasnicolas-grekasFeb 4, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Agree with nicolas

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Much better, done, thanks!

@ycerutoycerutoforce-pushed thedefault_strict_variables branch from1141a8a to4f74801CompareFebruary 4, 2018 20:33
'namespaced_path3' =>'namespace3',
),
'paths' =>array(
'namespaced_path3' =>'namespace3',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The indentation of this line needs to be fixed.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fixed, thanks.

@ycerutoycerutoforce-pushed thedefault_strict_variables branch from4f74801 to19f30e2CompareFebruary 5, 2018 13:27
@xabbuh
Copy link
Member

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

@ycerutoycerutoforce-pushed thedefault_strict_variables branch 2 times, most recently from4dd428e to9572ebdCompareFebruary 5, 2018 14:51
@ycerutoycerutoforce-pushed thedefault_strict_variables branch 2 times, most recently from65db960 to922878eCompareFebruary 5, 2018 14:55
@yceruto
Copy link
MemberAuthor

Should be ready now.

@fabpot
Copy link
Member

Thank you@yceruto.

@fabpotfabpot merged commit922878e intosymfony:masterFeb 6, 2018
fabpot added a commit that referenced this pull requestFeb 6, 2018
…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#L130Fixing it insymfony/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:```yamltwig:    # ...    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
@ycerutoyceruto deleted the default_strict_variables branchFebruary 6, 2018 12:13
@fabpotfabpot mentioned this pull requestMay 7, 2018
symfony-splitter pushed a commit to symfony/twig-bundle that referenced this pull requestMay 29, 2019
…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.1symfony/symfony#25780Commits-------83207370cb Remove default value deprecation for twig.strict_variables
fabpot added a commit that referenced this pull requestMay 29, 2019
…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#25780Commits-------8320737 Remove default value deprecation for twig.strict_variables
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@fabpotfabpotfabpot approved these changes

@xabbuhxabbuhxabbuh approved these changes

+1 more reviewer

@TobionTobionTobion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

7 participants

@yceruto@curry684@nicolas-grekas@xabbuh@fabpot@Tobion@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp