Inject the argument resolver into HttpKernel::__construct

I am a novice and would like to offer my patch - it defines the Symfony ArgumentResolver as a service in core.services.yml and adds it as an argument for HttpKernel.

Looking at ArgumentResolver->__construct():

    public function __construct(ArgumentMetadataFactoryInterface $argumentMetadataFactory = null, array $argumentValueResolvers = array())
    {
        $this->argumentMetadataFactory = $argumentMetadataFactory ?: new ArgumentMetadataFactory();
        $this->argumentValueResolvers = $argumentValueResolvers ?: self::getDefaultArgumentValueResolvers();
    }

This constructor defines optional parameters. In core,services.yml I did not add arguments to the service definition for ArgumentResolver. There is only one implementation of ArgumentMetadataFactoryInterface: ArgumentMetadataFactory which does not need and parameters on construction by itself.

Is that the way?

Thank you for your feedback. I wrote a new issue as you suggested and referenced to this one: https://www.drupal.org/node/2916310

Merged #2949627: Fix deprecated usages of Symfony HttpKernel in. Crediting @grahamC.

Hmmm... I don't think we can inject the symfony one in. Our controllers and arguments don't work that way. We need to separate the ControllerResolverInterface and ArgumentResolverInterface from Drupal's ControllerResolver. Fun.

This is going to be super hard to fix properly in Drupal 8 because it is going to be changes to things like \Drupal\Core\Controller\FormController which is extended by modules like ctools - so changing the constructor arguments in core will break that. We need to inject a new ArgumentResolverInterface service in order to conform to the expectations for Symfony 4.

Hmmm maybe #10 is okay because if something passes in the controller resolver it still implements ArgumentresolverInterface - and any usages of the methods will get a deprecation message. But nothing should be broken.

Patch attached is a implementation.

More things we need to swap out.

A few more

More...

Final fix.

