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] [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.
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
| $className =$match[1]; | ||
| $controllerName =$match[2]; | ||
| $actionName =$match[3]; |
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 probably belist(, $className, $controllerName, $actionName) = $match;
| /** @var string */ | ||
| public$controller; | ||
| /** @var string */ | ||
| public$action; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just followedhttps://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
| $provider =newAlternativeBundleNameProvider($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.
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
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'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 findingmost similar bundle name, so this definetely only 1. We can return prioritized list here leaving formatting to the end-user code
| <services> | ||
| <defaultspublic="false" /> | ||
| <serviceid="controller_layout.generic"class="Symfony\Component\HttpKernel\Controller\Layout\GenericControllerLayout"> |
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 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).
scaytraseDec 11, 2017 • 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.
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
| publicfunction__construct(KernelInterface$kernel) | ||
| private$layout; | ||
| publicfunction__construct(KernelInterface$kernel,ControllerLayoutInterface$layout =null) |
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.
As there is (currently) no configuration for this, how would someone change this to resolve it differently? Do you have examples ofwhy it should be solved differently?
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.
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.
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 specifya:b:c notation which points toRPC namespace. Currently we have to copy-paste this class to override layout and keep the resolving process
scaytrase commentedDec 11, 2017
Should I add |
derrabus commentedDec 17, 2017
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? |
scaytrase commentedDec 18, 2017
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
Tobion commentedFeb 24, 2018
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?