Extract the ArgumentResolver system to its own component #59794

chalasr · 2025-02-17T16:35:01Z

Q	A
Branch?	7.3
Bug fix?	no
New feature?	yes
Deprecations?	yes
Issues	Helps with #53915
License	MIT

This PR extracts the Argument Resolver system to its own component, paving the way for Console Argument Value Resolvers.

What it does:

Make ArgumentResolverInterface and implementation + ValueResolverInterface context-agnostic and move them to a new ArgumentResolver component.
Move ArgumentMetadataFactory(Interface) and ArgumentMetadata to the component.
Add a Request-aware ControllerArgumentResolver in HttpKernel that extends the component one.
Move the main resolution logic from reusable value resolvers into traits. Concrete implementations use these traits and are responsible for retrieving the raw value (to-be-resolved) from their context-specific input source e.g. HTTP Request or Console Input.
Add a Request-aware ControllerValueResolverInterface that extends the component interface.
Deprecate existing value resolvers in favor of the new ones while making most deprecated value resolvers use the traits to avoid code duplication for their remaining time to live.

The main purpose of this is code reuse, with the goal of making console commands first-class citizens by providing them with the same capabilities that controllers have, as started in #59340.
Those console argument resolvers will come as a separate PR on top of this one (I want something working for my SymfonyDay Chicago's talk so it will happen soon).

Work in progress. Todo:

Agree on the design (early feedback welcome 🙏 )
Finalize BC layers and FrameworkBundle's wiring
Adapt tests

OskarStark · 2025-02-17T17:55:29Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

        $loader->load('web.php');
        $loader->load('services.php');
        $loader->load('fragment_renderer.php');
        $loader->load('error_renderer.php');

        if (!ContainerBuilder::willBeAvailable('symfony/clock', ClockInterface::class, ['symfony/framework-bundle'])) {
+            $container->removeDefinition('clock');


Suggested change

$container->removeDefinition('clock');

mtarld · 2025-02-18T08:14:16Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -418,6 +421,7 @@ public function load(array $configs, ContainerBuilder $container): void

            $this->registerSerializerConfiguration($config['serializer'], $container, $loader);
        } else {
+            // @deprecated since Symfony 7.3
            $container->getDefinition('argument_resolver.request_payload')


Maybe we can leverage setDeprecated as well?

mtarld · 2025-02-18T08:21:40Z

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

@@ -29,6 +29,7 @@
 * Responsible for resolving the arguments passed to an action.
 *
 * @author Iltar van der Berg <kjarli@gmail.com>
+ * @deprecated


Suggested change

* @deprecated

*

* @deprecated since Symfony 7.3, use "Symfony\Component\ArgumentResolver\ArgumentResolver" from the "symfony/argument-resolver" component instead

(same for others)

mtarld · 2025-02-18T08:25:39Z

src/Symfony/Component/ArgumentResolver/ValueResolver/Traits/BackedEnumValueResolverTrait.php

+ */
+trait BackedEnumValueResolverTrait
+{
+    private function doResolve(ArgumentMetadata $argument, array $rawValues): iterable


Suggested change

private function doResolve(ArgumentMetadata $argument, array $rawValues): iterable

/**

* @return iterable<null|\BackedEnum>

*/

private function doResolve(ArgumentMetadata $argument, array $rawValues): iterable

mtarld · 2025-02-18T08:27:00Z

src/Symfony/Component/ArgumentResolver/ValueResolver/Traits/BackedEnumValueResolverTrait.php

+
+        try {
+            return [$enumType::from($value)];
+        } catch (\ValueError|\TypeError $e) {


Suggested change

} catch (\ValueError|\TypeError $e) {

} catch (\ValueError $e) {

We've made sure that $value is either a string or an int before.

Being either a string or an int does not guarantee that you have no TypeError. An int-backed enum will trigger a TypeError when passing a non-numeric string.

Indeed, thanks!

mtarld · 2025-02-18T08:30:14Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/BackedEnumValueResolver.php

@@ -11,6 +11,8 @@

 namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

+use Symfony\Component\ArgumentResolver\Exception\InvalidRawValueException;


You need to add a dependency to symfony/argument-resolver

mtarld · 2025-02-18T08:53:09Z

src/Symfony/Component/ArgumentResolver/ValueResolver/Traits/UidValueResolverTrait.php

+        try {
+            return [$uidClass::fromString($value)];
+        } catch (\InvalidArgumentException $e) {
+            throw new InvalidRawValueException(\sprintf('The uid for the "%s" parameter is invalid.', $argument->getName()), $e);


Suggested change

throw new InvalidRawValueException(\sprintf('The uid for the "%s" parameter is invalid.', $argument->getName()), $e);

throw new InvalidRawValueException(\sprintf('The uid for the "%s" parameter is invalid.', $argument->getName()), $e->getCode(), $e);

This might mean that this path is not tested

mtarld · 2025-02-18T08:55:07Z

src/Symfony/Component/ArgumentResolver/ValueResolver/Traits/DateTimeValueResolverTrait.php

+        }
+
+        if ($value instanceof \DateTimeInterface) {
+            return [$value instanceof $class ? $value : $class::createFromInterface($value)];


createFromInterface doesn't belong to DateTimeInterface.

$class is never DateTimeInterface at that point. It is always a subclass of it. And subclassing DateTimeInterface requires extending DateTime or DateTimeImmutable due to restrictions of the language.

mtarld · 2025-02-18T08:58:06Z

src/Symfony/Component/ArgumentResolver/ArgumentResolver.php

+        $this->argumentValueResolvers = $argumentValueResolvers ?: static::getDefaultValueResolvers();
+    }
+
+    public function getArguments(mixed $input, callable $callable, ?\ReflectionFunctionAbstract $reflector = null): array


Suggested change

public function getArguments(mixed $input, callable $callable, ?\ReflectionFunctionAbstract $reflector = null): array

final public function getArguments(mixed $input, callable $callable, ?\ReflectionFunctionAbstract $reflector = null): array

If don't think we want this to be overridden.

For the ControllerArgumentResolver::getArguments, maybe we can create a new abstract protected getSubject (or any other name) method to fit with a template-method-like design pattern?

mtarld · 2025-02-18T09:10:15Z

src/Symfony/Component/HttpKernel/HttpKernel.php

+            ? $this->argumentResolver->getArguments($request, $controller, $event->getControllerReflector())
+            : $this->argumentResolver->getArguments($request, $controller, $event->getControllerReflector());


These are the same calls, is the ternary needed?

mtarld · 2025-02-18T09:11:16Z

src/Symfony/Component/HttpKernel/Tests/EventListener/ErrorListenerTest.php

@@ -21,6 +21,7 @@
 use Symfony\Component\HttpKernel\Attribute\WithHttpStatus;
 use Symfony\Component\HttpKernel\Attribute\WithLogLevel;
 use Symfony\Component\HttpKernel\Controller\ArgumentResolver;


This is not needed anymore

GromNaN

Useful decoupling.

Perhaps we can take this opportunity to update for complex type support using TypeInfo.

GromNaN · 2025-02-18T12:54:06Z

src/Symfony/Component/ArgumentResolver/ArgumentMetadata/ArgumentMetadata.php

+    /**
+     * Returns the type of the argument.
+     *
+     * The type is the PHP class in 5.5+ and additionally the basic type in PHP 7.0+.
+     */
+    public function getType(): ?string
+    {
+        return $this->type;
+    }


This comment can be updated, we require PHP 8.2.

The TypeInfo component seems appropriate to support union and intersection types here.

GromNaN · 2025-02-18T12:58:02Z

src/Symfony/Component/ArgumentResolver/ArgumentMetadata/ArgumentMetadata.php

+    /**
+     * @param class-string          $name
+     * @param self::IS_INSTANCEOF|0 $flags
+     *
+     * @return array<object>
+     */
+    public function getAttributes(?string $name = null, int $flags = 0): array
+    {
+        if (!$name) {
+            return $this->attributes;
+        }
+
+        return $this->getAttributesOfType($name, $flags);
+    }


Either we make the getAttributesOfType method private, or we remove the arguments of this one. Having 2 methods that get the same arguments and return the same result is not ideal.

Suggested change

/**

* @param class-string $name

* @param self::IS_INSTANCEOF|0 $flags

*

* @return array<object>

*/

public function getAttributes(?string $name = null, int $flags = 0): array

{

if (!$name) {

return $this->attributes;

}

return $this->getAttributesOfType($name, $flags);

}

/**

* @return array<object>

*/

public function getAttributes(): array

{

return $this->attributes;

}

The backward-compatibility layer can be kept and deprecated in the deprecated child class.

GromNaN · 2025-02-18T12:59:41Z

src/Symfony/Component/ArgumentResolver/ArgumentMetadata/ArgumentMetadata.php

+ *
+ * @final
+ */
+class ArgumentMetadata


I understand that this class is not actually final because it is extended by the Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata class that is deprecated.

I will comment that

stof · 2025-02-18T14:11:13Z

src/Symfony/Component/ArgumentResolver/ValueResolver/ValueResolverInterface.php

+    /**
+     * Returns the resolved argument value(s).
+     */
+    public function resolve(ArgumentMetadata $argument): iterable;


This interface looks weird to me, as it does not have the source from which the data should be read to resolve it, which is totally different What is the expected usage of the component ?

Btw, the fact that the component does not contain any implementation of that interface except the one reading the default value of the callable parameter is a hint that this interface is not enough on its own.

The main expected usage is mostly code reuse for now. I started with resolve(ArgumentMetadata $argument, mixed $input): iterable; but it makes impossible to narrow mixed to Request in the child ControllerValueResolverInterface where Request is an optional argument currently

Then, making the input source an implementation detail doesn't feel irrelevant to me

Regarding code reuse, here are the stats for the core resolvers:

1 resolver is fully moved to ArgumentResolver (the DefaultValueResolver, which is dead simple)

5 resolvers are sharing code using reusable traits. Among them, the VariadicValueResolver is broken, and fixing it would leave nothing to share in the trait (moving it to the next category).

7 resolvers are not sharing any code

And this PR does not migrate the EntityValueResolver in the doctrine-bridge (which is bad IMO as it needs to be part of the work), but I suspect it would also end up in the category not reusing code.
I'm not sure the reusability for a few resolver (which are the most simple ones, and so the ones for which code duplication would be the less an issue from a maintenance point of view) justify making a BC break in the ecosystem with a replacement interface that is more complex to implement properly (by having to handle the case of a null request).

EntityValueResolver's logic is actually meant to be moved to the component as well since it is relevant for the Console and its coupling to the Request object is very low, as for other resolvers.
In addition to traits, the ArgumentResolver implementation which is a significant piece of code, ArgumentMetadata, ArgumentMetadataFactoryInterface + implementation and all argument-resolver related exception classes are also moved and made reusable. Duplicating all this code in Console would mean way too much to me, especially considering the fact that the added interface is actually useful on its own.

About BC, this change is transparent for most end applications. Only apps with custom value resolvers will feel it and this only requires them to implement the new interface instead of the legacy one + adjust one parameter in their signature. Again the duplication this change saves makes it worth it to me

EntityValueResolver could also stay in DoctrineBridge, either as a single class supporting both Console Input and Request, or two implementation + a trait.

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/BackedEnumValueResolver.php

stof · 2025-02-18T14:15:27Z

...ony/Component/HttpKernel/Controller/ArgumentResolver/ValueResolver/DateTimeValueResolver.php

+
+    public function resolve(ArgumentMetadata $argument, ?Request $request = null): iterable
+    {
+        if (!$request) {


This new interface forcing all implementations to handle the case of a null request looks like a strictly inferior solution compared to the existing feature (and I don't see the benefit for usage in the console component, as ControllerValueResolverInterface won't be reused in the console, so we could simply implement a similar system in symfony/console without changing the system of HttpKernel)

While it's not ideal, I don't think it's a big deal. This could even be addressed by replacing the instanceof check by phpdoc, which I think I'm going to do.

stof · 2025-02-18T14:28:34Z

src/Symfony/Component/ArgumentResolver/ValueResolver/Traits/VariadicValueResolverTrait.php

+            return [];
+        }
+
+        return $rawValues;


This implementation looks broken to me. A variadic argument will not select all request attributes

The class using this trait does not pass all request attributes but the one that matches the argument name

stof · 2025-02-18T14:30:28Z

...ony/Component/HttpKernel/Controller/ArgumentResolver/ValueResolver/VariadicValueResolver.php

+
+        $value = $request->attributes->get($argument->getName());
+
+        if (!\is_array($value)) {


this implementation is broken. You throw this exception for a non-array argument before checking whether the argument is variadic or no (the existing resolver skips non-variadic arguments early)

stof · 2025-02-18T14:32:41Z

src/Symfony/Component/ArgumentResolver/ValueResolver/Traits/UidValueResolverTrait.php

+{
+    private function doResolve(ArgumentMetadata $argument, array $rawValues): iterable
+    {
+        $value = $rawValues[0] ?? null;


Why expecting to get an array with a 0 key here ? there is no benefit passing an array (it does not even simplify the class using the trait, as it creates the array specially for that)

that's a leftover from a previous inheritance-based implementation of this PR, I will change it for a single value 👍

stof · 2025-02-18T14:34:33Z

src/Symfony/Component/ArgumentResolver/ValueResolver/Traits/DateTimeValueResolverTrait.php

+        }
+
+        if ($value instanceof \DateTimeInterface) {
+            return [$value instanceof $class ? $value : $class::createFromInterface($value)];


$class is never DateTimeInterface at that point. It is always a subclass of it. And subclassing DateTimeInterface requires extending DateTime or DateTimeImmutable due to restrictions of the language.

stof · 2025-02-18T14:43:39Z

src/Symfony/Component/ArgumentResolver/ValueResolver/ValueResolverInterface.php

+    /**
+     * Returns the resolved argument value(s).
+     */
+    public function resolve(ArgumentMetadata $argument): iterable;


Regarding code reuse, here are the stats for the core resolvers:

1 resolver is fully moved to ArgumentResolver (the DefaultValueResolver, which is dead simple)

5 resolvers are sharing code using reusable traits. Among them, the VariadicValueResolver is broken, and fixing it would leave nothing to share in the trait (moving it to the next category).

7 resolvers are not sharing any code

And this PR does not migrate the EntityValueResolver in the doctrine-bridge (which is bad IMO as it needs to be part of the work), but I suspect it would also end up in the category not reusing code.
I'm not sure the reusability for a few resolver (which are the most simple ones, and so the ones for which code duplication would be the less an issue from a maintenance point of view) justify making a BC break in the ecosystem with a replacement interface that is more complex to implement properly (by having to handle the case of a null request).

smnandre · 2025-02-22T05:03:11Z

src/Symfony/Component/ArgumentResolver/ArgumentResolver.php

+use Symfony\Component\ArgumentResolver\ValueResolver\ValueResolverInterface;
+
+/**
+ * Responsible for resolving the arguments passed to an action.


Suggested change

* Responsible for resolving the arguments passed to an action.

* Responsible for resolving the arguments passed to an callable.

?

smnandre · 2025-04-09T16:06:57Z

Do you need (or would accept) any help here?

This is something we'd love to use in LiveComponent 👼

OskarStark · 2025-04-10T11:27:50Z

Ups 🙈

chalasr · 2025-04-11T12:04:27Z

Thanks for the pings, I'll get back to work on this (and reach out to you @smnandre) asap.
Doing my best to cope with my lifelong OSS todo list in this work (and trips) intensive period :).

Note that this is too big and not ready enough to make it into 7.3 at this stage, so we have 6 months to move forward with 7.4 as target.

carsonbot added Status: Needs Review Deprecation Feature labels Feb 17, 2025

carsonbot added this to the 7.3 milestone Feb 17, 2025

chalasr force-pushed the argument-resolver branch from 6f49b5a to e891ad9 Compare February 17, 2025 16:39

OskarStark reviewed Feb 17, 2025

View reviewed changes

mtarld reviewed Feb 18, 2025

View reviewed changes

GromNaN reviewed Feb 18, 2025

View reviewed changes

stof reviewed Feb 18, 2025

View reviewed changes

smnandre reviewed Feb 22, 2025

View reviewed changes

chalasr added 3 commits March 22, 2025 18:22

Adding the ArgumentResolver component

fbfae9a

Rework

d29f355

Experimenting ways to abstract the input source

f28fb52

chalasr force-pushed the argument-resolver branch from e891ad9 to f28fb52 Compare March 23, 2025 18:19

OskarStark closed this Apr 10, 2025

OskarStark reopened this Apr 10, 2025

	* @deprecated
	*
	* @deprecated since Symfony 7.3, use "Symfony\Component\ArgumentResolver\ArgumentResolver" from the "symfony/argument-resolver" component instead

	} catch (\ValueError\|\TypeError $e) {
	} catch (\ValueError $e) {

		@@ -11,6 +11,8 @@

		namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

		use Symfony\Component\ArgumentResolver\Exception\InvalidRawValueException;

	throw new InvalidRawValueException(\sprintf('The uid for the "%s" parameter is invalid.', $argument->getName()), $e);
	throw new InvalidRawValueException(\sprintf('The uid for the "%s" parameter is invalid.', $argument->getName()), $e->getCode(), $e);

	public function getArguments(mixed $input, callable $callable, ?\ReflectionFunctionAbstract $reflector = null): array
	final public function getArguments(mixed $input, callable $callable, ?\ReflectionFunctionAbstract $reflector = null): array

		? $this->argumentResolver->getArguments($request, $controller, $event->getControllerReflector())
		: $this->argumentResolver->getArguments($request, $controller, $event->getControllerReflector());


		$value = $request->attributes->get($argument->getName());

		if (!\is_array($value)) {

	* Responsible for resolving the arguments passed to an action.
	* Responsible for resolving the arguments passed to an callable.

Extract the ArgumentResolver system to its own component #59794

Are you sure you want to change the base?

Extract the ArgumentResolver system to its own component #59794

Conversation

chalasr commented Feb 17, 2025 • edited Loading

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

Choose a reason for hiding this comment

stof Feb 18, 2025 • edited Loading

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

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

GromNaN left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

chalasr Feb 18, 2025 • edited Loading

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

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

Choose a reason for hiding this comment

smnandre commented Apr 9, 2025

OskarStark commented Apr 10, 2025

chalasr commented Apr 11, 2025 • edited Loading

chalasr commented Feb 17, 2025 •

edited

Loading

stof Feb 18, 2025 •

edited

Loading

chalasr Feb 18, 2025 •

edited

Loading

chalasr Feb 18, 2025 •

edited

Loading

chalasr Feb 18, 2025 •

edited

Loading

chalasr commented Apr 11, 2025 •

edited

Loading