Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[HttpKernel] [DX] Configurable controller layout #25422

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 5 commits into from
Closed

[HttpKernel] [DX] Configurable controller layout #25422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f40e775
Extract controller layout
scaytrase Dec 10, 2017
845eaf8
Fix tests
scaytrase Dec 10, 2017
a045a25
Apply fabpot patch
scaytrase Dec 10, 2017
2a5ce45
Use list
Dec 11, 2017
85ece54
Make Action Reference be VO
scaytrase Dec 22, 2017
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
10 changes: 5 additions & 5 deletions 10 src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ class RouterDebugCommand extends Command
{
protected static $defaultName = 'debug:router';
private $router;
private $parser;

public function __construct(RouterInterface $router)
public function __construct(RouterInterface $router, ControllerNameParser $parser)
{
parent::__construct();

$this->router = $router;
$this->parser = $parser;
}

/**
Expand Down Expand Up @@ -109,9 +111,8 @@ protected function execute(InputInterface $input, OutputInterface $output)
private function convertController(Route $route)
{
if ($route->hasDefault('_controller')) {
$nameParser = new ControllerNameParser($this->getApplication()->getKernel());
try {
$route->setDefault('_controller', $nameParser->build($route->getDefault('_controller')));
$route->setDefault('_controller', $this->parser->build($route->getDefault('_controller')));
} catch (\InvalidArgumentException $e) {
}
}
Expand All @@ -133,9 +134,8 @@ private function extractCallable(Route $route)
}
}

$nameParser = new ControllerNameParser($this->getApplication()->getKernel());
try {
$shortNotation = $nameParser->build($controller);
$shortNotation = $this->parser->build($controller);
$route->setDefault('_controller', $shortNotation);

return $controller;
Expand Down
64 changes: 14 additions & 50 deletions 64 src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@

namespace Symfony\Bundle\FrameworkBundle\Controller;

use Symfony\Component\HttpKernel\Controller\ActionReference;
use Symfony\Component\HttpKernel\Controller\ControllerLayoutInterface;
use Symfony\Component\HttpKernel\Controller\Layout\GenericControllerLayout;
use Symfony\Component\HttpKernel\KernelInterface;
use Symfony\Component\HttpKernel\Util\AlternativeBundleNameProvider;

/**
* ControllerNameParser converts controller from the short notation a:b:c
Expand All @@ -24,9 +28,12 @@ class ControllerNameParser
{
protected $kernel;

public function __construct(KernelInterface $kernel)
private $layout;

public function __construct(KernelInterface $kernel, ControllerLayoutInterface $layout = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

As there is (currently) no configuration for this, how would someone change this to resolve it differently? Do you have examples of why it should be solved differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

General idea is to reuse the class itself for other layouts. So we can use default generic layout for controllers and custom layouts with same logic for something else.

Am not sure that this class should live here, but not in http-kernel component (to be used standalone), but moving a class is a tricky thing, so I haven't suggest it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be more concrete we have secondary routing for RPC controllers allowing to specify a:b:c notation which points to RPC namespace. Currently we have to copy-paste this class to override layout and keep the resolving process

{
$this->kernel = $kernel;
$this->layout = $layout ?: new GenericControllerLayout($this->kernel);
}

/**
Expand Down Expand Up @@ -60,19 +67,16 @@ public function parse($controller)
$originalController
);

if ($alternative = $this->findAlternative($bundleName)) {
$provider = new AlternativeBundleNameProvider($this->kernel);

if ($alternative = $provider->findAlternative($bundleName)) {
$message .= sprintf(' Did you mean "%s:%s:%s"?', $alternative, $controller, $action);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there only 1 alternative? I can imagine there could be multiple alternatives. Assuming 1 alternative is the status quo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just moved this responsibility (https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php#L107-L132) out to the separate class since it could be reused in many bundle guessing snippets

It's logic is about finding most similar bundle name, so this definetely only 1. We can return prioritized list here leaving formatting to the end-user code

}

throw new \InvalidArgumentException($message, 0, $e);
}

$try = $bundle->getNamespace().'\\Controller\\'.$controller.'Controller';
if (class_exists($try)) {
return $try.'::'.$action.'Action';
}

throw new \InvalidArgumentException(sprintf('The _controller value "%s:%s:%s" maps to a "%s" class, but this class was not found. Create this class or check the spelling of the class and its namespace.', $bundleName, $controller, $action, $try));
return $this->layout->build(new ActionReference($bundle, $controller, $action));
}

/**
Expand All @@ -86,48 +90,8 @@ public function parse($controller)
*/
public function build($controller)
{
if (0 === preg_match('#^(.*?\\\\Controller\\\\(.+)Controller)::(.+)Action$#', $controller, $match)) {
throw new \InvalidArgumentException(sprintf('The "%s" controller is not a valid "class::method" string.', $controller));
}

$className = $match[1];
$controllerName = $match[2];
$actionName = $match[3];
foreach ($this->kernel->getBundles() as $name => $bundle) {
if (0 !== strpos($className, $bundle->getNamespace())) {
continue;
}

return sprintf('%s:%s:%s', $name, $controllerName, $actionName);
}

throw new \InvalidArgumentException(sprintf('Unable to find a bundle that defines controller "%s".', $controller));
}

/**
* Attempts to find a bundle that is *similar* to the given bundle name.
*/
private function findAlternative(string $nonExistentBundleName): ?string
{
$bundleNames = array_map(function ($b) {
return $b->getName();
}, $this->kernel->getBundles());

$alternative = null;
$shortest = null;
foreach ($bundleNames as $bundleName) {
// if there's a partial match, return it immediately
if (false !== strpos($bundleName, $nonExistentBundleName)) {
return $bundleName;
}

$lev = levenshtein($nonExistentBundleName, $bundleName);
if ($lev <= strlen($nonExistentBundleName) / 3 && (null === $alternative || $lev < $shortest)) {
$alternative = $bundleName;
$shortest = $lev;
}
}
$reference = $this->layout->parse($controller);

return $alternative;
return sprintf('%s:%s:%s', $reference->getBundle()->getName(), $reference->getController(), $reference->getAction());
}
}
1 change: 1 addition & 0 deletions 1 src/Symfony/Bundle/FrameworkBundle/Resources/config/console.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@

<service id="console.command.router_debug" class="Symfony\Bundle\FrameworkBundle\Command\RouterDebugCommand">
<argument type="service" id="router" />
<argument type="service" id="controller_name_converter" />
<tag name="console.command" command="debug:router" />
</service>

Expand Down
5 changes: 5 additions & 0 deletions 5 src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,14 @@
<services>
<defaults public="false" />

<service id="controller_layout.generic" class="Symfony\Component\HttpKernel\Controller\Layout\GenericControllerLayout">
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably be the FQCN as ID, though I'm not sure if this is default for the Symfony Core. If not, I'd like to see an alias for autowiring purposes (perhaps on the interface as well if this is the default).

Copy link
Contributor Author

@scaytrase scaytrase Dec 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

There are no FQCN identifiers in this file, neither did I. I think this is a step for further improvements outside of the scope of this PR

<argument type="service" id="kernel" />
</service>

<service id="controller_name_converter" class="Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser">
<tag name="monolog.logger" channel="request" />
<argument type="service" id="kernel" />
<argument type="service" id="controller_layout.generic" />
</service>

<service id="controller_resolver" class="Symfony\Bundle\FrameworkBundle\Controller\ControllerResolver">
Expand Down
6 changes: 4 additions & 2 deletions 6 src/Symfony/Bundle/FrameworkBundle/Tests/Command/RouterDebugCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use PHPUnit\Framework\TestCase;
use Symfony\Bundle\FrameworkBundle\Console\Application;
use Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser;
use Symfony\Component\Console\Tester\CommandTester;
use Symfony\Bundle\FrameworkBundle\Command\RouterDebugCommand;
use Symfony\Component\HttpKernel\KernelInterface;
Expand Down Expand Up @@ -52,8 +53,9 @@ public function testDebugInvalidRoute()
*/
private function createCommandTester()
{
$application = new Application($this->getKernel());
$application->add(new RouterDebugCommand($this->getRouter()));
$kernel = $this->getKernel();
$application = new Application($kernel);
$application->add(new RouterDebugCommand($this->getRouter(), new ControllerNameParser($kernel)));

return new CommandTester($application->find('debug:router'));
}
Expand Down
6 changes: 4 additions & 2 deletions 6 src/Symfony/Bundle/FrameworkBundle/Tests/Command/RouterMatchCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use PHPUnit\Framework\TestCase;
use Symfony\Bundle\FrameworkBundle\Console\Application;
use Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser;
use Symfony\Component\Console\Tester\CommandTester;
use Symfony\Bundle\FrameworkBundle\Command\RouterMatchCommand;
use Symfony\Bundle\FrameworkBundle\Command\RouterDebugCommand;
Expand Down Expand Up @@ -46,9 +47,10 @@ public function testWithNotMatchPath()
*/
private function createCommandTester()
{
$application = new Application($this->getKernel());
$kernel = $this->getKernel();
$application = new Application($kernel);
$application->add(new RouterMatchCommand($this->getRouter()));
$application->add(new RouterDebugCommand($this->getRouter()));
$application->add(new RouterDebugCommand($this->getRouter(), new ControllerNameParser($kernel)));

return new CommandTester($application->find('router:match'));
}
Expand Down
6 changes: 1 addition & 5 deletions 6 src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerNameParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ private function createParser()
{
$bundles = array(
'SensioCmsFooBundle' => $this->getBundle('TestBundle\Sensio\Cms\FooBundle', 'SensioCmsFooBundle'),
'FoooooBundle' => $this->getBundle('TestBundle\FooBundle', 'FoooooBundle'),
'FooBundle' => $this->getBundle('TestBundle\FooBundle', 'FooBundle'),
);

Expand All @@ -161,11 +162,6 @@ private function createParser()
}))
;

