Extracting the argument resolving from the ControllerResolver #18187

linaori · 2016-03-16T08:16:08Z

Q	A
Branch	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#17933 (pre-work)
License	MIT
Doc PR	~

This PR is focused on extracting the argument resolving from the ControllerResolver into a separate part: the ArgumentResolver. This means that the HttpKernel will know about both and extending/implementing/decorating will be a lot easier.

The current setup should be fully backwards compatible. If the HttpKernel gets no ArgumentResolver, the ControllerResolver will be checked for the ArgumentResolverInterface. If not present it will create a new ArgumentResolver. When ControllerResolver fallback 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.

stloyd · 2016-03-16T08:19:42Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

@@ -23,7 +23,7 @@
 *
 * @author Fabien Potencier <fabien@symfony.com>
 */
-class ControllerResolver implements ControllerResolverInterface
+class ControllerResolver extends ArgumentResolver implements ControllerResolverInterface


As the min. PHP version is 5.5.9, IMO this could be trait not "normal" class, WDYT?

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 implementing doGetArguments, which I think is done in Drupal for example (I could be wrong).

Yes, Drupal does use doGetArguments

larowlan · 2016-03-16T08:50:52Z

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.

nicoschoenmaker · 2016-03-16T09:14:22Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolverInterface.php

+     *
+     * @throws \RuntimeException When value for argument given is not provided
+     */
+    public function getArguments(Request $request, $controller);


Is it a conscious decision to not include the "callable" typehint to the $controller variable? It is included in the docs.

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 · 2016-03-24T10:31:17Z

Failing tests unrelated to changes :(

GuilhemN · 2016-03-30T16:19:09Z

src/Symfony/Component/HttpKernel/Controller/TraceableControllerResolver.php

-    /**
-     * {@inheritdoc}
-     */
-    public function getArguments(Request $request, $controller)


this also looks like a bc break

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 ;)

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.

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).

Yeah the suggestion you proposed would fix the deprecation notice.

linaori · 2016-03-31T07:39:16Z

@fabpot @Ener-Getick I've changed the BC layer slightly. The getArguments() is back in the interface but without requiring the ArgumentResolverInterface.

fabpot · 2016-03-31T07:47:56Z

Perfect! 👍

linaori · 2016-03-31T07:49:15Z

src/Symfony/Component/HttpKernel/Controller/TraceableControllerResolver.php

+     */
+    public function getArguments(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);


Woops, a small typo, this should be the, will fix this after travis is done as there was an error which I failed to see

linaori · 2016-03-31T08:01:58Z

@fabpot do you mind if I continue these changes in #18308? This PR starts spewing deprecation notices because of the BC layer which are already fixed in that PR as I hit the same issue.

I can close this and merge my changes in there.

fabpot · 2016-03-31T08:15:40Z

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 · 2016-03-31T08:18:09Z

Continuing in #18308

@wouterj

…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

carsonbot added the Status: Needs Review label Mar 16, 2016

javiereguiluz added Feature HttpKernel labels Mar 16, 2016

stloyd reviewed Mar 16, 2016
View reviewed changes

nicoschoenmaker reviewed Mar 16, 2016
View reviewed changes

Iltar van der Berg added 5 commits March 25, 2016 10:46

Extracting arg resolving from ControllerResolver

c0331a0

FrameworkBundle now requires http-kernel 3.1 or higher

a57d6ae

Fixed the authors

4c997f5

Fixed a failing test due to partial mocking

70c6341

Updated changelog/upgrade files

b3ed1f0

linaori mentioned this pull request Mar 25, 2016

Added an ArgumentResolver with clean extension point #18308

Merged

GuilhemN reviewed Mar 30, 2016
View reviewed changes

Iltar van der Berg added 2 commits March 31, 2016 09:12

Improved the BC layer

750dae9

Fixed the BC layer in HttpKernel

c20eb4b

linaori reviewed Mar 31, 2016
View reviewed changes

linaori closed this Mar 31, 2016

linaori deleted the feature/argument-resolver branch February 8, 2019 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extracting the argument resolving from the ControllerResolver #18187

Extracting the argument resolving from the ControllerResolver #18187

linaori commented Mar 16, 2016

stloyd Mar 16, 2016

linaori Mar 16, 2016

larowlan Mar 16, 2016

larowlan commented Mar 16, 2016

nicoschoenmaker Mar 16, 2016

linaori Mar 16, 2016

linaori commented Mar 24, 2016

GuilhemN Mar 30, 2016

linaori Mar 30, 2016

linaori Mar 30, 2016

GuilhemN Mar 30, 2016

linaori Mar 30, 2016

linaori commented Mar 31, 2016

fabpot commented Mar 31, 2016

linaori Mar 31, 2016

linaori commented Mar 31, 2016

fabpot commented Mar 31, 2016

linaori commented Mar 31, 2016

Extracting the argument resolving from the ControllerResolver #18187

Extracting the argument resolving from the ControllerResolver #18187

Conversation

linaori commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larowlan commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Mar 31, 2016

fabpot commented Mar 31, 2016

Choose a reason for hiding this comment

linaori commented Mar 31, 2016

fabpot commented Mar 31, 2016

linaori commented Mar 31, 2016