I've run the Drupal\ctools\Tests\Wizard\CToolsWizardTest which tests ctool's form controllers still works with this patch applied. (It currently fails on 8.6.x HEAD and this patch in the same way - but the ctools.wizard.form services are not broken because the @controller_resolver implements ArgumentResolverInterface.

+++ b/core/lib/Drupal/Core/Controller/FormController.php
@@ -33,13 +34,13 @@
    */
-  public function __construct(ControllerResolverInterface $controller_resolver, FormBuilderInterface $form_builder) {
-    $this->controllerResolver = $controller_resolver;
+  public function __construct(ArgumentResolverInterface $argument_resolver, FormBuilderInterface $form_builder) {
+    $this->argumentResolver = $argument_resolver;
     $this->formBuilder = $form_builder;

entity_browser also has a subclass of this.

I don't quite understand yet how this doesn't break, those two interfaces do not seem to share anything?

@Berdir yeah it's super fun - basically it is clever Symfony. Our \Drupal\Core\Controller\ControllerResolver extends \Symfony\Component\HttpKernel\Controller\ControllerResolver which implements \Symfony\Component\HttpKernel\Controller\ControllerResolverInterface and for BC \Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface. So our concrete implementation implements both of those interfaces too. And \Symfony\Component\HttpKernel\Controller\ControllerResolverInterface::getArguments and \Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface::getArguments have the same signature apart from the fact that ControllerResolverInterface::getArguments is deprecated. It took me a while to get my head around this.

Fixing @todo.

I'm making this a major because without this patch we are currently triggering a silenced deprecation on every request made to a site.

Add a change record https://www.drupal.org/node/2959408

#22: Ok, I get it now. If anyone else is confused, here is the class diagram:

We did the type hint on our own ControllerResolverInterface, which implements both symfony interfaces now. So the old class is compatible with both type hints. The new class however just implements the new one, so there is a unlikely chance that someone has the old type hint and somehow ends up getting the new implementation, but I don't quite see how.

@dawehner: The change is not necessary for the first deprecation message that we are removing, but I think it is for the second because that' about calling getArguments() I guess we could split it up and do those parts separataly but it's pretty closely related and conceptually about the same deprecation/change?

+++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php
@@ -129,6 +129,10 @@ protected function createController($controller) {
+    // Note the duplicates the deprecation message of

nit, Note this instead of Note the

Looking at https://github.com/symfony/symfony/pull/18308 I wonder if I'm doing this right. Maybe we should be using Symfony's \Symfony\Component\HttpKernel\Controller\ArgumentResolver and adding new \Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface objects for our special ways to resolve arguments.

This would potentially allow as to cache the result of \Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactory::createArgumentMetadata() and not do multiple relections on the controller in a request.

To go the full Symfony way we need to fix our container dumper - see #2961380: Support IteratorArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue.

Patch attach reworks this to use value resolvers. We need to performance test this since this is on the critical path. No interdiff because this is a new approach.

In the future it'll allow us to support variadic arguments and more. For example, we could do something like \Symfony\Component\HttpKernel\Controller\ArgumentResolver\ServiceValueResolver and then just inject services into controllers with no need for the ::create method we have a the moment. For way less boilerplate.

Discussed with @dawehner he pointed out that we shouldn't really add a new extension point here. We might choose to do that later maybe to remove param convertors. Also not adding the tagged service extension point here allows us to not need #2961380: Support IteratorArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue.

Whoops forgot a bit.

Ho hum.

I tried to profile this change wrt to requesting the node view or a single node using blackfire. It didn't even show up in the graphs because blackfire will ignore anything that

take less that 1% of the global costs on all dimensions (time, cpu, memory, etc.).

Resolved the @todo's.

1. > I tried to profile this change wrt to requesting the node view or a single node using blackfire.

   Maybe we should profile it on a minimal controller response that returns a response object, to avoid most of the other things, something like an autocomplete controller for example or even something that returns a static response?

2. +++ b/core/core.services.yml
   @@ -708,12 +708,39 @@ services:
   +  argument_resolver.default:
   +    class: Symfony\Component\HttpKernel\Controller\ArgumentResolver\DefaultValueResolver
   +    tags:
   +      - { name: http_kernel.controller.argument_resolver, priority: -100 }
   +    public: false
   

   looks like this is the only one left with the tag, is that really needed now? did not review the recent patches.

3. +++ b/core/lib/Drupal/Core/Controller/ArgumentResolver/RawParameterValueResolver.php
   @@ -0,0 +1,28 @@
   +/**
   + * @todo
   + */
   

   still needa bunch of docblocks on these classes.

   Crosspost :)

4. +++ b/core/lib/Drupal/Core/Controller/FormController.php
   @@ -63,7 +64,7 @@ public function getContentResult(Request $request, RouteMatchInterface $route_ma
        $request->attributes->set('form', []);
        $request->attributes->set('form_state', $form_state);
   -    $args = $this->controllerResolver->getArguments($request, [$form_object, 'buildForm']);
   +    $args = $this->argumentResolver->getArguments($request, [$form_object, 'buildForm']);
        $request->attributes->remove('form');
        $request->attributes->remove('form_state');
   

   so the type might be backwards compatible, but what about the varible name? What if someone overrides the class, uses this but did not change the constructor?

   protected variables are excluded I guess, but still unfortunate if we break it like this?

5. +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
   @@ -27,10 +35,13 @@ class TitleResolver implements TitleResolverInterface {
       * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
       *   The translation manager.
   +   * @param \Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface $argument_resolver
   +   *   The argument resolver.
       */
   -  public function __construct(ControllerResolverInterface $controller_resolver, TranslationInterface $string_translation) {
   +  public function __construct(ControllerResolverInterface $controller_resolver, TranslationInterface $string_translation, ArgumentResolverInterface $argument_resolver) {
        $this->controllerResolver = $controller_resolver;
        $this->stringTranslation = $string_translation;
   +    $this->argumentResolver = $argument_resolver;
      }
   

   This one for example adds a new argument and keeps the old one, what's the difference?

Thanks for the review @Berdir
1. Fixed
2. :)
3. Hmmm... do we have any examples of this in contrib? I feel that not changing the arguments to the constructor is better for BC. I guess we could keep the property and mark it for removal in 9? Can't do anything to trigger a deprecation though :(
4. Because the TitleResolver does $callable = $this->controllerResolver->getControllerFromDefinition($callback);

3. We could if we remove it and go through __get(). Not sure if it's worth doing that, but it should work ;)

+++ b/core/lib/Drupal/Core/Controller/FormController.php
@@ -17,11 +18,11 @@
-  protected $controllerResolver;
+  protected $argumentResolver;

+++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
@@ -48,11 +48,11 @@ class LocalActionManager extends DefaultPluginManager implements LocalActionMana
-  protected $controllerResolver;
+  protected $argumentResolver;

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -51,11 +51,11 @@ class LocalTaskManager extends DefaultPluginManager implements LocalTaskManagerI
-  protected $controllerResolver;
+  protected $argumentResolver;

I've thought about @Berdir's comment and I think we need to implement a more complicated BC layer :(

Here's a patch with more BC.

I think this is fine now.

The only BC thing I can see is that controllerResolver is now only set if the old service is passed in, If there is a subclass out there that uses $this->controllerResolver without overriding the constructor then it would be broken. But I guess that's not very likely and AFAIK, protected properties are technically excluded from BC (so are constructors...)

@dawehner I named it this to steer well clear of \Drupal\Component\Utility\ArgumentsResolver - so I went with a service name that indicated its for resolving arguments for a controller as part of the http kernel.

@dawehner see your comment #3 :)

All nitpicks but just enough to mark CNW.

1. +++ b/core/lib/Drupal/Core/Controller/FormController.php
   @@ -16,10 +17,25 @@
   +   *
   +   * @deprecated
   +   *   Using the 'controller_resolver' service as the first argument is
   +   *   deprecated, use the 'http_kernel.controller.argument_resolver' instead.
   +   *   If your subclass requires the 'controller_resolver' service add it as an
   +   *   additional argument.
   +   *
   

   It's a little bit odd that this doesn't refer to the property. Would expect it to read something like "Deprecated property that is only assigned when the the controller_resolver service is used as the first parameter" or similar. Also it's not technically the service that's deprecated but passing an instance of ControllerResolverInterface - I can never quite figure out when to refer to interfaces vs. services when they're 1-1 in practice. The service reference tells you where to fix it I guess but I also sit there thinking "How do you know I injected this particularly named service".

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -128,9 +144,13 @@ class LocalTaskManager extends DefaultPluginManager implements LocalTaskManagerI
        $this->factory = new ContainerFactory($this, '\Drupal\Core\Menu\LocalTaskInterface');
   -    $this->controllerResolver = $controller_resolver;
   +    $this->argumentResolver = $argument_resolver;
   +    if ($argument_resolver instanceof ControllerResolverInterface) {
   +      @trigger_error("Using the 'controller_resolver' service as the first argument is deprecated, use the 'http_kernel.controller.argument_resolver' instead. If your subclass requires the 'controller_resolver' service add it as an additional argument. See https://www.drupal.org/node/2959408.", E_USER_DEPRECATED);
   +      $this->controllerResolver = $argument_resolver;
   +    }
        $this->requestStack = $request_stack;
   

   I saw Berdir's comment before the code, then had to read this a couple of times - but agreed it's fine. The constructor and the property are both fair game, and this allows the deprecated property and bc layer to be dropped cleanly in 9.x. Don't think we've done it like this before, but it probably won't be the last time.

3. +++ b/core/tests/Drupal/Tests/Core/Controller/FormControllerTest.php
   @@ -0,0 +1,30 @@
   + * Tests that the FormController class.
   

   We should either remove 'that' or finish the sentence.

Thanks for the review @catch.

1. Good point - I had just copied the deprecation from the __construct() where it makes more sense. I've changed this to your suggestion and added an @see to the constructor to hopefully close the loop.
2. Yep I think when we're dealing with a base class that might be used by contrib and custom we need to be careful with constructors and properties. Similar things have come up in #2894261: Deprecated service entity.manager needs to be replaced with entity_type.manager in ContentEntityForm
3. Removed the that so we can add additional test coverage of FormController here without having to change things.

Changes look good so moving back to RTBC.

Committed 13768e8 and pushed to 8.6.x. Thanks!

Publish the change record