[HttpKernel] [DX] Configurable controller layout #25422
Conversation
|
|
||
| $className = $match[1]; | ||
| $controllerName = $match[2]; | ||
| $actionName = $match[3]; |
There was a problem hiding this comment.
Could probably be list(, $className, $controllerName, $actionName) = $match;
| /** @var string */ | ||
| public $controller; | ||
| /** @var string */ | ||
| public $action; |
There was a problem hiding this comment.
Should be private, otherwise they can be modified afterwards. I think this class should probably be final as well.
There was a problem hiding this comment.
Just followed https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Controller/ControllerReference.php here. Can make it final VO to be more protected from modifications
| 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); |
There was a problem hiding this comment.
Is there only 1 alternative? I can imagine there could be multiple alternatives. Assuming 1 alternative is the status quo
There was a problem hiding this comment.
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
| @@ -7,9 +7,14 @@ | |||
| <services> | |||
| <defaults public="false" /> | |||
|
|
|||
| <service id="controller_layout.generic" class="Symfony\Component\HttpKernel\Controller\Layout\GenericControllerLayout"> | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
| public function __construct(KernelInterface $kernel) | ||
| private $layout; | ||
|
|
||
| public function __construct(KernelInterface $kernel, ControllerLayoutInterface $layout = null) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
Should I add |
|
I still haven't understood the problem that this is going to solve, I must admit. Symfony (as far as I know) offers three ways to specify a controller:
Your PR only generalizes the third way. Symfony 4 encourages developers to write bundle-less applications, so the use-cases of that that third way are supposed to decrease now. Also, bundle-less applications cannot make use of your extension point. On top of that: Matching requests to controllers can be done with custom request matchers (if the standard router does not fit), resolving arbitrary strings to controllers can be done with custom controller resolvers. So, imho we have extension points to cover such cases already. Why would I need another way to specify a controller inside a bundle? |
You don't need another way to specify a controller, that fact stands for this patch from it's beginning, the way stands the same We want to re-use logic of Request matching would make us totally copy-paste all this code, since it's not cachaeble, not configurable, etc (since it's a custom code all the time). We have ready-to-use algorythms, which suits us with little modifications, why should be repeat them instead of proposing a patch? |
* Use ~4.1 for http-kernel to fix lower-deps test passing
|
Closing as bundle:controller:action notation is deprecate since #26085. So we won't add new features/refactor this obsolete logic. Thanks for your work on this @scaytrase About the second part of your issue:
This could indeed be refactored. It would be possible to move the logic inside the controller resolver out to a separate class that does not know about Request at all. The existing controller resolver would just read the _controller request attribute and forward to the extracted service. |
First part of #23259 RFC (cc @iltar @nicolas-grekas).
The general idea is to allow reusing
ControllerNameParserfor different than original*Bundle\*Controller::*Actionlayout.Original layout is now stored in
GenericControllerLayoutimplementation which nicely covers the structure logic from usersFor me building bundle+controller+action into string looks like
buildmethod and parsing string into bundle+controller+action looks likeparsemethod. But it turns out than ControllerNameParser::build usesparse(to build from parsed results) and ControllerNameParser::parse usesbuildone (to build from exploded parts). Does this makes sence?