[DI] Case sensitive parameter names #23874

ro0NL · 2017-08-12T09:26:46Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#23809
License	MIT
Doc PR	symfony/symfony-docs#...

@GuilhemN took your patch.. but i use the same deprecation messages as for case sensitive service id's, i found it more clear. Also comparing to $origName to keep the diff smaller

GuilhemN · 2017-08-12T09:29:28Z

Looks good so far 👍 thanks for taking care of this.

ro0NL · 2017-08-12T10:53:43Z

src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php

+     */
+    public static function normalizeName($name)
+    {
+        if (0 === strpos($name, 'env(') && ')' === substr($name, -1) && 'env()' !== $name) {


@nicolas-grekas can you validate this approach? We dont want a deprecation for env(NAME) i guess =/

derrabus · 2017-08-12T13:57:36Z

src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php

+
+        $normalizedName = strtolower($name);
+        if ($normalizedName !== (string) $name) {
+            @trigger_error(sprintf('Parameter names will be made case sensitive in Symfony 4.0. Using "%s" instead of "%s" is deprecated since version 3.4.', $name, $normalizedName), E_USER_DEPRECATED);


So this basically means that I get a deprecation message whenever I use uppercase characters in parameter names? This is kind of misleading because uppercase letters won't be forbidden in Symfony 4, will they?

I mean, if I set a parameter like myCamelCaseParameter and always use myCamelCaseParameter to fetch it from the bag, I don't see a problem with that, although it might not be best practice to format parameter names that way.

The problematic workflow would be to set myCamelCaseParameter and request mycamelcaseparameter afterwards. Because, as soon as parameter names are treated case-sensitively, this piece of code is going to break. And imho only in this case, I should get a deprecation warning.

That's what was done for services name but the complexity to do it is not worth it for parameters imo. If you don't want to have these deprecations, you'll still be able to quickly upgrade to 4.0.

Saying "don't use uppercase letters" in order to allow them later looks weird to me as well. I would expect set() to not alter $name and get() to trigger if the lowercased name of an existing parameter equals $name (better put effort in complex BC layers than give complex upgrade paths to our users)

Deprecation warnings in Symfony are DX feature, used to point out code that is going to break on update. If we produce false positives, we make it harder for users to detect this kind of issues. It's hard to see, let's say, 2 real issues if they're hidden among 198 false positives.

@chalasr @derrabus that's fair points. I'll give it a try.

What if you keep a list of all parameters (that are not in lowercase), then when fetching a parameter (in normalized format) check of the casing differs with the original? if there is no difference you don't have to throw an deprecation warning.

In practice keeping a list of normalized names (like we do for private id's atm).

@ro0NL you can do it of course, that's your PR :)

I think we should keep $this->parameters as is and store original keys in a new property such as $this->rawKeys, the difficulty is to keep this property after compilation.

Yeah.. im already working on the same approach as done for service id's 👍

What do we expect fromall().. should the keys stay lowercase?

edi: this is a bit more work :) i found out.

@ro0NL having keys in their original case would make more sense with this change imo. That's a small bc break but I think it's acceptable.

ro0NL · 2017-08-13T11:24:23Z

I think this will do 👍

nicolas-grekas · 2017-08-13T14:40:09Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+        return $normalizedName;
+EOF;
+            } else {
+                $normalizedParams = 'return strtolower($name);';


in this case, no deprecation is triggered, isnt it?

nicolas-grekas

Almost good. There is one case that needs more care:
resolving env placeholders should be case insensitive, because that's purely internal and compile time, and that'll allow them to work through DI ext transformations (some do "strtolower" which would be fine already, but others might do "strtoupper", which should continue to work)

nicolas-grekas · 2017-08-14T14:11:21Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php


        foreach ($this->container->getParameterBag()->all() as $key => $value) {
            if ($key !== $resolvedKey = $this->container->resolveEnvPlaceholders($key)) {
                throw new InvalidArgumentException(sprintf('Parameter name cannot use env parameters: %s.', $resolvedKey));
            }
+            if ($key !== $lcKey = strtolower($key)) {
+                $normalizedParams[] = str_repeat(' ', 8).sprintf('%s => %s,', $this->export($lcKey), $this->export($key));


not sure using str_repeat is better than putting spaces in the sprintf (same below)

nicolas-grekas · 2017-08-14T14:11:43Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+
+EOF;
+
+            $code .= '    private $normalizedParameterNames = '.($normalizedParams ? sprintf("array(\n%s\n%s);", implode("\n", $normalizedParams), str_repeat(' ', 4)) : 'array();')."\n";


str_repeat? (etc if more :) )

nicolas-grekas · 2017-08-14T14:12:09Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+
+    private function normalizeParameterName($name)
+    {
+        if (!is_string($name)) {


the cast should be unconditional, the engine already takes care of this "if"

nicolas-grekas · 2017-08-14T14:17:45Z

src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php

+    private function normalizeParameterName($name)
+    {
+        if (!is_string($name)) {
+            $name = (string) $name;


should be unconditional

nicolas-grekas · 2017-08-14T14:18:25Z

src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php

+    /**
+     * @internal to be removed in 4.0
+     */
+    public function normalizeName($name)


why public?

ro0NL · 2017-08-14T15:41:26Z

@nicolas-grekas that means we keep normalizeParameterName in 4.0 right? Always checking for env() syntax...

The request contradicts with params being case sensitive though.. do we really care about "DI transformations"? Also in bash vars are case sensitive... given that you get exactly the right upgrade path currently.

nicolas-grekas · 2017-08-14T15:55:51Z

OK, reread again, works already (I was talking about env "placeholders", not env "references", if that distinction makes sense - see str_ireplace in ContainerBuilder::resolveEnvPlaceholders(), which is fine)

Tobion · 2017-08-15T23:20:31Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+    private function normalizeParameterName($name)
+    {
+        if (isset($this->normalizedParameterNames[$normalizedName = strtolower($name)]) || isset($this->parameters[$normalizedName]) || array_key_exists($normalizedName, $this->parameters)) {
+            $normalizedName = isset($this->normalizedParameterNames[$normalizedName]) ? $this->normalizedParameterNames[$normalizedName] : $normalizedName;


that is not the normalized (lowercased) parameter name, but the original name. using the same variable for both things is confusing

Or it might be easier to rename the variable $normalizedName = strtolower($name) to $lowercasedName to remove the duplicate usage

not sure we care.. it's optimized code. I believe this will be a hot code path.

nicolas-grekas · 2017-08-22T11:30:37Z

Status: reviewed

fabpot · 2017-08-22T22:03:17Z

Thank you @ro0NL.

@GuilhemN

This PR was merged into the 3.4 branch. Discussion ---------- [DI] Case sensitive parameter names | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #23809 | License | MIT | Doc PR | symfony/symfony-docs#...  @GuilhemN took your patch.. but i use the same deprecation messages as for case sensitive service id's, i found it more clear. Also comparing to $origName to keep the diff smaller Commits ------- 8a1d168 [DI] Case sensitive parameter names

TomasVotruba · 2017-08-28T08:37:40Z

Great job @ro0NL and @GuilhemN , thank you

This PR was merged into the 3.4 branch. Discussion ---------- [DI] Add upgrade note about case insenstive params | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  Forgotten in #23874 :) PS: im working on a PR for 4.0 as well Commits ------- 3890996 [DI] Add upgrade note about case insenstive params

carsonbot added Status: Needs Review DependencyInjection Deprecation labels Aug 12, 2017

ro0NL commented Aug 12, 2017

View reviewed changes

chalasr added Status: Needs Work Feature and removed Status: Needs Review labels Aug 12, 2017

chalasr added this to the 3.4 milestone Aug 12, 2017

derrabus reviewed Aug 12, 2017

View reviewed changes

chalasr added Status: Needs Review and removed Status: Needs Work labels Aug 12, 2017

nicolas-grekas reviewed Aug 13, 2017

View reviewed changes

nicolas-grekas reviewed Aug 14, 2017

View reviewed changes

[DI] Case sensitive parameter names

8a1d168

nicolas-grekas approved these changes Aug 14, 2017

View reviewed changes

Tobion reviewed Aug 15, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 22, 2017

fabpot approved these changes Aug 22, 2017

View reviewed changes

fabpot merged commit 8a1d168 into symfony:3.4 Aug 22, 2017

ro0NL deleted the di/params branch August 28, 2017 09:57

ro0NL mentioned this pull request Aug 31, 2017

[DI] Add upgrade note about case insenstive params #24050

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Case sensitive parameter names #23874

[DI] Case sensitive parameter names #23874

ro0NL commented Aug 12, 2017 •

edited

Loading

GuilhemN commented Aug 12, 2017

ro0NL Aug 12, 2017

derrabus Aug 12, 2017

GuilhemN Aug 12, 2017

chalasr Aug 12, 2017 •

edited

Loading

derrabus Aug 12, 2017

GuilhemN Aug 12, 2017

sstok Aug 12, 2017 •

edited

Loading

GuilhemN Aug 12, 2017

ro0NL Aug 12, 2017

ro0NL Aug 12, 2017 •

edited

Loading

GuilhemN Aug 12, 2017

ro0NL commented Aug 13, 2017

nicolas-grekas Aug 13, 2017

ro0NL Aug 13, 2017

nicolas-grekas left a comment •

edited

Loading

nicolas-grekas Aug 14, 2017

nicolas-grekas Aug 14, 2017

nicolas-grekas Aug 14, 2017

nicolas-grekas Aug 14, 2017

nicolas-grekas Aug 14, 2017

ro0NL commented Aug 14, 2017 •

edited

Loading

nicolas-grekas commented Aug 14, 2017

Tobion Aug 15, 2017

Tobion Aug 15, 2017

ro0NL Aug 16, 2017

nicolas-grekas commented Aug 22, 2017

fabpot commented Aug 22, 2017

TomasVotruba commented Aug 28, 2017


		EOF;

		$code .= ' private $normalizedParameterNames = '.($normalizedParams ? sprintf("array(\n%s\n%s);", implode("\n", $normalizedParams), str_repeat(' ', 4)) : 'array();')."\n";

[DI] Case sensitive parameter names #23874

[DI] Case sensitive parameter names #23874

Conversation

ro0NL commented Aug 12, 2017 • edited Loading

GuilhemN commented Aug 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Aug 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstok Aug 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Aug 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment • 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

Choose a reason for hiding this comment

ro0NL commented Aug 14, 2017 • edited Loading

nicolas-grekas commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 22, 2017

fabpot commented Aug 22, 2017

TomasVotruba commented Aug 28, 2017

ro0NL commented Aug 12, 2017 •

edited

Loading

chalasr Aug 12, 2017 •

edited

Loading

sstok Aug 12, 2017 •

edited

Loading

ro0NL Aug 12, 2017 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading

ro0NL commented Aug 14, 2017 •

edited

Loading