Skip to content

Extract the ArgumentResolver system to its own component #59794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: 7.3
Choose a base branch
from

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Feb 17, 2025

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

$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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$container->removeDefinition('clock');

@@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can leverage setDeprecated as well?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(same for others)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private function doResolve(ArgumentMetadata $argument, array $rawValues): iterable
/**
* @return iterable<null|\BackedEnum>
*/
private function doResolve(ArgumentMetadata $argument, array $rawValues): iterable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

@stof stof Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, thanks!

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

namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

use Symfony\Component\ArgumentResolver\Exception\InvalidRawValueException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if ($value instanceof \DateTimeInterface) {
return [$value instanceof $class ? $value : $class::createFromInterface($value)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createFromInterface doesn't belong to DateTimeInterface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$this->argumentValueResolvers = $argumentValueResolvers ?: static::getDefaultValueResolvers();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +177 to +178
? $this->argumentResolver->getArguments($request, $controller, $event->getControllerReflector())
: $this->argumentResolver->getArguments($request, $controller, $event->getControllerReflector());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the same calls, is the ternary needed?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed anymore

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful decoupling.

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

Comment on lines +49 to +57
/**
* 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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment can be updated, we require PHP 8.2.


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

Comment on lines +99 to +112
/**
* @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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +18 to +21
*
* @final
*/
class ArgumentMetadata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will comment that

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@chalasr chalasr Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@chalasr chalasr Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@chalasr chalasr Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return [];
}

return $rawValues;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

if (!\is_array($value)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if ($value instanceof \DateTimeInterface) {
return [$value instanceof $class ? $value : $class::createFromInterface($value)];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

use Symfony\Component\ArgumentResolver\ValueResolver\ValueResolverInterface;

/**
* Responsible for resolving the arguments passed to an action.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

?

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

smnandre commented Apr 9, 2025

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

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

@OskarStark OskarStark closed this Apr 10, 2025
@OskarStark OskarStark reopened this Apr 10, 2025
@OskarStark
Copy link
Contributor

Ups 🙈

@chalasr
Copy link
Member Author

chalasr commented Apr 11, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants