[HttpKernel] Introduce pinnable value resolvers with #[ValueResolver] and #[AsPinnedValueResolver]

[MatTheCat]

Q	A
Branch?	6.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #48927
License	MIT
Doc PR	symfony/symfony-docs#17763

Introducing a new ValueResolver attribute, which allows to

• “pin” a value resolver to an argument, meaning only said resolver will be called
• prevent a resolver to be called for an argument

Every existing resolver-related attribute (MapEntity, CurrentUser…) now extends ValueResolver.

Each controller.argument_value_resolver tag is added a name attribute, which is the resolver’s FQCN. This is the first argument ValueResolver expects.

A new AsPinnedValueResolver attribute is added for autoconfiguration, adding the controller.pinned_value_resolver tag. Such resolvers can only be “pinned”, meaning they won’t ever be called for an argument missing the ValueResolver attribute.

[chalasr]

The feature makes sense to me

[nicolas-grekas]

For param converters we have this behavior:

You can register a converter by priority, by name (attribute "converter"), or both. If you don't specify a priority or a name, the converter will be added to the converter stack with a priority of 0. To explicitly disable the registration by priority you have to set priority="false" in your tag definition.

Do we want a way to not register a value resolver in the iterator and make it accessible only via explicit attribute? That's require accepting both an iterator and a locator on the constructor.

[MatTheCat]

Thanks for the review!

Do we want a way to not register a value resolver in the iterator and make it accessible only via explicit attribute?

From what I understood, we only want a way to force a specific resolver to be used for an argument.

You can register a converter […] by name

Maybe we could do the same? This would require to name existing resolvers.

[nicolas-grekas]

Here is my review as a diff proposal:

diff --git a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
index 3ce8519c26..ab318d1334 100644
--- a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
+++ b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
@@ -11,7 +11,6 @@
 
 namespace Symfony\Component\HttpKernel\Controller;
 
-use Symfony\Component\DependencyInjection\ServiceLocator;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Attribute\ValueResolver;
 use Symfony\Component\HttpKernel\Controller\ArgumentResolver\DefaultValueResolver;
@@ -31,34 +30,39 @@ use Symfony\Contracts\Service\ServiceProviderInterface;
  */
 final class ArgumentResolver implements ArgumentResolverInterface
 {
-    private ArgumentMetadataFactoryInterface $argumentMetadataFactory;
-    private ServiceProviderInterface $argumentValueResolvers;
-
     /**
      * @param iterable<mixed, ArgumentValueResolverInterface|ValueResolverInterface>|ServiceProviderInterface<ValueResolverInterface> $argumentValueResolvers
      */
-    public function __construct(ArgumentMetadataFactoryInterface $argumentMetadataFactory = null, iterable|ServiceProviderInterface $argumentValueResolvers = [])
-    {
-        $this->argumentMetadataFactory = $argumentMetadataFactory ?? new ArgumentMetadataFactory();
-        $this->argumentValueResolvers = $argumentValueResolvers instanceof ServiceProviderInterface
-            ? $argumentValueResolvers
-            : self::getServiceProvider($argumentValueResolvers ?: self::getDefaultArgumentValueResolvers())
-        ;
+    public function __construct(
+        private ArgumentMetadataFactoryInterface|null $argumentMetadataFactory = new ArgumentMetadataFactory(),
+        private iterable|ServiceProviderInterface $argumentValueResolvers = [],
+    ) {
+        if (null === $argumentMetadataFactory) {
+            $this->argumentMetadataFactory = new ArgumentMetadataFactory();
+            trigger_deprecation('symfony/http-kernel', '6.3', 'Passing null as $argumentMetadataFactory to "%s" is deprecated. Pass a "%s" instead.', static::class, ArgumentMetadataFactory::class);
+        }
     }
 
     public function getArguments(Request $request, callable $controller, \ReflectionFunctionAbstract $reflector = null): array
     {
         $arguments = [];
-        $resolversName = array_keys($this->argumentValueResolvers->getProvidedServices());
+        $resolversMap = null;
+        $allResolvers = $this->argumentValueResolvers ?: self::getDefaultArgumentValueResolvers();
+        $isProvider = $allResolvers instanceof ServiceProviderInterface;
 
         foreach ($this->argumentMetadataFactory->createArgumentMetadata($controller, $reflector) as $metadata) {
-            $argumentResolversName = $resolversName;
+
             if ($attributes = $metadata->getAttributesOfType(ValueResolver::class)) {
-                $argumentResolversName = [$attributes[0]->name];
+                $name = $attributes[0]->name;
+                $resolvers = [$name => $isProvider ? null : ($resolversMap ??= self::getResolversMap($allResolvers))[$name]];
+            } else {
+                $resolvers = $isProvider ? $allResolvers->getProvidedServices() : $allResolvers;
             }
 
-            foreach ($argumentResolversName as $resolverName) {
-                $resolver = $this->argumentValueResolvers->get($resolverName);
+            foreach ($resolvers as $name => $resolver) {
+                if ($isProvider) {
+                    $resolver = $allResolvers->get($name);
+                }
 
                 if ((!$resolver instanceof ValueResolverInterface || $resolver instanceof TraceableValueResolver) && !$resolver->supports($request, $metadata)) {
                     continue;
@@ -115,16 +119,16 @@ final class ArgumentResolver implements ArgumentResolverInterface
     /**
      * @param iterable<ArgumentValueResolverInterface|ValueResolverInterface> $resolvers
      *
-     * @return ServiceProviderInterface<ArgumentValueResolverInterface|ValueResolverInterface>
+     * @return array<string, ArgumentValueResolverInterface|ValueResolverInterface>
      */
-    private static function getServiceProvider(iterable $resolvers): ServiceProviderInterface
+    private static function getResolversMap(iterable $resolvers): array
     {
-        $factories = [];
+        $map = [];
         $i = 0;
         foreach ($resolvers as $name => $resolver) {
-            $factories[$name === $i++ ? $resolver::class : $name] = static fn () => $resolver;
+            $map[$name === $i++ ? $resolver::class : $name] = $resolver;
         }
 
-        return new ServiceLocator($factories);
+        return $map;
     }
 }

From what I understood, we only want a way to force a specific resolver to be used for an argument.

Sure, but this is unrelated to my question: do we want a way to make a value resolver accessible only via the attribute? The extra bundle allows this. Do we also want that capability? I don't have an answer yet but I'm still wondering.

[MatTheCat]

Hmm the ServiceLocatorTagPass is sorting services by key, thus overriding the priority order (from #22175) 🤔

Is there a way to circumvent this behavior?

do we want a way to make a value resolver accessible only via the attribute?

I personally never had any use-case for this so I cannot say.

[stof]

a ServiceLocator does not guarantee iteration order. To me, this means you should inject 2 different arguments:

• an iterator sorted by priority
• a service locator keyed by name and used only for this ValueResolver argument.

[stof]

An alternative way is to make this ValueResolver be processed by a ValueResolver registered first (with a very high priority) and receiving this ServiceLocator instead of making it a special feature of the ArgumentResolver.

[MatTheCat]

Thanks @stof for the leads 👍

Is there any incentive for implementing one rather than the other?

[nicolas-grekas]

I like @stof's idea! Less low level complexity, each class their use case.

[MatTheCat]

Okay, going with a dedicated value resolver.

[nicolas-grekas]

/cc @symfony/mergers I'd like to merge this to unlock some other PRs. Could you please have a look? 🙏

[lyrixx]

👍🏼 for the feature

[nicolas-grekas]

Thank you @MatTheCat.