Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] fix kernel.name deprecation#28888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
kernel.container_class already embeds the entropy from kernel.environment and kernel.debug
| /** | ||
| * {@inheritdoc} | ||
| * | ||
| * @deprecated since Symfony 4.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let's be even more explicit and duplicate this info from the interface
| returnstr_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
keeping only the last part is enough IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would say quite the contrary for multi-app projects. Let's stay on the safe side and keep the FQDN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, reverted
| $name =substr($name,1 +$i); | ||
| } | ||
| return$this->name.$name.ucfirst($this->environment).($this->debug ?'Debug' :'').'Container'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
nicolas-grekasOct 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| protectedfunctiongetContainerClass() | ||
| { | ||
| returnstr_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
fabpot commentedOct 16, 2018
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.