[DI] Case sensitive parameter names #23874
Conversation
|
Looks good so far 👍 thanks for taking care of this. |
| */ | ||
| public static function normalizeName($name) | ||
| { | ||
| if (0 === strpos($name, 'env(') && ')' === substr($name, -1) && 'env()' !== $name) { |
There was a problem hiding this comment.
@nicolas-grekas can you validate this approach? We dont want a deprecation for env(NAME) i guess =/
|
|
||
| $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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Yeah.. im already working on the same approach as done for service id's 👍
There was a problem hiding this comment.
What do we expect fromall().. should the keys stay lowercase?
edi: this is a bit more work :) i found out.
There was a problem hiding this comment.
@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.
|
I think this will do 👍 |
| return $normalizedName; | ||
| EOF; | ||
| } else { | ||
| $normalizedParams = 'return strtolower($name);'; |
There was a problem hiding this comment.
in this case, no deprecation is triggered, isnt it?
There was a problem hiding this comment.
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)
|
|
||
| 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)); |
There was a problem hiding this comment.
not sure using str_repeat is better than putting spaces in the sprintf (same below)
|
|
||
| EOF; | ||
|
|
||
| $code .= ' private $normalizedParameterNames = '.($normalizedParams ? sprintf("array(\n%s\n%s);", implode("\n", $normalizedParams), str_repeat(' ', 4)) : 'array();')."\n"; |
There was a problem hiding this comment.
str_repeat? (etc if more :) )
|
|
||
| private function normalizeParameterName($name) | ||
| { | ||
| if (!is_string($name)) { |
There was a problem hiding this comment.
the cast should be unconditional, the engine already takes care of this "if"
| private function normalizeParameterName($name) | ||
| { | ||
| if (!is_string($name)) { | ||
| $name = (string) $name; |
| /** | ||
| * @internal to be removed in 4.0 | ||
| */ | ||
| public function normalizeName($name) |
|
@nicolas-grekas that means we keep 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. |
|
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) |
| 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; |
There was a problem hiding this comment.
that is not the normalized (lowercased) parameter name, but the original name. using the same variable for both things is confusing
There was a problem hiding this comment.
Or it might be easier to rename the variable $normalizedName = strtolower($name) to $lowercasedName to remove the duplicate usage
There was a problem hiding this comment.
not sure we care.. it's optimized code. I believe this will be a hot code path.
|
Status: reviewed |
|
Thank you @ro0NL. |
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#... <!--highly recommended for new features--> @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
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 | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> Forgotten in #23874 :) PS: im working on a PR for 4.0 as well Commits ------- 3890996 [DI] Add upgrade note about case insenstive params
@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