Do not normalize array keys in twig globals #26770
Conversation
| @@ -76,6 +76,7 @@ private function addGlobalsSection(ArrayNodeDefinition $rootNode) | |||
| ->useAttributeAsKey('key') | |||
| ->example(array('foo' => '"@bar"', 'pi' => 3.14)) | |||
| ->prototype('array') | |||
| ->normalizeKeys(false) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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) |
|
This PR breaks BC for no benefit IMO, as leaving dashes untouched will not help at all (they are forbidden) |
|
@stof : The issue here actually isn't that the global var name is transformed (it's already configured with twig:
globals:
some-global: { 'some-key': 'some-value' }will expose: "some-global" => array: [
"some_key" => "some-value"
]hence, a global variable named (using |
|
@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. |
|
I haven't checked, but this should be on 2.7 or master. |
|
@fabpot rebased against 2.7 |
|
should be on master to me as well, consistent with the decision made in #21718 |
|
Lets' go for master on my side also. |
|
hmm, the key normalization is indeed done before the |
| public function testArrayKeysInGlobalsAreNotNormalized() | ||
| { | ||
| $input = array( | ||
| 'globals' => array('some-global' => array('some-key' => 'some-value')), |
There was a problem hiding this comment.
Can you avoid using a dash in the global name itself, to make the test less confusing ?
There was a problem hiding this comment.
@stof I consciously did that to ensure that the normalization is also not happening for the global names themselves
There was a problem hiding this comment.
I would split that into two tests and use a meaningful test name then to make the intention more clear.
|
@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. |
|
@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. |
|
Split the test and rebased against master. Ready to be merged from my POV |
|
Thank you @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
We should leave array keys in twig globals untouched.