Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[HttpKernel] Adds @QueryParam annotation#36135

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
tsantos84 wants to merge7 commits intosymfony:masterfromtsantos84:query-param-annotation
Closed

[HttpKernel] Adds @QueryParam annotation#36135

tsantos84 wants to merge7 commits intosymfony:masterfromtsantos84:query-param-annotation

Conversation

@tsantos84
Copy link
Contributor

@tsantos84tsantos84 commentedMar 18, 2020
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?/no
Tickets-
LicenseMIT
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 usingSymfony Validator and throwBadRequestException automatically in case of any constraint violation. In practical terms, this PR allows that a code like this:

/** * @Route("/search") */functionsearchAction(Request$request){$term =$request->query->get('term');if (null ===$term ||length($term) <3) {thrownewHttpException(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") */functionsearchAction(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 toSymfony HttpKernel component as it has already classes to deal with controllers and argument resolving.
  2. Validating query strings withSymfony Validator requires add manuallysymfony/validator package to yourcomposer.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.
  • IfPHP annotations RFC gets approved, this PR will reduce the amount of work to port annotations from docblock to native ones.

maidmaid, b1rdex, kaznovac, renan-taranto, rogierknoester, and Koc reacted with thumbs up emojilinaori and kaznovac reacted with rocket emoji
@nicolas-grekasnicolas-grekas added this to thenext milestoneMar 19, 2020
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

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 inControllerListener by a compile-time one?

This would require using a tag to identify controllers. We could reusecontroller.service_arguments or add a newkernel.controller (that could maybe replacecontroller.service_arguments btw)

WDYT?

linaori reacted with thumbs up emoji
@tsantos84
Copy link
ContributorAuthor

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 inControllerListener by a compile-time one?

This would require using a tag to identify controllers. We could reusecontroller.service_arguments or add a newkernel.controller (that could maybe replacecontroller.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 dedicatedArgumentValueResolver and the value would be resolved byServiceValueResolver or something like that.

@nicolas-grekas
Copy link
Member

Yes, something like that :)

@linaori
Copy link
Contributor

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
Copy link
ContributorAuthor

tsantos84 commentedApr 4, 2020
edited
Loading

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 implementArgumentValueResolverInterface interface and put some generated logic there. Essentially we would have:

1. Container implementingArgumentValueResolverInterface:

class appDevDebugProjectContainerextends Containerimplements ArgumentValueResolverInterface {privatestatic$argumentResolvers = ['SomeController::IndexAction:Logger' =>'resolveSomeControllerIndexActionLoggerArgument.php'    ];publicfunctionsupports($request,$argumentMetadata) {$controllerName =...;returnisset(self::$argumentResolvers[$controllerName]);   }publicfunctionresolve($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.phpreturn$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
Copy link
ContributorAuthor

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
Copy link
Member

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 loadedafter the container is compiled. Not sure it's worth pursuing.

@tsantos84
Copy link
ContributorAuthor

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

Ok and agree considering the Symfony's design. My opinion about this is that the framework - in a general aspect - already knows what controllers are configured and devs wouldn't need to configure it again through tags. But this subject is not part of this PR and I don't want to discuss here to not change the main scope of this feature.

About the implementation of this feature, which way should we go?@nicolas-grekas, could you readthis and explain if this is what you thoughthere?

@piku235
Copy link
Contributor

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 inControllerListener by a compile-time one?

This would require using a tag to identify controllers. We could reusecontroller.service_arguments or add a newkernel.controller (that could maybe replacecontroller.service_arguments btw)

WDYT?

I've managed with dropping the annotation controller listener and extracting it to the compiler pass. I decided to compile annotations in the service container, as they are holders of information needed to process the request, and also some validation can already occur at the compile-time.
I mainly based my compiler passRegisterControllerAnnotationLocatorsPass on theRegisterControllerArgumentLocatorsPass. For each controller class, method or argument there's a registered container with all annotations, but only if any occurs. For now, I reused thecontroller.service_argument but it'd be better to rename it to thecontroller or thekernel.controller tag for more clarity. The main struggle with this was of course the problem with exporting objects to the service container. I came up with a simple solution: use the builtin__set_state method that is used byvar_export itself, in case of absence use the builtin serialization.
In my opinion, it's sufficient for just exporting annotations - in most cases, they have a simple structure. In general, exporting objects to the service container seem to be a broader topic, that's why I created theObjectExporterInterface and put my implementation in theDefaultObjectExporter. I thought I'll be able to use theVarExporter component, but for now, I don't see it fitting. I think exporting objects can be another great feature for the DI component. Everything that I've mentioned is already implemented and partially tested (I still need to work on it) in my bundleJungiFrameworkExtraBundle.


Now regarding the PR itself - the@QueryParam annotation. I've already put some of my thoughts in#36037 (comment) (quite long, can be boring).

Regarding what@nicolas-grekas wrote in#36037 (comment)

What is likely a problem is when it just provides another way to access some info: we don't need several ways to do what is already possible, because that calls for confusion.

Maybe start with a single annotation, eg QueryParam?

IMO starting it only with the@QueryParam annotation can also lead to confusion in some cases, mostly when someone will use it in a mixed way eg. POST & GET data. Someone can think why then at all use the@QueryParam annotation if still, I need to query theRequest instance to access the needed data? I think this annotation request style is great when can be used in order to avoid querying the wholeRequest object, but when it comes down to cases where theRequest instance is still needed then it's better to rely completely on it and do not use the annotations.
I've already used this annotation request style in several of my projects and most used and helpful is without a doubt the@RequestBody annotation. I don't even think I could work without it now doing the processing of some DTOs in the request. Maybe instead of just releasing the@QueryParam it'd better start from the@RequestBody or just release them together? Or just keep it in the extra bundle and see how they are doing? :)

WDYT?

@fabpot
Copy link
Member

Closing as#37829 introduced a simpler way to implement such attributes.

Implementing a simple RequestQuery attribute could be something like:

diff --git a/src/Symfony/Component/HttpKernel/Attribute/RequestQuery.php b/src/Symfony/Component/HttpKernel/Attribute/RequestQuery.phpnew file mode 100644index 0000000000..133c5457ae--- /dev/null+++ b/src/Symfony/Component/HttpKernel/Attribute/RequestQuery.php@@ -0,0 +1,22 @@+<?php++/*+ * This file is part of the Symfony package.+ *+ * (c) Fabien Potencier <fabien@symfony.com>+ *+ * For the full copyright and license information, please view the LICENSE+ * file that was distributed with this source code.+ */++namespace Symfony\Component\HttpKernel\Attribute;++use Attribute;++/**+ * Gets the request query parameter of the same name as the argument name.+ */+#[Attribute(Attribute::TARGET_PARAMETER)]+class RequestQuery implements ArgumentInterface+{+}diff --git a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.phpindex 4285ba7631..e78e07c8c6 100644--- a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php+++ b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php@@ -14,6 +14,7 @@ namespace Symfony\Component\HttpKernel\Controller; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Controller\ArgumentResolver\DefaultValueResolver; use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestAttributeValueResolver;+use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestQueryResolver; use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestValueResolver; use Symfony\Component\HttpKernel\Controller\ArgumentResolver\SessionValueResolver; use Symfony\Component\HttpKernel\Controller\ArgumentResolver\VariadicValueResolver;@@ -89,6 +90,7 @@ final class ArgumentResolver implements ArgumentResolverInterface             new RequestAttributeValueResolver(),             new RequestValueResolver(),             new SessionValueResolver(),+            new RequestQueryResolver(),             new DefaultValueResolver(),             new VariadicValueResolver(),         ];diff --git a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestQueryResolver.php b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestQueryResolver.phpnew file mode 100644index 0000000000..f68570c40e--- /dev/null+++ b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestQueryResolver.php@@ -0,0 +1,39 @@+<?php++/*+ * This file is part of the Symfony package.+ *+ * (c) Fabien Potencier <fabien@symfony.com>+ *+ * For the full copyright and license information, please view the LICENSE+ * file that was distributed with this source code.+ */++namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;++use Symfony\Component\HttpFoundation\Request;+use Symfony\Component\HttpKernel\Attribute\RequestQuery;+use Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface;+use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata;++/**+ * @author Fabien Potencier <fabien@symfony.com>+ */+final class RequestQueryResolver implements ArgumentValueResolverInterface+{+    /**+     * {@inheritdoc}+     */+    public function supports(Request $request, ArgumentMetadata $argument): bool+    {+        return $argument->getAttribute() instanceof RequestQuery;+    }++    /**+     * {@inheritdoc}+     */+    public function resolve(Request $request, ArgumentMetadata $argument): iterable+    {+        yield $request->query->all()[$argument->getName()] ?? $argument->hasDefaultValue() ? $argument->getDefaultValue() : null;+    }+}
tsantos84 reacted with thumbs up emoji

@fabpot
Copy link
Member

Let's resume the discussion in#38162

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

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

6 participants

@tsantos84@nicolas-grekas@linaori@piku235@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp