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] Deprecate the Kernel name#28809
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
fabpot commentedOct 10, 2018
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | #26904 |
| License | MIT |
| Doc PR | n/a |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| protectedfunctiongetContainerClass() | ||
| { | ||
| return$this->name.ucfirst($this->environment).($this->debug ?'Debug' :'').'ProjectContainer'; | ||
| return'app'.ucfirst($this->environment).($this->debug ?'Debug' :'').'ProjectContainer'; |
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.
shouldn't we keep using it for now, for BC ? Otherwise, it would be a BC break for people using multiple kernels (with different names) and expecting to have the container classes not clashing together.
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.
You mean when a multi-kernel app share the same cache?
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 don't think this is worth it. As of Symfony 4.x, we are only supporting Flex (so, doing it in 3.4 was not possible, but I think this is totally legitimate now).
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, is it better now?
ce81b5b to59048e2Compare| protectedfunctiongetContainerClass() | ||
| { | ||
| return$this->name.ucfirst($this->environment).($this->debug ?'Debug' :'').'ProjectContainer'; | ||
| return'app'.md5(get_class($this)).ucfirst($this->environment).($this->debug ?'Debug' :'').'ProjectContainer'; |
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.
Could this potentially break existing applications with multiple kernels?
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.
How? Each Kernel class must have a different FQCN anyway, so it's still unique.
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.
Having additional arguments in the application kernel class that are different for each instance does work too, doesn't it?
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.
It can work, but this won't break with my change as in this case, the cache cannot be shared between apps (today already).
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 will break apps rely on the kernel name to create "virtual" kernels. For example, I've some projects based on this ideahttps://github.com/yceruto/symfony-skeleton-vkernel.
I don't know whether this practice is right or not, but it simplify our projects sharing vendors, config and src. It work well thank to the kernel name and just one Kernel 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.
Then, you need to override thegetContainerClass().
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
xabbuh commentedOct 13, 2018
tests are failing |
fabpot commentedOct 15, 2018
Tests fixed |
This PR was merged into the 4.2-dev branch.Discussion----------[HttpKernel] Deprecate the Kernel name| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#26904| License | MIT| Doc PR | n/a<!--Write a short README entry for your feature/bugfix here (replace this comment block.)This will help people understand your PR and can be used as a start of the Doc PR.Additionally: - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the master branch.-->Commits-------98ff750 [HttpKernel] deprecated the Kernel name
| protectedfunctiongetContainerClass() | ||
| { | ||
| return$this->name.ucfirst($this->environment).($this->debug ?'Debug' :'').'ProjectContainer'; | ||
| returnstr_replace('\\','_',\get_class($this)).ucfirst($this->environment).($this->debug ?'Debug' :'').'Container'; |
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.
This change breaks BC by removing the "name" (the basename of the project_dir really) from the dimensions that compose separate namespaces.
This is why the CI is red (and was also caught before merging ;) )
Side note: the container class is also a legacy concept; its real name is random (it's the hash of its contents actually.)
The only real remaining use case of the container class is for generating thename of the file that serves as entry point for loading the container.
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
This PR was squashed before being merged into the 4.2-dev branch (closes#28939).Discussion----------[WebProfilerBundle] Remove application name| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->This fixes a regression in master, i think after#28809 (or something related)There's still a "application name" concept out there today, and it's `ConfigDataCollector::getApplicationName()` 🤔In my case the service resolved to```php$e = new \Symfony\Component\HttpKernel\DataCollector\ConfigDataCollector(\basename('/app'));```Causing the app name to be, well, `app` (again :P). It was really confusing.Before:(the "a" is from "app" here)After:orI think we need to deprecate the argument for real?cc@fabpotCommits-------f5c355e [WebProfilerBundle] Remove application name
