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

[RFC][HttpKernel] Configurable argument value resolvers #29692

Open
vudaltsov opened this Issue Dec 26, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@vudaltsov
Copy link
Contributor

vudaltsov commented Dec 26, 2018

Hi everyone!

Recently got interested in @ParamConverters as a convenient extension point in Symfony. Some time ago in sensiolabs/SensioFrameworkExtraBundle#436 @fabpot proposed an idea on how to implement configurable argument resolvers. Let me explain the idea:

  • A single annotation @Arg(string $name, array options = []) is proposed as a replacement for @ParamConverter.
  • A new interface ArgumentValueOptionsInterface extends ArgumentValueResolverInterface adds configure(OptionsResolver $resolver) method.
  • A new subscriber ControllerArgumentOptionsListener listens to the KernelEvents::CONTROLLER event and looks for @Arg mappings for the current controller callable. Then it iterates over the ArgumentValueOptionsInterface[] instances to find a resolver based on supports(Request $request, ArgumentMetadata $argument). Once the resolver is found, @Arg.options are resolved and saved to $request->attributes->set($name.'_options', $resolvedOptions).
  • Later in SomeArgumentValueResolver::resolve(Request $request, ArgumentMetadata $argument) we receive resolved options by calling $request->attributes->get($name.'_options', []) and use them to yield the value.

While I think that this is a nice solution (especially when looking from the perspective of the FrameworkExtraBundle), it has one important drawback. @ParamConverter has a possibility to specify converter explicitly by leveraging the converter argument. It allows to resolve arguments which by themselves do not provide enough information to find a resolver. A great example is @ParamConverter("name", converter="fos_rest.request_body") from FOSRestBundle. This converter allows to deserialize $request->getContent() into a DTO based on the argument type hint. Unlike SomeEntityClass which we can check with ManagerRegistry::getManagerForClass(), SomeDTOClass does not give any hint that it should be processed by a RequestBodyParamConverter. So, the @Arg concept without a possibility to be explicit about the resolver seems to be not suitable for this case.

I have a different idea. It think, it fits the HttpKernel much better than the bundle, that's why I am creating an issue here.

Btw, the idea to move @ParamConverters configuration to the core was already discussed in sensiolabs/SensioFrameworkExtraBundle#436 (comment).

Design

  1. Let's agree that once the argument is explicitly configured (for example by an annotation), we already know what resolver it should be processed by. It's like with validator constraints. So I propose to create the following interface:

    interface ArgumentConfigInterface
    {
        /**
         * Returns the FQCN of the argument value resolver that resolves the argument with this config.
         */
        public function resolvedBy(): string;
    }
  2. I think that within the HttpKernel component this config might be a part of argument's metadata. Later it will allow us to cache everything together easily. Thus let's add another property to the ArgumentMetadata:

    namespace Symfony\Component\HttpKernel\ControllerMetadata;
    
    class ArgumentMetadata
    {
        // ...
    
        private $config;
    
        public function __construct(
            string $name,
            ?string $type,
            bool $isVariadic,
            bool $hasDefaultValue,
            $defaultValue,
            bool $isNullable = false,
            ?ArgumentConfigInterface $config = null
        ) {
           // ...
    
           $this->config = $config;
        }
    
        // ...
    
        public function getConfig(): ?ArgumentConfigInterface
        {
            return $this->config;
        }
    }
  3. Should we stick to the annotation mapping? No, let's create an extension point!

    interface ArgumentConfigLoaderInterface
    {
        public function getArgumentConfig(callable $controller, string $name): ?ArgumentConfigInterface;
    }
    
    final class AnnotationArgumentConfigLoader implements ArgumentConfigLoaderInterface
    {
        public function __construct(Doctrine\Common\Annotations\Reader $reader)
        {
            // ...
        }
    }

    Now we can inject ArgumentConfigLoaderInterface into ArgumentMetadataFactory and produce config aware ArgumentMetadata instances.

  4. Next, let's adjust ArgumentResolver. Previously we retrieved converters only by iteration, now we want to get them by FQCN as well.

    namespace Symfony\Component\HttpKernel\Controller;
    
    final class ArgumentResolver implements ArgumentResolverInterface
    {
        // ...
    
        private $argumentValueResolversByClass;
    
        public function __construct(
            // ...
            ?ContainerInterface $argumentValueResolversByClass = null
        ) {
            // ...
            $this->argumentValueResolversByClass = $argumentValueResolversByClass ?: self::getDefaultArgumentValueResolversByClass();
        }
    
        // ...
    
        private function getResolver(Request $request, ArgumentMetadata $metadata): ArgumentValueResolverInterface
        {
            if (null !== $config = $metadata->getConfig()) {
                return $this->argumentValueResolversByClass->get($config->resolvedBy());
            }
    
            foreach ($this->argumentValueResolvers as $resolver) {
                if ($resolver->supports($request, $metadata)) {
                    return $resolver;
                }
            }
    
            throw new \RuntimeException();
        }
    }
  5. I think we should also cache all argument metadata. That can be easily done with a decorator CachingArgumentMetadataFactory implements ArgumentMetadataFactoryInterface.

