Skip to content
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

[HttpKernel] Adds @QueryParam annotation #36135

Open
wants to merge 7 commits into
base: master
from

Conversation

@tsantos84
Copy link
Contributor

tsantos84 commented Mar 18, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? /no
Tickets -
License MIT
Doc PR -

As discussed on #36037, this PR adds the annotation @QueryParam to map query string in controller arguments. In addition, it allows to validate parameters using Symfony Validator and throw BadRequestException automatically in case of any constraint violation. In practical terms, this PR allows that a code like this:

/**
 * @Route("/search")
 */
function searchAction(Request $request)
{
    $term = $request->query->get('term');
    if (null === $term || length($term) < 3) {
        throw new HttpException(400);
    }
    $limit = $request->query->getInt('limit', 10);
    // perform search with $term and $limit
}

Can be written like this:

/**
 * @Route("/search")
 *
 * @QueryParam("term", constraints={@Assert\Length(min=3)})
 * @QueryParam("limit")
 */
function searchAction(string $term, int $limit = 10)
{
    // perform search with $term and $limit
}

Assumptions

Some assumptions were made until now and can be changed during the development:

  1. Add this feature to Symfony HttpKernel component as it has already classes to deal with controllers and argument resolving.
  2. Validating query strings with Symfony Validator requires add manually symfony/validator package to your composer.json

Doubts

  1. Should we consider to allow configure query params through yml/xml configuration as well?

Future

  • This would be the first movement to bring some features presented in SensioExtraBundle to Symfony's Core and weaken the dependency of applications with that bundle.
  • As soon as this PR gets merged (if it passed, of course), we can start working on another annotations like @RequestBody, @RequestCookie and etc.
  • If PHP annotations RFC gets approved, this PR will reduce the amount of work to port annotations from docblock to native ones.
@tsantos84 tsantos84 force-pushed the tsantos84:query-param-annotation branch from 348a03f to 1257690 Mar 19, 2020
@nicolas-grekas nicolas-grekas added this to the next milestone Mar 19, 2020
Copy link
Member

nicolas-grekas left a comment

I see a lot in common with the framework-extra-bundle here, which is nice, but can't we do some extra miles and replace the runtime logic in ControllerListener by a compile-time one?

This would require using a tag to identify controllers. We could reuse controller.service_arguments or add a new kernel.controller (that could maybe replace controller.service_arguments btw)

WDYT?

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

tsantos84 commented Mar 19, 2020

I see a lot in common with the framework-extra-bundle here, which is nice, but can't we do some extra miles and replace the runtime logic in ControllerListener by a compile-time one?

This would require using a tag to identify controllers. We could reuse controller.service_arguments or add a new kernel.controller (that could maybe replace controller.service_arguments btw)

WDYT?

Let me see if I understand. What you're proposing is a way to fetch data from a compiled container instead of a dedicated ArgumentValueResolver and the value would be resolved by ServiceValueResolver or something like that.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 19, 2020

Yes, something like that :)

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Mar 20, 2020

I would recommend a different tag name, I'm not using service arguments for my controllers, and I'd still like to use this feature 😄

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

tsantos84 commented Apr 4, 2020

Hi @nicolas-grekas, I've thought a lot how could we leverage the compilation mechanism to go ahead with this feature. One possible solution would be make the generated container implement ArgumentValueResolverInterface interface and put some generated logic there. Essentially we would have:

1. Container implementing ArgumentValueResolverInterface:

class appDevDebugProjectContainer extends Container implements ArgumentValueResolverInterface 
{
    private static $argumentResolvers = [
        'SomeController::IndexAction:Logger' => 'resolveSomeControllerIndexActionLoggerArgument.php'
    ];
  
   public function supports($request, $argumentMetadata) {
       $controllerName = ...;
       return isset(self::$argumentResolvers[$controllerName]);
   }

   public function resolve($request, $argumentMetadata) {
        $controllerName = ...;
        yield $this->load(self::$argumentResolvers[$controllerName]);
   }
}

2. Dedicated file resolver to each controller argument

This file could be located inside the generated container directory.

// var/cache/prod/ContainerOOVkbxl/resolveSomeControllerIndexActionLoggerArgument.php
return $this->get(LoggerInterface::class);

3. Register the container in the argument value resolver list


Honestly I can't see another way to implement this feature leveraging the compilation system much different from this. If you still see a better way to go, I'm totally opened and curious to hear you. :) Otherwise, we can keep the logic in a listener and do the job there. WDYT?

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

tsantos84 commented Apr 4, 2020

Thinking about tags for controllers, what if create a compile pass to read controllers from routing config? We wouldn't need to create a new tag nor use the existing one. Actually, the existing one could be deprecated.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 4, 2020

There's a possible link between this PR and #36320

About parsing route during compilation, that's orthogonal to the way things are designed today: routes are loaded after the container is compiled. Not sure it's worth pursuing.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.