Fix return value from CRUDController::getRestMethod() respecting Request::getHttpMethodParameterOverride()

[phansys]

Subject

Fix return value from CRUDController::getRestMethod() respecting Request::getHttpMethodParameterOverride().

I am targeting this branch, because this change respects BC.

Closes #6046.
Closes #6098.

Changelog

### Fixed
- Fixed return value from `CRUDController::getRestMethod()` respecting `Request::getHttpMethodParameterOverride()`.

### Deprecated
- Deprecated `Sonata\AdminBundle\Controller\CRUDController::getRestMethod()` method in favor of `Symfony\Component\HttpFoundation\Request::getMethod()`.

[phansys]

@sonata-project/contributors, do you know why we need this method instead of trusting Request::getMethod() directly?

if (!$method && self::$httpMethodParameterOverride) {
    $method = $this->request->get('_method', $this->query->get('_method', 'POST'));
}

AFAIK, this is only avoiding to use the logic defined there, like falling back to $this->query->get('_method', 'POST') or using X-HTTP-METHOD-OVERRIDE header.
Since it's not clear to me, is not easy to create valid test cases.
Any clue is welcome.

[phansys]

Reading the history, maybe this method is not required anymore.

[wbloszyk]

Request::getMethod() will return $_SERVER['REQUEST_METHOD'] with some hack for POST where are additional checking and return if exist in some order:

1. $this->headers->get('X-HTTP-METHOD-OVERRIDE');
2. $httpMethodParameterOverride === true
   • $_POST['_method']
   • $_GET['_method']
   • 'POST'

Technicly exist two solution:
1.

Request::enableHttpMethodParameterOverride()
if (Request::METHOD_DELETE === $request->getMethod()) {

or

if (
    Request::METHOD_POST === $request->getMethod() &&
    'DELETE' === $request->request->get('_method')
) {

For better understand it look at Symfony\Component\HttpFoundation\Request::getMethod()

[wbloszyk]

Suggested change

Request::enableHttpMethodParameterOverride();

This should not be enabled in tests.

[phansys]

What do you propose to cover the expected behavior then?
Additionally, please note that the original value for Request::$httpMethodParameterOverride is being restored after each individual test:

SonataAdminBundle/tests/Controller/CRUDControllerTest.php

Lines 376 to 386 in 39c4e24

	protected function tearDown(): void
	{
	parent::tearDown();
	if (!$this->httpMethodParameterOverride && Request::getHttpMethodParameterOverride()) {
	$disableHttpMethodParameterOverride = \Closure::bind(static function (): void {
	self::$httpMethodParameterOverride = false;
	}, null, Request::class);
	$disableHttpMethodParameterOverride();
	}
	}

[wbloszyk]

This should be done in Action, otherwise test will work and action do not:

SonataAdminBundle/src/Controller/CRUDController.php

Line 220 in 39c4e24

if (Request::METHOD_DELETE === $request->getMethod()) {

[phansys]

I'm sorry @wbloszyk, I don't understand your suggestion or why you say it won't work.
AFAIK, the choice about enabling or disabling the http method overriding is a global responsibility in the context of the whole application execution. By instance, the configuration node framework.http_method_override establishes this setting globally.

[wbloszyk]

https://symfony.com/doc/2.6/book/forms.html#book-forms-changing-action-and-method

NOTE
If the form’s method is not GET or POST, but PUT, PATCH or DELETE, Symfony will insert a hidden field with the name _method that stores this method. The form will be submitted in a normal POST request, but Symfony’s router is capable of detecting the _method parameter and will interpret it as a PUT, PATCH or DELETE request. Read the cookbook chapter “How to Use HTTP Methods beyond GET and POST in Routes” for more information.

When $disableHttpMethodParameterOverride === false then

if (Request::METHOD_DELETE === $request->getMethod()) { } // always false
if (Request::METHOD_DELETE === $request->getRestMethod()) { } // always false, before this change - true

[wbloszyk]

Otherwise Request::enableHttpMethodParameterOverride() will be not nesessery in tests.

[phansys]

https://symfony.com/doc/2.6/book/forms.html#book-forms-changing-action-and-method

I'm not sure I understand what are you trying to expose, but I think we should start from the fact that Request::$httpMethodParameterOverride is a property that enables the Request::getMethod() to return other values than the real HTTP method ($_SERVER['REQUEST_METHOD']), based on the "_method" param provided in the $_POST or $_GET superglobals or in the "X-HTTP-METHOD-OVERRIDE" header.
Regarding this doc entry, be aware that FormConfigBuilder::setMethod() is setting this "_method" parameter.

Otherwise Request::enableHttpMethodParameterOverride() will be not nesessery in tests.

If you run these tests, you'll see that calling Request::enableHttpMethodParameterOverride() is necessary to achieve the expected result.

Maybe I'm not understanding the scenario you are talking about.
If that is the case or there is something else I'm omitting or misunderstanding about your concern, please try to share a test case or a alternative PR with what you think should be done. That way I think we could get a clearer perspective.

[wbloszyk]

If you run these tests, you'll see that calling Request::enableHttpMethodParameterOverride() is necessary to achieve the expected result.

So code is working when Request::$httpMethodParameterOverride is enabled and don't when it is disabled. It is what I mean. I think current code is working correct and we should consider #6046 one more time.

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[VincentLanglet]

@phansys @wbloszyk What's the status of this ?

[phansys]

@phansys @wbloszyk What's the status of this ?

IIUC the issue, this PR is RTM.
@wbloszyk, if you think other approach must be followed, please try to provide a new PR in order to check what you mean, since I'm not understanding the point in your comments.

[wbloszyk]

@phansys In this case, when people will have this parameter disabled then this if will be treated as allways false.

Case 1:

framework:
    http_method_override: true

All forms will be generated as GET|POST (Request::getMethod()). If you want use DELETE then you should take it from _method parameter.

Case 2:

framework:
    http_method_override: false

Form attribute method will be override. If you want use DELETE then you can take it from Request::getMethod().

[phansys]

AFAIK, the "_method" request parameter must only receive a special treatment when Request::$httpMethodParameterOverride is set to true, otherwise it is a common parameter like any other:

• https://symfony.com/doc/current/routing.html#matching-http-methods

HTML forms only support GET and POST methods. If you’re calling a route with a different method from an HTML form, add a hidden field called _method with the method to use (e.g. ). If you create your forms with Symfony Forms this is done automatically for you.

• https://symfony.com/doc/current/forms.html#changing-the-action-and-http-method

If the form’s method is not GET or POST, but PUT, PATCH or DELETE, Symfony will insert a hidden field with the name _method that stores this method. The form will be submitted in a normal POST request, but Symfony’s routing is capable of detecting the _method parameter and will interpret it as a PUT, PATCH or DELETE request. See the http_method_override option.

• https://symfony.com/doc/current/reference/configuration/framework.html#configuration-framework-http-method-override

http_method_override¶
type: boolean default: true

This determines whether the _method request parameter is used as the intended HTTP method on POST requests. If enabled, the Request::enableHttpMethodParameterOverride method gets called automatically. It becomes the service container parameter named kernel.http_method_override.

[wbloszyk]

Correct, my bad here. It is little confusing. By default Request::$httpMethodParameterOverride is set to true. It mean it should not be set to true in tests. But as standalone component Request::$httpMethodParameterOverride is set to false. In test ,where Request is not taken from kernel, this parameter must be override.

protected static $httpMethodParameterOverride = false;

[VincentLanglet]

Thanks @phansys