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

Do not normalize array keys in twig globals #26770

Merged
merged 1 commit into from Apr 20, 2018

Conversation

@lstrojny
Contributor

lstrojny commented Apr 3, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? rather no
Tests pass? yes
Fixed tickets n.A.
License MIT
Doc PR n.A.

We should leave array keys in twig globals untouched.

@lstrojny lstrojny changed the base branch from master to 3.4 Apr 3, 2018

@@ -76,6 +76,7 @@ private function addGlobalsSection(ArrayNodeDefinition $rootNode)
->useAttributeAsKey('key')
->example(array('foo' => '"@bar"', 'pi' => 3.14))
->prototype('array')
->normalizeKeys(false)

This comment has been minimized.

@fabpot

fabpot Apr 3, 2018

Member

Don't we have the same issue in 2.7 as well? I'm wondering if it would not be "safer" to do this in master instead.

This comment has been minimized.

@ogizanagi

ogizanagi Apr 3, 2018

Member

We merged a similar fix as an assumed BC break on master only. This one may have worse impacts. I don't think it is worth breaking existing apps relying on this behavior, so I'd do it on master only (and mention it in the UPGRADE-4.1.md file).

This comment has been minimized.

@lyrixx

lyrixx Apr 3, 2018

Member

For the record, a similar PR has been rejected on the workflow component to keep the BC. But in the workflow there was an alternative way to configure things.

This comment has been minimized.

@lstrojny

lstrojny Apr 3, 2018

Contributor

Sure, one person’s bugfix is another person's BC break. Relying (and keeping) a behavior where the keys of an array magically change doesn't sound like a good idea to me though.

@stof

This comment has been minimized.

Member

stof commented Apr 3, 2018

this is indeed a BC break technically, in case some people use dashes in their global names (although that would be weird, as they are not allowed in Twig identifiers)

@stof

This comment has been minimized.

Member

stof commented Apr 3, 2018

This PR breaks BC for no benefit IMO, as leaving dashes untouched will not help at all (they are forbidden)

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 3, 2018

@stof : The issue here actually isn't that the global var name is transformed (it's already configured with ->normalizeKeys(false)), but that an array registered as a global will see its keys transformed while those are perfectly fine. I.e:

twig:
    globals:
        some-global: { 'some-key': 'some-value' }

will expose:

"some-global" => array: [
    "some_key" => "some-value"
]

hence, a global variable named some-global (you'll get some troubles accessing it indeed, but still doable using _context['some-global'] despite clearly not recommended) but with transformed array keys. Dashes in array keys aren't forbidden.

(using some-global as name for the global var in the test case is misleading regarding this)

@lstrojny

This comment has been minimized.

Contributor

lstrojny commented Apr 3, 2018

@stof what @ogizanagi is saying

@ogizanagi choice of name for the global itself is there to verify that the current behavior (which enables dashes in global names already) stays as is.

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 4, 2018

I haven't checked, but this should be on 2.7 or master.

@lstrojny lstrojny changed the base branch from 3.4 to 2.7 Apr 4, 2018

@lstrojny

This comment has been minimized.

Contributor

lstrojny commented Apr 4, 2018

@fabpot rebased against 2.7

@chalasr

This comment has been minimized.

Member

chalasr commented Apr 4, 2018

should be on master to me as well, consistent with the decision made in #21718

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 4, 2018

Lets' go for master on my side also.
@lstrojny while rebasing, could you please add a note in the UPGRADING file about it?

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Apr 4, 2018

@stof

This comment has been minimized.

Member

stof commented Apr 4, 2018

hmm, the key normalization is indeed done before the beforeNormalization callbacks, so the value is not yet in the value key of the array node (and so gets normalized)

public function testArrayKeysInGlobalsAreNotNormalized()
{
$input = array(
'globals' => array('some-global' => array('some-key' => 'some-value')),

This comment has been minimized.

@stof

stof Apr 4, 2018

Member

Can you avoid using a dash in the global name itself, to make the test less confusing ?

This comment has been minimized.

@lstrojny

lstrojny Apr 4, 2018

Contributor

@stof I consciously did that to ensure that the normalization is also not happening for the global names themselves

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2018

Member

I would split that into two tests and use a meaningful test name then to make the intention more clear.

This comment has been minimized.

@lstrojny

lstrojny Apr 19, 2018

Contributor

Split the test

@lstrojny

This comment has been minimized.

Contributor

lstrojny commented Apr 4, 2018

@nicolas-grekas @chalasr I disagree it should go to master only, as it’s clearly a bugfix. The referenced decision in #21718 looks quite different for me. While it’s technically the same issue ("normalization where normalization shouldn't happen") the context matters a lot and the context is very different. Prioritizing BC at all costs in the context of authentications makes a lot of sense to me and I feel it was the right call. In the context of template globals though, correctness should be prioritized over complete backwards compatibility.

Adding it to the UPGRADING file makes a lot of sense.

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 5, 2018

@lstrojny The problem is that we cannot afford to break some code out there in a patch release. Unfortunately, we've had several such cases in the past (and quite recently with the latest releases). The story is always the same: we find something that we want to fix in 2.7, which is a bug that also changes the current behavior in subtle ways, but we think that it's ok as nobody can rely on this bad behavior, and of course, some people are relying on the buggy behavior, they are upset, we are reverting, and I need to do another quick release (2.7, 2.8, 3.4, and 4.0 right now). Lot of time and frustration. So, I would advise to target master on this one.

@lstrojny lstrojny changed the base branch from 2.7 to master Apr 19, 2018

@lstrojny

This comment has been minimized.

Contributor

lstrojny commented Apr 19, 2018

Split the test and rebased against master. Ready to be merged from my POV

@fabpot

fabpot approved these changes Apr 20, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 20, 2018

Thank you @lstrojny.

@fabpot fabpot merged commit 8c16727 into symfony:master Apr 20, 2018

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 Apr 20, 2018

feature #26770 Do not normalize array keys in twig globals (lstrojny)
This PR was merged into the 4.1-dev branch.

Discussion
----------

Do not normalize array keys in twig globals

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | rather no
| Tests pass?   | yes
| Fixed tickets | n.A.
| License       | MIT
| Doc PR        | n.A.

We should leave array keys in twig globals untouched.

Commits
-------

8c16727 Don’t normalize global values

@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