Fix return value from CRUDController::getRestMethod() respecting `R…

…equest::getHttpMethodParameterOverride()`
sonata-project · Aug 27, 2020 · d1ffb31 · d1ffb31
1 parent 10afadb
commit d1ffb31
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 9 deletions.
diff --git a/UPGRADE-3.x.md b/UPGRADE-3.x.md
@@ -4,6 +4,11 @@ UPGRADE 3.x
 UPGRADE FROM 3.74 to 3.75
 =========================
 
+## Deprecated `Sonata\AdminBundle\Controller\CRUDController::getRestMethod()` method
+
+`Sonata\AdminBundle\Controller\CRUDController::getRestMethod()` method is deprecated.
+Use `Symfony\Component\HttpFoundation\Request::getMethod()` instead.
+
 ## Deprecated `Sonata\AdminBundle\Model\ModelManagerInterface` collection-related methods.
 
 Use:

diff --git a/src/Controller/CRUDController.php b/src/Controller/CRUDController.php
@@ -217,7 +217,7 @@ public function deleteAction($id) // NEXT_MAJOR: Remove the unused $id parameter
             return $preResponse;
         }
 
-        if (Request::METHOD_DELETE === $this->getRestMethod()) {
+        if (Request::METHOD_DELETE === $request->getMethod()) {
             // check the csrf token
             $this->validateCsrfToken('sonata.delete');
 
@@ -405,7 +405,7 @@ public function editAction($deprecatedId = null) // NEXT_MAJOR: Remove the unuse
     public function batchAction()
     {
         $request = $this->getRequest();
-        $restMethod = $this->getRestMethod();
+        $restMethod = $request->getMethod();
 
         if (Request::METHOD_POST !== $restMethod) {
             throw $this->createNotFoundException(sprintf(
@@ -1098,20 +1098,25 @@ protected function isXmlHttpRequest()
     }
 
     /**
+     * NEXT_MAJOR: Remove this method.
+     *
      * Returns the correct RESTful verb, given either by the request itself or
      * via the "_method" parameter.
      *
+     * @deprecated since sonata-project/admin-bundle 3.x, to be removed in 4.0. Use `Request::getMethod()` instead.
+     *
      * @return string HTTP method, either
      */
     protected function getRestMethod()
     {
-        $request = $this->getRequest();
-
-        if (Request::getHttpMethodParameterOverride() || !$request->request->has('_method')) {
-            return $request->getMethod();
-        }
+        @trigger_error(sprintf(
+            'Method "%s()" is deprecated since sonata-project/admin-bundle 3.x'
+            .', to be removed in 4.0. Use `%s::getMethod()` instead.',
+            __METHOD__,
+            Request::class
+        ), E_USER_DEPRECATED);
 
-        return $request->request->get('_method');
+        return $this->getRequest()->getMethod();
     }
 
     /**
@@ -1244,7 +1249,7 @@ protected function redirectTo($object)
             $url = $this->admin->generateUrl('create', $params);
         }
 
-        if ('DELETE' === $this->getRestMethod()) {
+        if ('DELETE' === $request->getMethod()) {
             return $this->redirectToList();
         }
 

diff --git a/tests/Controller/CRUDControllerTest.php b/tests/Controller/CRUDControllerTest.php
@@ -157,11 +157,17 @@ class CRUDControllerTest extends TestCase
      */
     private $logger;
 
+    /**
+     * @var bool
+     */
+    private $httpMethodParameterOverride = false;
+
     /**
      * {@inheritdoc}
      */
     protected function setUp(): void
     {
+        $this->httpMethodParameterOverride = Request::getHttpMethodParameterOverride();
         $this->container = new Container();
         $this->request = new Request();
         $this->pool = new Pool($this->container, 'title', 'logo.png');
@@ -367,6 +373,18 @@ static function (string $name, $object, array $parameters = []): string {
         }
     }
 
+    protected function tearDown(): void
+    {
+        parent::tearDown();
+
+        if (!$this->httpMethodParameterOverride && Request::getHttpMethodParameterOverride()) {
+            $disableHttpMethodParameterOverride = \Closure::bind(static function (): void {
+                self::$httpMethodParameterOverride = false;
+            }, null, Request::class);
+            $disableHttpMethodParameterOverride();
+        }
+    }
+
     public function testRenderJson1(): void
     {
         $data = ['example' => '123', 'foo' => 'bar'];
@@ -1095,11 +1113,14 @@ public function testDeleteActionAjaxSuccess2(): void
 
         $this->request->headers->set('X-Requested-With', 'XMLHttpRequest');
 
+        Request::enableHttpMethodParameterOverride();
+
         $response = $this->controller->deleteAction(1);
 
         $this->assertInstanceOf(Response::class, $response);
         $this->assertSame(json_encode(['result' => 'ok']), $response->getContent());
         $this->assertSame([], $this->session->getFlashBag()->all());
+        $this->assertSame(Request::METHOD_DELETE, $this->request->getMethod());
     }
 
     public function testDeleteActionAjaxError(): void
@@ -1226,11 +1247,14 @@ public function testDeleteActionSuccess2(string $expectedToStringValue, string $
 
         $this->request->request->set('_sonata_csrf_token', 'csrf-token-123_sonata.delete');
 
+        Request::enableHttpMethodParameterOverride();
+
         $response = $this->controller->deleteAction(1);
 
         $this->assertInstanceOf(RedirectResponse::class, $response);
         $this->assertSame(['flash_delete_success'], $this->session->getFlashBag()->get('sonata_flash_success'));
         $this->assertSame('list', $response->getTargetUrl());
+        $this->assertSame(Request::METHOD_DELETE, $this->request->getMethod());
     }
 
     /**
@@ -1261,11 +1285,14 @@ public function testDeleteActionSuccessNoCsrfTokenProvider(string $expectedToStr
         $this->request->setMethod(Request::METHOD_POST);
         $this->request->request->set('_method', Request::METHOD_DELETE);
 
+        Request::enableHttpMethodParameterOverride();
+
         $response = $this->controller->deleteAction(1);
 
         $this->assertInstanceOf(RedirectResponse::class, $response);
         $this->assertSame(['flash_delete_success'], $this->session->getFlashBag()->get('sonata_flash_success'));
         $this->assertSame('list', $response->getTargetUrl());
+        $this->assertSame(Request::METHOD_DELETE, $this->request->getMethod());
     }
 
     public function testDeleteActionWrongRequestMethod(): void
@@ -1284,6 +1311,8 @@ public function testDeleteActionWrongRequestMethod(): void
         //without POST request parameter "_method" should not be used as real REST method
         $this->request->query->set('_method', Request::METHOD_DELETE);
 
+        Request::enableHttpMethodParameterOverride();
+
         $this->assertInstanceOf(Response::class, $this->controller->deleteAction(1));
 
         $this->assertSame($this->admin, $this->parameters['admin']);
@@ -1296,6 +1325,7 @@ public function testDeleteActionWrongRequestMethod(): void
 
         $this->assertSame([], $this->session->getFlashBag()->all());
         $this->assertSame('@SonataAdmin/CRUD/delete.html.twig', $this->template);
+        $this->assertSame(Request::METHOD_GET, $this->request->getMethod());
     }
 
     /**
@@ -1350,12 +1380,16 @@ public function testDeleteActionInvalidCsrfToken(): void
         $this->request->request->set('_method', Request::METHOD_DELETE);
         $this->request->request->set('_sonata_csrf_token', 'CSRF-INVALID');
 
+        Request::enableHttpMethodParameterOverride();
+
         try {
             $this->controller->deleteAction(1);
         } catch (HttpException $e) {
             $this->assertSame('The csrf token is not valid, CSRF attack?', $e->getMessage());
             $this->assertSame(400, $e->getStatusCode());
         }
+
+        $this->assertSame(Request::METHOD_DELETE, $this->request->getMethod());
     }
 
     public function testEditActionNotFoundException(): void