[HttpKernel] fix kernel.name deprecation #28888
Conversation
| @@ -367,7 +367,7 @@ protected function loadCacheDriver($cacheName, $objectManagerName, array $cacheD | |||
| } else { | |||
| $seed = '_'.$container->getParameter('kernel.project_dir'); | |||
| } | |||
| $seed .= '.'.$container->getParameter('kernel.container_class').'.'.$container->getParameter('kernel.environment').'.'.$container->getParameter('kernel.debug'); | |||
| $seed .= '.'.$container->getParameter('kernel.container_class'); | |||
There was a problem hiding this comment.
kernel.container_class already embeds the entropy from kernel.environment and kernel.debug
| @@ -276,6 +276,8 @@ public function locateResource($name, $dir = null, $first = true) | |||
|
|
|||
| /** | |||
| * {@inheritdoc} | |||
| * | |||
| * @deprecated since Symfony 4.2 | |||
There was a problem hiding this comment.
let's be even more explicit and duplicate this info from the interface
| return str_replace('\\', '_', \get_class($this)).ucfirst($this->environment).($this->debug ? 'Debug' : '').'Container'; | ||
| $name = \get_class($this); | ||
| if (false !== $i = strrpos($name, '\\')) { | ||
| $name = substr($name, 1 + $i); |
There was a problem hiding this comment.
keeping only the last part is enough IMHO
There was a problem hiding this comment.
I would say quite the contrary for multi-app projects. Let's stay on the safe side and keep the FQDN.
| $name = substr($name, 1 + $i); | ||
| } | ||
|
|
||
| return $this->name.$name.ucfirst($this->environment).($this->debug ? 'Debug' : '').'Container'; |
There was a problem hiding this comment.
adding $this->name back is what fixes deps=high tests - to be removed in 5.0 (alongside with the property)
| $name = substr($name, 1 + $i); | ||
| } | ||
|
|
||
| return $this->name.$name.ucfirst($this->environment).($this->debug ? 'Debug' : '').'Container'; |
There was a problem hiding this comment.
This change is wrong. We don't want to rely on the name anymore. Right now, name is kind of required only because of the way we tests things internally, but nobody will have the same issue IRL.
There was a problem hiding this comment.
what's the issue with keeping it in 4.x and removing in 5.0?
Patching tests on lower branches looks even more wrong to me
| @@ -440,7 +442,7 @@ protected function build(ContainerBuilder $container) | |||
| */ | |||
| protected function getContainerClass() | |||
| { | |||
| return str_replace('\\', '_', \get_class($this)).ucfirst($this->environment).($this->debug ? 'Debug' : '').'Container'; | |||
| return $this->name.str_replace('\\', '_', \get_class($this)).ucfirst($this->environment).($this->debug ? 'Debug' : '').'Container'; | |||
There was a problem hiding this comment.
Keeping the $this->name prefix is a requirement here. The failing tests on lower branches hint this is a BC break for multi-kernel apps that generate a different container per name.
Once ppl will have moved out of using name, then we will be free to remove it - in 5.0.
|
Thank you @nicolas-grekas. |
This PR was merged into the 4.2-dev branch. Discussion ---------- [HttpKernel] fix kernel.name deprecation | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Finishes #28809 and makes tests green again. Commits ------- 4964ffc [HttpKernel] fix kernel.name deprecation
Finishes #28809 and makes tests green again.