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] Add RequestQuery attribute#38162

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
fabpot wants to merge1 commit intosymfony:masterfromfabpot:query-param-attribute

Conversation

@fabpot
Copy link
Member

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
Ticketsrefs#36135
LicenseMIT
Doc PR

Implements a RequestQuery attribute (see#37829 for the feature) like described in#36135. As we don't have yet constraint attributes (#38096), I've submitted the PR to start a discussion (not even sure we want to support it in core).

linaori, Koc, derrabus, and antoinearrive reacted with hooray emoji
@piku235
Copy link
Contributor

Hi@fabpot, thanks for continuing the discussion about request attributes.

The whole discussion actually started from#36037. To quickly summarize it, the feelings about the request annotations/attributes were quite mixed. In my opinion, they're a great addition, I myself I've been using them for more than a year already. I published not so long ago a stable version of my bundleJungiFrameworkExtraBundle that containsRequestBody,QueryParam,RequestParam and more. For now, it contains only annotations, but I've finished working on attributes and I plan soon to publish them. I used a different way of accessing annotations/attributes in a request-time, instead of the lastest feature#37829, I used a method that@nicolas-grekas mentioned in#36135 (review). Thanks to that, I didn't lose on performance in a request-time, and also could perform additional validation in the compile-time (especially for annotations).

As I wrote before in#36135 (comment), my only worry with publishing only RequestQuery (before QueryParam) is it may pollute a controller interface in some cases, it introduces a mixed style - using attributes exchangeably with aRequest instance. It may happen that one action uses only query parameters, so RequestQuery can be easily used, but another action in the same controller will use an entireRequest instance to eg deserialize the request message body. From the pure controller interface perspective, it doesn't look so good. The great add of this annotation/attribute style is that they refrain from using an entireRequest instance where the context is bigger than needed and parameters are unknown, so there's a need to check them yourself before using.

In my opinion, if we really want to have it in the core, it'll be good to publish RequestQuery with RequestBody attribute. Trough my experience, I've been using the most RequestBody and sometimes QueryParam, they should cover most of the use cases in a controller.

@nicolas-grekasnicolas-grekas added this to thenext milestoneSep 14, 2020
@nicolas-grekasnicolas-grekas changed the titleAdd RequestQuery attribute[HttpKernel] Add RequestQuery attributeSep 14, 2020
@derrabus
Copy link
Member

I'm currently working on a project where the API makes heavy use of query parameters. To me, an attribute like this would really make sense if it is combined with input validation and error handling, so that a request with an invalid or missing mandatory query parameter won't event hit the controller. This way, I could remove a lot of redundant boilerplate.

And I don't think that we would necessarily need constraints for that. Parameter types could also indicate validation. Take for example a URL like this:http://example.com/api/search?q=some+search+term&page=2

publicfunction searchAction(    #[RequestQuery]string$q,    #[RequestQuery]int$page =1) {

In this case, we could already perform the following validations:

  • Query parameterq is missing: The parameter is not nullable and the action does not define a default value => error.
  • page=foo: Not a valid integer => error.

We could for instance throw aBadRequestHttpException in such a case.

@fabpotfabpot closed thisOct 7, 2020
@fabpot
Copy link
MemberAuthor

We've just moved away frommaster as the main branch to use5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

@rrajkomar
Copy link

Hello,
Just wondering what is the state of this PR ?
Has another PR been created and not been linked or has this feature been abandoned ?
Thanks.

derrabus reacted with thumbs up emoji

@derrabus
Copy link
Member

I think, this PR was merely meant as a PoC. At least I would want to pick up this topic for Symfony 5.3. 🙃

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

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

6 participants

@fabpot@piku235@derrabus@rrajkomar@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp