[HttpKernel] Deprecate auto-discovery in Kernel::getName() #24292
Conversation
| @@ -297,6 +305,10 @@ public function getName() | |||
| if (ctype_digit($this->name[0])) { | |||
| $this->name = '_'.$this->name; | |||
| } | |||
|
|
|||
| if ('app' !== $this->name) { | |||
There was a problem hiding this comment.
Why app here? Why does that indicate that it's been auto-discovered?
And what would this look like in 4.0? Would this method exist... but throw an exception? (so that the user either needs to override this method or pass in as an arg?)
There was a problem hiding this comment.
And what would this look like in 4.0? Would this method exist... but throw an exception? (so that the user either needs to override this method or pass in as an arg?)
For 4.0 auto-discovery is removed and it should look like a simple getter.
There was a problem hiding this comment.
@weaverryan AFAIK, in 4.0, this method would be getName() { return 'app';}. So anyone getting a different name through auto-discovery will need to override the method instead, to see the name explicitly (and so this code would not run anymore).
If the auto-discovered name is app (which is the case for all standard-edition projects), migrating to 4.0 would not break things as the hardcoded app would do the same.
We have 2 solutions in 4.0:
- make the method return an hardcoded name and tell multi-kernel projects that they should override the method to avoid bugs. Existing multi-kernel projects will be warned for this by the deprecation. And new multi-kernels project should be warned about it with a big warning in the 4.0 documentation about multi-kernel setup. This is the approach indicated by the current deprecation.
- make the method abstract. This is a bigger BC break as all SE projects would have to implement the method, but it would make the explanation simpler for multi-kernel projects as they would work the same (everyone has to overwrite the method). For flex projects, this is not an issue as the recipe is doing the override already (to have it named
appand notsrcin 3.x projects), so it would just mean keeping the override in the 4.0 recipe.
If we go for the second option, the deprecation should instead be moved at the very beginning of getName and say Not overriding Kernel::getName is deprecated since Symfony 3.4. The method will be astract in 4.0.`
There was a problem hiding this comment.
@stof for 4.0 it should look like getName() { return $this->name; }
|
I would suggest to use the FQCN instead of |
| @@ -297,6 +305,10 @@ public function getName() | |||
| if (ctype_digit($this->name[0])) { | |||
| $this->name = '_'.$this->name; | |||
| } | |||
|
|
|||
| if ('app' !== $this->name) { | |||
| @trigger_error(sprintf('Auto-discovery of the kernel name is deprecated since Symfony 3.4 and won\'t be supported in 4.0.'), E_USER_DEPRECATED); | |||
There was a problem hiding this comment.
The message should tell users to override Kernel::getName()
There was a problem hiding this comment.
The idea is to pass the Kernel name as constructor argument, with app default for 4.0
There was a problem hiding this comment.
then it should tell them to pass it in the constructor
There was a problem hiding this comment.
actually, the deprecation should happen in the constructor itself (when passing null)
|
@fabpot FQCN is incompatible with the current regex. And allowing more chars is a BC break (as it makes the name unsuitable for some of its current usage, especially if we allow |
|
Passing the name in the constructor makes things harder to use actually. The kernel is instantiated in several different places in the kernel (in the front controller, in the console, in each KernelTestCase test, in your Behat testsuite if you use it with the Symfony2Extension, etc...), and so it forces to duplicate the name everywhere (they must use the same name to reuse the cache). I prefer the approach I thought about: keep overriding |
|
@stof I know, but computing a hash out of the class name would work well. |
For single Kernel approach having a default Kernel name Now for multiple kernels approach, we need anyway update the kernel to instantiate everywhere, right? either with the documented https://symfony.com/doc/current/configuration/multiple_kernels.html multi-class approach or name-based virtual kernel we are forced to update all entry points to set the class name or the kernel name respectively (just once for v-kernel by using env var for kernel name).
You could still, even configuring it
Why is it wrong to define the kernel name when it is instantiated? Indeed it made possible to optimize the multi-kernel class approach with just one Kernel. |
Im not sure. We are really flexible this way, and explicit. I tend to agree with @yceruto, IMHO the current approach is too opinionated in design. For "we need to init kernel everywhere" i suggest a
Yes, interesting. Ill look into that. |
|
@ro0NL an env variable won't work for a multi-kernel approach. You may need to use both kernels in the same process (and even in other cases, it might be hard to change the env variable depending on the kernel being used in this process). Creating a kernel/config.php to configure this would just make things more complex than defining thi in your src/Kernel.php file where the kernel class is defined. For projects, you should never need to pass different names for the same kernel (the name is here to make sure that multi-kernel setups don't reuse the same class names for the DIC and router generated classes, so changing the name in your project would mean your breaking the usage of your cache) |
|
I still think that getting rid of the name altogether is the best option. |
|
@fabpot that practically means virtual kernel support needs to happen on user's side. Might be a path, but out-of-the-box support with For this PR; i really like kernels having a name by design. For technical reasons, but also easy identification i guess. Basically not sure if getting rid of it is really worth it. It's just the auto-discovery thingy which seems a bit opinionated; not really practical/helping today. What about a new form of (default) auto-discovery, i.e. For the constructor arg; that be a new feature. Could be left out i guess. cc @yceruto |
| */ | ||
| public function __construct($environment, $debug) | ||
| public function __construct($environment, $debug, $name = null) |
|
Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw. |
|
I'd suggest to do nothing here and close this one. Not really worth it, isn't it? |
not sure you want to settle with |
|
We already did to me :) |
|
well.. hm.. ok. You can override anyway. Closing as there no open issues currently. |
The auto-discovery of kernel name doesnt work well with new structure. You probably want to override it.
@fabpot suggested to get rid of getName(), however; this is an important feature actually. It enables multi-kernel approach. See https://github.com/yceruto/symfony-skeleton-vkernel for example, but also things like
ApiKernel,AppKernellive today.In core it's crucial for
Kernel::getContainerClass()and%kernel.name%might being used a lot today. So the concept of a kernel name should not be deprecated to me.This PR suggests the default kernel name will be
"app"in 4.0, but any other can be specified as constructor arg. I.e. set it from APP_NAME env.cc @yceruto