[WIP] Refactored argument resolving out of the ControllerResolver #11457
Conversation
|
This would work together with #10529 which does the same for the other part of it: the class/method resolving. |
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| public function accepts(Request $request, \ReflectionParameter $parameter) |
There was a problem hiding this comment.
Usually methods like this named supports in the Symfony codebase.
There was a problem hiding this comment.
SensioFrameworkExtraBundle, which did have this feature previously, used supports. We tried to stick as much as possible to the SensioFrameworkExtraBundle API, so the migration will be easy.
| { | ||
| $attributes = $request->attributes->all(); | ||
|
|
||
| return $attributes[$parameter->name]; |
There was a problem hiding this comment.
this should use $request->attributes->get() too
| * | ||
| * @param ArgumentResolverInterface $resolver | ||
| */ | ||
| public function addResolver(ArgumentResolverInterface $resolver) |
There was a problem hiding this comment.
If i remember correct, normally methods like this are called add since they would logically only contain ArgumentResolverInterface instances.
7a8cfae to
b352992
Compare
|
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 |
|
What do core devs think about this PR?
|
|
@wouterj I would say 2 |
d30c01f to
966ffe6
Compare
8e8c34c to
416a33e
Compare
|
Why can't HHVM find the compiler pass class? |
| /** | ||
| * @var ArgumentResolverInterface[] | ||
| */ | ||
| protected $resolvers = array(); |
|
@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 |
|
@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. |
|
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. |
|
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 ArgumentResolver f29bf4c Refactor ArgumentResolverTest cee5106 cs fixes cfcf764 Added an ArgumentResolver with clean extension point 360fc5f 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.