Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Extracting the argument resolving from the ControllerResolver#18187
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
| * @author Fabien Potencier <fabien@symfony.com> | ||
| */ | ||
| class ControllerResolverimplements ControllerResolverInterface | ||
| class ControllerResolverextends ArgumentResolverimplements ControllerResolverInterface |
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 the min. PHP version is 5.5.9, IMO this could be trait not "normal" class, WDYT?
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.
How would you envision this as a trait? By extending I make sure the children of this class will keep working. By making a trait I would have to also add this trait in the argument resolver.
I've thought about decoration instead, but that would break any child class implementingdoGetArguments, which I think is done in Drupal for example (I could be wrong).
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.
Yes, Drupal does use doGetArguments
larowlan commentedMar 16, 2016
Drupal can work with this, we'd need to move our existing doGetArguments logic in to a new service that implements ArgumentResolverInterface and inject that into our controller resolver to avoid the deprecation warnings when Symfony 4 is out. Makes sense to me. |
| * | ||
| * @throws \RuntimeException When value for argument given is not provided | ||
| */ | ||
| public function getArguments(Request $request, $controller); |
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 it a conscious decision to not include the "callable" typehint to the $controller variable? It is included in the docs.
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.
Changing the actual typehint would be a BC break. I know this was not done for 2.* because of the 5.3 support (introduced in 5.4 afaik). Maybe this typehint could be added in 4.0.
linaori commentedMar 24, 2016
Failing tests unrelated to changes :( |
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| publicfunctiongetArguments(Request$request,$controller) |
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.
this also looks like a bc break
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.
It's not a BC break as it will duck-type on the default implementation. When this code is released, there will be no implementations without this method so everything will be alright. I just have to convince@fabpot of this ;)
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.
Sorry, I thought that was for the interface, this is indeed a BC break. However, the moment the framework is updated to 3.1, it will have both services in the default implementation and will inject them accordingly, so in theory nothing will break.
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.
Internally but someone might depend on this. I would follow@fabpot and not change the interface as we have better solutions (more complex but working).
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.
Yeah the suggestion you proposed would fix the deprecation notice.
linaori commentedMar 31, 2016
@fabpot @Ener-Getick I've changed the BC layer slightly. The |
fabpot commentedMar 31, 2016
Perfect! 👍 |
| */ | ||
| publicfunctiongetArguments(Request$request,$controller) | ||
| { | ||
| @trigger_error(sprintf('This %s method is deprecated as of 3.1 and will be removed in 4.0. Please use the %s instead.',__METHOD__, TraceableArgumentResolver::class),E_USER_DEPRECATED); |
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.
Woops, a small typo,this should bethe, will fix this after travis is done as there was an error which I failed to see
linaori commentedMar 31, 2016
fabpot commentedMar 31, 2016
Sure, one PR is enough. So, feel free to close this one and do all the work in the other one. If we can finish it today, it can still be part of 3.1, which would be excellent. |
linaori commentedMar 31, 2016
Continuing in#18308 |
…iltar, HeahDude)This PR was merged into the 3.1-dev branch.Discussion----------Added an ArgumentResolver with clean extension point| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#17933 (pre-work),#1547,#10710| License | MIT| Doc PR |symfony/symfony-docs#6422**This PR is a follow up for and blocked by:#18187**, relates to#11457 by@wouterj. When reviewing, please take the last commit: [Added an ArgumentResolver with clean extension point](4c092b3)This PR provides:- The ability to tag your own `ArgumentValueResolverInterface`. This means that you can effectively expand on the argument resolving in the `HttpKernel` without having to implement your own `ArgumentResolver`.- The possibility to cache away argument metadata via a new `ArgumentMetadataFactory` which simply fetches the data from the cache, effectively omitting 1 reflection call per request. *Not implemented in this PR, but possible once this is merged.*- The possibility to add a PSR-7 adapter to resolve the correct request, avoids the paramconverters- The possibility to add a value resolver to fetch stuff from $request->query- Drupal could simplify [their argument resolving](https://github.com/drupal/drupal/blob/8.1.x/core/lib/Drupal/Core/Controller/ControllerResolver.php) by a lot- etc.The aim for this PR is to provide a 100% BC variant to add argument resolving in a clean way, this is shown by the 2 tests: `LegacyArgumentResolverTest` and `ArgumentResolverTest`./cc@dawehner@larowlan if you have time, can you check the impact for Drupal? I think this should be a very simple change which should make it more maintainable.Commits-------1bf80c9 Improved DX for the ArgumentResolverf29bf4c Refactor ArgumentResolverTestcee5106 cs fixescfcf764 Added an ArgumentResolver with clean extension point360fc5f Extracting arg resolving from ControllerResolver
This PR is focused on extracting the argument resolving from the
ControllerResolverinto a separate part: theArgumentResolver. This means that theHttpKernelwill know about both and extending/implementing/decorating will be a lot easier.The current setup should be fully backwards compatible. If the
HttpKernelgets noArgumentResolver, theControllerResolverwill be checked for theArgumentResolverInterface. If not present it will create a newArgumentResolver. WhenControllerResolverfallback is used, it will issue deprecation warnings.For future work, this also means that it will be easier to simply put in a cached variant of either, which will improve performance reducing reflection calls. In a future PR, I'd also like to extract the controller creation into a factory of some sorts, but this is future talk.
/cc@dawehner@larowlan can you check the impact in Drupal for this?
/cc@wouterj because you've already attempted something similar.