$bundles = array(
'SensioCmsFooBundle' => $this->getBundle('TestBundle\Sensio\Cms\FooBundle', 'SensioCmsFooBundle'),
'FoooooBundle' => $this->getBundle('TestBundle\FooBundle', 'FoooooBundle'),
'FooBundle' => $this->getBundle('TestBundle\FooBundle', 'FooBundle'),
);
$kernel
->expects($this->any())
->method('getBundles')
Expand Down
2 changes: 1 addition & 1 deletion 2 src/Symfony/Bundle/FrameworkBundle/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"symfony/config": "~3.4|~4.0",
"symfony/event-dispatcher": "~3.4|~4.0",
"symfony/http-foundation": "~3.4|~4.0",
"symfony/http-kernel": "~3.4|~4.0",
"symfony/http-kernel": "~3.4|~4.1",
"symfony/polyfill-mbstring": "~1.0",
"symfony/filesystem": "~3.4|~4.0",
"symfony/finder": "~3.4|~4.0",
Expand Down
51 changes: 51 additions & 0 deletions 51 src/Symfony/Component/HttpKernel/Controller/ActionReference.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Controller;

use Symfony\Component\HttpKernel\Bundle\BundleInterface;

/**
* Class for holding bundle + controller name + action name.
*
* @author Pavel Batanov <pavel@batanov.me>
*/
final class ActionReference
{
/** @var BundleInterface */
private $bundle;
/** @var string */
private $controller;
/** @var string */
private $action;

public function __construct(BundleInterface $bundle, string $controller, string $action)
{
$this->bundle = $bundle;
$this->controller = $controller;
$this->action = $action;
}

public function getBundle(): BundleInterface
{
return $this->bundle;
}

public function getController(): string
{
return $this->controller;
}

public function getAction(): string
{
return $this->action;
}
}
45 changes: 45 additions & 0 deletions 45 src/Symfony/Component/HttpKernel/Controller/ControllerLayoutInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Controller;

use Symfony\Component\HttpKernel\Exception\ControllerLayoutException;

/**
* Responsible for build FQCN::action from bundle + controller name + action name
* and parse FQCN::action into bundle + controller name + action name.
*
* @author Pavel Batanov <pavel@batanov.me>
*/
interface ControllerLayoutInterface
{
/**
* Decompose controller string into bundle, controller and action.
*
* @param string $controller
*
* @return ActionReference
*
* @throws ControllerLayoutException
*/
public function parse(string $controller): ActionReference;

/**
* Builds a controller string for given bundle, controller, and action.
*
* @param ActionReference $action
*
* @return string
*
* @throws ControllerLayoutException
*/
public function build(ActionReference $action): string;
}
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.