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

[HttpFoundation] fixed using _method parameter with invalid type #28080

Merged
merged 1 commit into from Jul 29, 2018

Conversation

Projects
None yet
5 participants
@Phobetor
Copy link
Contributor

commented Jul 27, 2018

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

This change makes sure that an incoming _method parameter is only used when it is a string value.

$request = new Request();
$request->setMethod('POST');
$request->query->set('_method', array('delete', 'patch'));
$this->assertEquals('POST', $request->getMethod(), '->getMethod() returns the request method if invalid type is defined in query');

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jul 27, 2018

Member

assertSame()

This comment has been minimized.

Copy link
@Phobetor

Phobetor Jul 27, 2018

Author Contributor

@xabbuh
Sorry, I just copied that from the assertion above to make no mistake. Thanks for the hint.

Should I fix this only for my new assertion or also for the others? Or should this be done in a different ticket?

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jul 27, 2018

Member

changing it just here is okay IMO

This comment has been minimized.

Copy link
@Phobetor

Phobetor Jul 27, 2018

Author Contributor

Done.

@xabbuh

xabbuh approved these changes Jul 27, 2018

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Jul 28, 2018

@@ -1276,7 +1276,10 @@ public function getMethod()
if ($method = $this->headers->get('X-HTTP-METHOD-OVERRIDE')) {
$this->method = strtoupper($method);
} elseif (self::$httpMethodParameterOverride) {
$this->method = strtoupper($this->request->get('_method', $this->query->get('_method', 'POST')));
$method = $this->request->get('_method', $this->query->get('_method', 'POST'));
if (\is_string($method)) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 28, 2018

Member

what should happen when a non-string is found here?
returning it as is looks strange, isn't it? it break the method's signature somehow.

This comment has been minimized.

Copy link
@Phobetor

Phobetor Jul 28, 2018

Author Contributor

In that case it would keep the current request method ("POST") because the override value is not valid. Sounds consistant to me.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 28, 2018

Member

Oh, indeed :)

@nicolas-grekas nicolas-grekas force-pushed the Phobetor:ticket_28079 branch from c0a95d1 to 63583de Jul 29, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

Thank you @Phobetor.

@nicolas-grekas nicolas-grekas merged commit 63583de into symfony:2.8 Jul 29, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jul 29, 2018

bug #28080 [HttpFoundation] fixed using _method parameter with invali…
…d type (Phobetor)

This PR was squashed before being merged into the 2.8 branch (closes #28080).

Discussion
----------

[HttpFoundation] fixed using _method parameter with invalid type

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

This change makes sure that an incoming `_method` parameter is only used when it is a string value.

Commits
-------

63583de [HttpFoundation] fixed using _method parameter with invalid type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.