[DI] Remove case insensitive parameter names #24052
Conversation
| @@ -1643,7 +1613,7 @@ private function getServiceCall($id, Reference $reference = null) | |||
| if ('service_container' === $id) { | |||
| return '$this'; | |||
| } | |||
|
|
|||
| @@ -68,7 +66,7 @@ public function all() | |||
| */ | |||
| public function get($name) | |||
| { | |||
| $name = $this->normalizeName($name); | |||
| $name = (string) $name; | |||
There was a problem hiding this comment.
Previously implied by strtolower. And we actually rely on that, tests fail otherwise.
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L1558 leads to has/getParameter($paramInstance) call.
There was a problem hiding this comment.
then the dumper should be updated on the lowest branch for making it cast to string beforehand IMHO
There was a problem hiding this comment.
why? we don't use strict types (fortunately), so casting is just fine IMHO - and is functionally similar to adding the type hint on the signature (if we could)
There was a problem hiding this comment.
It's a matter of contract. If @param string means, we also support objects that can be cast to string, then we need to add the string cast everywhere in symfony. So I agree with @chalasr that this is a bug of the dumper.
There was a problem hiding this comment.
I'm just saying string and castable object instance have not been treated the same everywhere.
The thing is we used strtolower before, which happily casts. Thus making it a supported feature, probably unintended but at this point we have to consider it.
Therefor i dont think it's part of this PR, which is now merged anyway. But maybe for the sake of consistency, we could cast beforehand, where needed. And deprecate passing stringish values or just ignore the fact it was possible before, regarding end users.
There was a problem hiding this comment.
In the past we usually rejected explicit type casts and let it be the user's responsibility to respect our API.
There was a problem hiding this comment.
And in the past also we had to revert as bugfix because we broke things with this policy...
There was a problem hiding this comment.
But we break BC in this case :) Technically at least, passing a stringish object actually worked as expected before. Core uses it :)
So yes there's an inconsistency between $a[strtolower($k)] and $a[$k] by design. I believe @nicolas-grekas comment "This is PHP" reflects that.
Now if everywhere we'd do fn(string $k) the inconsistency is gone, while preserving the stringish behavior. Id prefer doing that instead of removing it here first.
edit: missed comment in between, as usual :)
There was a problem hiding this comment.
Lets just keep it. This will always be a case-by-case decision until we have type declarations.
|
Thank you @ro0NL. |
This PR was merged into the 4.0-dev branch. Discussion ---------- [DI] Remove case insensitive parameter names | Q | A | ------------- | --- | Branch? | master | 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--> First round, lets see :) Commits ------- ed3df04 [DI] Remove case insensitive parameter names
|
Yes, thank you! 👏 I think this resolves my issue #23381 |
First round, lets see :)