8000 [HttpKernel] Deprecate auto-discovery in Kernel::getName() by ro0NL · Pull Request #24292 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] Deprecate auto-discovery in Kernel::getName() #24292

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[HttpKernel] Deprecate auto-discovery in Kernel::getName() #24292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/TwigBundle/Tests/Functional/NoTemplatingEntryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class NoTemplatingEntryTest extends TestCase
{
public function test()
{
$kernel = new NoTemplatingEntryKernel('dev', true);
$kernel = new NoTemplatingEntryKernel('dev', true, 'app');
$kernel->boot();

$container = $kernel->getContainer();
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/TwigBundle/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"symfony/config": "~3.2|~4.0",
"symfony/twig-bridge": "^3.4|~4.0",
"symfony/http-foundation": "~2.8|~3.0|~4.0",
"symfony/http-kernel": "^3.3|~4.0",
"symfony/http-kernel": "^3.4|~4.0",
"twig/twig": "~1.34|~2.4"
},
"require-dev": {
Expand Down
20 changes: 16 additions & 4 deletions src/Symfony/Component/HttpKernel/Kernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,23 @@ abstract class Kernel implements KernelInterface, RebootableInterface, Terminabl
/**
* Constructor.
*
* @param string $environment The environment
* @param bool $debug Whether to enable debugging or not
* @param string $environment The environment
* @param bool $debug Whether to enable debugging or not
* @param string|null $name The application name
*/
public function __construct($environment, $debug)
public function __construct($environment, $debug, $name = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a BC break.

{
$this->environment = $environment;
$this->debug = (bool) $debug;
$this->rootDir = $this->getRootDir();
$this->name = $this->getName();

if (null === $name) {
$this->name = $this->getName();
} elseif (preg_match('~^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$~', $name)) {
$this->name = $name;
} else {
throw new \InvalidArgumentException(sprintf('"%s" is not a valid kernel name.', $name));
}

if ($this->debug) {
$this->startTime = microtime(true);
Expand Down Expand Up @@ -297,6 +305,10 @@ public function getName()
if (ctype_digit($this->name[0])) {
$this->name = '_'.$this->name;
}

if ('app' !== $this->name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member
@yceruto yceruto Sep 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member
@stof stof Sep 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 app and not src in 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.`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof for 4.0 it should look like getName() { return $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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message should tell users to override Kernel::getName()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to pass the Kernel name as constructor argument, with app default for 4.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it should tell them to pass it in the constructor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, the deprecation should happen in the constructor itself (when passing null)

}
}

return $this->name;
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/Fixtures/KernelForTest.php
View file
Open in desktop
9E88
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@

class KernelForTest extends Kernel
{
public function __construct($environment, $debug, $name = 'app')
{
parent::__construct($environment, $debug, $name);
}

public function getBundleMap()
{
return $this->bundleMap;
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/Fixtures/KernelWithoutBundles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@

class KernelWithoutBundles extends Kernel
{
public function __construct($environment, $debug, $name = 'app')
{
parent::__construct($environment, $debug, $name);
}

public function registerBundles()
{
return array();
Expand Down
25 changes: 19 additions & 6 deletions src/Symfony/Component/HttpKernel/Tests/KernelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ public function testGetName()
{
$kernel = new KernelForTest('test', true);

$this->assertEquals('Fixtures', $kernel->getName());
$this->assertEquals('app', $kernel->getName());
}

public function testOverrideGetName()
Expand Down Expand Up @@ -771,6 +771,10 @@ public function testKernelWithoutBundles()
$this->assertTrue($kernel->getContainer()->getParameter('test_executed'));
}

/**
* @group legacy
* @expectedDeprecation Auto-discovery of the kernel name is deprecated since Symfony 3.4 and won't be supported in 4.0.
*/
public function testKernelRootDirNameStartingWithANumber()
{
$dir = __DIR__.'/Fixtures/123';
Expand Down Expand Up @@ -816,14 +820,14 @@ public function testKernelReset()

$containerClass = get_class($kernel->getContainer());
$containerFile = (new \ReflectionClass($kernel->getContainer()))->getFileName();
unlink(__DIR__.'/Fixtures/cache/custom/FixturesCustomDebugProjectContainer.php.meta');
unlink(__DIR__.'/Fixtures/cache/custom/appCustomDebugProjectContainer.php.meta');

$kernel = new CustomProjectDirKernel();
$kernel->boot();

$this->assertSame($containerClass, get_class($kernel->getContainer()));
$this->assertFileExists($containerFile);
unlink(__DIR__.'/Fixtures/cache/custom/FixturesCustomDebugProjectContainer.php.meta');
unlink(__DIR__.'/Fixtures/cache/custom/appCustomDebugProjectContainer.php.meta');

$kernel = new CustomProjectDirKernel(function ($container) { $container->register('foo', 'stdClass')->setPublic(true); });
$kernel->boot();
Expand All @@ -840,6 +844,15 @@ public function testKernelPass()
$this->assertTrue($kernel->getContainer()->getParameter('test.processed'));
}

/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage "123" is not a valid kernel name.
*/
public function testInvalidKernelName()
{
new KernelForTest('dev', true, '123');
}

/**
* Returns a mock for the BundleInterface.
*
Expand Down Expand Up @@ -895,7 +908,7 @@ protected function getKernel(array $methods = array(), array $bundles = array())
$kernel = $this
->getMockBuilder('Symfony\Component\HttpKernel\Kernel')
->setMethods($methods)
->setConstructorArgs(array('test', false))
->setConstructorArgs(array('test', false, 'app'))
->getMockForAbstractClass()
;
$kernel->expects($this->any())
Expand Down Expand Up @@ -944,7 +957,7 @@ class CustomProjectDirKernel extends Kernel

public function __construct(\Closure $buildContainer = null)
{
parent::__construct('custom', true);
parent::__construct('custom', true, 'app');

$this->baseDir = 'foo';
$this->buildContainer = $buildContainer;
Expand Down Expand Up @@ -982,7 +995,7 @@ class PassKernel extends CustomProjectDirKernel implements CompilerPassInterface
public function __construct(\Closure $buildContainer = null)
{
parent::__construct();
Kernel::__construct('pass', true);
Kernel::__construct('pass', true, 'app');
}

public function process(ContainerBuilder $container)
Expand Down
0