Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WIP] Refactored argument resolving out of the ControllerResolver#11457
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
dawehner commentedJul 24, 2014
This would work together with#10529 which does the same for the other part of it: the class/method resolving. |
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.
Usually methods like this namedsupports in the Symfony codebase.
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.
SensioFrameworkExtraBundle, which did have this feature previously, usedsupports. We tried to stick as much as possible to the SensioFrameworkExtraBundle API, so the migration will be easy.
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 should use$request->attributes->get() too
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.
If i remember correct, normally methods like this are calledadd since they would logically only containArgumentResolverInterface instances.
7a8cfae tob352992Comparewouterj commentedFeb 19, 2015
I just revisited this PR for 3.0, as it was impossible to make a BC pull request. I've fixed the comments. This could be made so generic that it will work for any callable (not just controllers). This would cover#10710 |
wouterj commentedApr 3, 2015
What do core devs think about this PR?
|
fabpot commentedApr 4, 2015
@wouterj I would say 2 |
d30c01f to966ffe6Compare8e8c34c to416a33eComparewouterj commentedApr 9, 2015
Why can't HHVM find the compiler pass class? |
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
stof commentedMay 26, 2015
@wouterj I just thought about a BC way introduce the ArgumentResolver concept in Symfony: inject the ArgumentResolverManager inside the ControllerResolver for now (with a default value matching the current behavior) to be used in its existing This would allow to start using this extension point already. Building an ArgumentResolver which would call the ControllerResolver old API would not allow to preserve BC, because you would not have BC for |
wouterj commentedJun 7, 2015
@stof this isn't FC for standalone usage, is it? As you have to pass the argument resolver manager with custom resolvers to ControllerResolver in 2.8 and to AppKernel in 3.0. |
wouterj commentedJun 7, 2015
Hmm, we can of course still pass the argument resolver manager to AppKernel and introduce a setArgumentResolverManager method in the ControllerResolver (which is called in AppKernel). This would fix that. |
wouterj commentedJun 13, 2015
Closing in favor of#14971 |
…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
Todo
BC
I've done my best to make this BC. It is now fully BC except from one case: If the logic in
ControllerResolver#getArguments()is no longer used. If one would have used that method for custom argument resolving logic, it is no ignored. There should be a way that we can support both methods at the same time (in other words, create an argument resolver which uses the controller resolver).Thanks
A huge thanks to@iltar for giving me the idea and helping me setting up this PR.