After all these non-breaking BC steps we'll be able to create configs like @Entity, @BodyConverter, @GetParam with ease.

Benefits

  • Custom configs for argument value resolvers.
  • Faster argument value resolver resolution for configured arguments.
  • One point to cache everything.
  • Out-of-the-box improved @ParamConverters with no need to require sensio/framework-extra-bundle.

Ready to discuss!
ping @fabpot, @iltar.

@javiereguiluz javiereguiluz changed the title [WIP][HttpKernel] Configurable argument value resolvers [RFC][HttpKernel] Configurable argument value resolvers Dec 27, 2018

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Dec 27, 2018

Valentin, thanks for proposing this improvement. In order to have more arguments to decide in favor or against this proposal, we'd need to also know the "drawbacks". You only listed the "benefits".

Maybe it doesn't have any important drawback, but please double check it for things like: performance impact, app upgrade pain, incompatibilities with other Symfony features, etc.

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Dec 28, 2018

@javiereguiluz , thanx for editing the title, now I know the difference between WIP and RFC 😂

Performance impact

It's not going to be slower than @ParamConverters and will be definitely faster if we add caching for ArgumentMetadata

Upgrade

My suggestion is to implement this in HttpKernel, so it's a new feature for Symfony, not for FrameworkExtraBundle. It might be disabled by default in the FrameworkBundle config. After we implement @Entity in DoctrineBridge, one can just change the annotation and switch configs.

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Dec 28, 2018

In your ArgumentConfigInterface you have defined a resolvedBy method. This is then called as return $this->argumentValueResolversByClass->get($config->resolvedBy());. What's the idea you have behind the value resolvedBy returns? Will this be a custom class you implement? Do you have an example of this?

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Dec 29, 2018

@iltar , the ArgumentConfigInterface::resolvedBy method returns the string class name of the exact argument value resolver.

Consider a controller:

use App\Entity\Product;

final class ProductController
{
    /**
     * @Route("/product/{id<\d+>}")
     */
    public function __invoke(Product $product): Response
    {
    }
}

In this case we don't know what Product $product argument really is. That is why we ask each ArgumentValueResolverInterface whether it supports Product $product or not. When EntityValueResolver says "yes, I know how to handle this" by returning true in the supports method, we call $resolver->resolve().

use App\Entity\Product;

final class ProductController
{
    /**
     * @Route("/product/{id<\d+>}")
     * @Entity("product", expr="repository.findById(id)")
     */
    public function __invoke(Product $product): Response
    {
    }
}

In this case we don't need to iterate over value resolvers. What for? We already know that $product should be handled by EntityValueResolver, because that's what @Entity annotation means. We just fetch this resolver by name obtained from ArgumentConfigInterface::resolvedBy and call resolve. As I said earlier, when mapping is present, it's like with validator constraints: nothing to guess.

This mechanism will also solve a common @Entity User implements UserInterface argument problem. Once we put @CurrentUser("user") annotation, the resolver will be Symfony\Component\Security\Http\Controller\UserValueResolver and not EntityValueResolver.

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Dec 29, 2018

I can make an example of my idea in a simple website-skeleton project with some HttpKernel services replaced.

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Dec 29, 2018

So if I understand you correctly (and to formulate the feature in a short sentence), you want to pre-bind a resolver to a specific variable name based on config (which can be annotations, xml, yaml etc).

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Dec 29, 2018

Yes, exactly. What do you think?

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Dec 29, 2018

I think it's a great idea, as you probably already figured from the other issue you linked.

@curry684

This comment has been minimized.

Copy link
Contributor

curry684 commented Jan 4, 2019

Never understood why this wasn't native functionality to begin with, it's awesome once you start using and extending the mechanisms.

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Jan 4, 2019

It looks like a lot of people are interested in this and everyone is positive about the implementation idea. Should I start working on a PR?

@adrenalinkin

This comment has been minimized.

Copy link

adrenalinkin commented Jan 13, 2019

Hello everyone! Past year i was facing the problem which looks same to described but relates to Swagger Documentation.
I tried to find the way to validate request data according to the generated documentation. But i did not found any realization for that.
So I started to realize solution for my purpose. When basic possibility has been achievement by swagger-resolver-bundle and validation has been provided I am did something very same to described behavior like in the current PR (realization example).

So I thought about resolving any Request (not only described by swagger doc). And @vudaltsov want to offer something like that by current PR.

I vote YES! And open to help with realization if that will be need.

P.S.: In my opinion we should not add annotation to the controller but to the concrete DTO. Because anyway we need to resolve data from Request into concrete object(s).

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