Skip to content

[HttpFoundation] change precedence of parameters in Request::get #16076

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

Merged
merged 1 commit into from
Oct 5, 2015

Conversation

Tobion
Copy link
Contributor

@Tobion Tobion commented Oct 2, 2015

Q A
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Allowing the request attributes to be overwritten via GET parameters is risky and made #8966 even worse.
It is even more risky because it skips the requirements checks as configured in routing. So people that set requirements for routing placeholders like \d+ or html|json can be sure it is validated when using the routing variables. But if developers use $request->get() to retrieve them, anybody from outside can set any value for those.

*
* Avoid using this method in controllers:
*
* * slow
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not relevant anymore with the removal of deep and also since the refactoring in #12369

@Tobion
Copy link
Contributor Author

Tobion commented Oct 4, 2015

An alternative would be to deprecate get and instead introduce a new method like getParam with the new precedence. This way there would be an upgrade path.

@fabpot
Copy link
Member

fabpot commented Oct 4, 2015

@Tobion I think this PR is fine without introducing another method.

👍

@fabpot
Copy link
Member

fabpot commented Oct 5, 2015

Thank you @Tobion.

@fabpot fabpot merged commit e8d6764 into symfony:master Oct 5, 2015
fabpot added a commit that referenced this pull request Oct 5, 2015
…quest::get (Tobion)

This PR was merged into the 3.0-dev branch.

Discussion
----------

[HttpFoundation] change precedence of parameters in Request::get

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Allowing the request attributes to be overwritten via GET parameters is risky and made #8966 even worse.
It is even more risky because it skips the requirements checks as configured in routing. So people that set requirements for routing placeholders like `\d+` or `html|json` can be sure it is validated when using the routing variables. But if developers use `$request->get()` to retrieve them, anybody from outside can set any value for those.

Commits
-------

e8d6764 [HttpFoundation] change precedence of parameters in Request::get
@Tobion Tobion deleted the request-get-priority branch October 5, 2015 11:50
@fabpot fabpot mentioned this pull request Nov 16, 2015
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.

3 participants