feature #33217 [FrameworkBundle][DX] Improving the redirect config wh…

…en using RedirectController (yceruto) This PR was merged into the 4.4 branch. Discussion ---------- [FrameworkBundle][DX] Improving the redirect config when using RedirectController | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#12189 follow-up #24637 **Before:** ```yaml # config/routes.yaml doc_shortcut: path: /doc controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction defaults: route: 'doc_page' legacy_doc: path: /legacy/doc controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction defaults: path: 'https://legacy.example.com/doc' ``` **After:** ```yaml # config/routes.yaml doc_shortcut: path: /doc controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController defaults: route: 'doc_page' legacy_doc: path: /legacy/doc controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController defaults: path: 'https://legacy.example.com/doc' ``` See more before/after configs (XML, PHP) in doc PR symfony/symfony-docs#12189 Commits ------- 0ebb469 Improving redirect config when using RedirectController
symfony · Aug 23, 2019 · dc8d470 · dc8d470
2 parents 123418e + 0ebb469
commit dc8d470
Show file tree

Hide file tree

Showing 2 changed files with 103 additions and 3 deletions.
diff --git a/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php b/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php
@@ -157,4 +157,23 @@ public function urlRedirectAction(Request $request, string $path, bool $permanen
 
         return new RedirectResponse($url, $statusCode);
     }
+
+    public function __invoke(Request $request): Response
+    {
+        $p = $request->attributes->get('_route_params', []);
+
+        if (\array_key_exists('route', $p)) {
+            if (\array_key_exists('path', $p)) {
+                throw new \RuntimeException(sprintf('Ambiguous redirection settings, use the "path" or "route" parameter, not both: "%s" and "%s" found respectively in "%s" routing configuration.', $p['path'], $p['route'], $request->attributes->get('_route')));
+            }
+
+            return $this->redirectAction($request, $p['route'], $p['permanent'] ?? false, $p['ignoreAttributes'] ?? false, $p['keepRequestMethod'] ?? false, $p['keepQueryParams'] ?? false);
+        }
+
+        if (\array_key_exists('path', $p)) {
+            return $this->urlRedirectAction($request, $p['path'], $p['permanent'] ?? false, $p['scheme'] ?? null, $p['httpPort'] ?? null, $p['httpsPort'] ?? null, $p['keepRequestMethod'] ?? false);
+        }
+
+        throw new \RuntimeException(sprintf('The parameter "path" or "route" is required to configure the redirect action in "%s" routing configuration.', $request->attributes->get('_route')));
+    }
 }
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php
@@ -42,6 +42,22 @@ public function testEmptyRoute()
         } catch (HttpException $e) {
             $this->assertSame(404, $e->getStatusCode());
         }
+
+        $request = new Request([], [], ['_route_params' => ['route' => '', 'permanent' => true]]);
+        try {
+            $controller($request);
+            $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown');
+        } catch (HttpException $e) {
+            $this->assertSame(410, $e->getStatusCode());
+        }
+
+        $request = new Request([], [], ['_route_params' => ['route' => '', 'permanent' => false]]);
+        try {
+            $controller($request);
+            $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown');
+        } catch (HttpException $e) {
+            $this->assertSame(404, $e->getStatusCode());
+        }
     }
 
     /**
@@ -71,15 +87,18 @@ public function testRoute($permanent, $keepRequestMethod, $keepQueryParams, $ign
 
         $router = $this->getMockBuilder(UrlGeneratorInterface::class)->getMock();
         $router
-            ->expects($this->once())
+            ->expects($this->exactly(2))
             ->method('generate')
             ->with($this->equalTo($route), $this->equalTo($expectedAttributes))
             ->willReturn($url);
 
         $controller = new RedirectController($router);
 
         $returnResponse = $controller->redirectAction($request, $route, $permanent, $ignoreAttributes, $keepRequestMethod, $keepQueryParams);
+        $this->assertRedirectUrl($returnResponse, $url);
+        $this->assertEquals($expectedCode, $returnResponse->getStatusCode());
 
+        $returnResponse = $controller($request);
         $this->assertRedirectUrl($returnResponse, $url);
         $this->assertEquals($expectedCode, $returnResponse->getStatusCode());
     }
@@ -116,14 +135,35 @@ public function testEmptyPath()
         } catch (HttpException $e) {
             $this->assertSame(404, $e->getStatusCode());
         }
+
+        $request = new Request([], [], ['_route_params' => ['path' => '', 'permanent' => true]]);
+        try {
+            $controller($request);
+            $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown');
+        } catch (HttpException $e) {
+            $this->assertSame(410, $e->getStatusCode());
+        }
+
+        $request = new Request([], [], ['_route_params' => ['path' => '', 'permanent' => false]]);
+        try {
+            $controller($request);
+            $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown');
+        } catch (HttpException $e) {
+            $this->assertSame(404, $e->getStatusCode());
+        }
     }
 
     public function testFullURL()
     {
         $request = new Request();
         $controller = new RedirectController();
+
         $returnResponse = $controller->urlRedirectAction($request, 'http://foo.bar/');
+        $this->assertRedirectUrl($returnResponse, 'http://foo.bar/');
+        $this->assertEquals(302, $returnResponse->getStatusCode());
 
+        $request = new Request([], [], ['_route_params' => ['path' => 'http://foo.bar/']]);
+        $returnResponse = $controller($request);
         $this->assertRedirectUrl($returnResponse, 'http://foo.bar/');
         $this->assertEquals(302, $returnResponse->getStatusCode());
     }
@@ -132,8 +172,13 @@ public function testFullURLWithMethodKeep()
     {
         $request = new Request();
         $controller = new RedirectController();
+
         $returnResponse = $controller->urlRedirectAction($request, 'http://foo.bar/', false, null, null, null, true);
+        $this->assertRedirectUrl($returnResponse, 'http://foo.bar/');
+        $this->assertEquals(307, $returnResponse->getStatusCode());
 
+        $request = new Request([], [], ['_route_params' => ['path' => 'http://foo.bar/', 'keepRequestMethod' => true]]);
+        $returnResponse = $controller($request);
         $this->assertRedirectUrl($returnResponse, 'http://foo.bar/');
         $this->assertEquals(307, $returnResponse->getStatusCode());
     }
@@ -151,12 +196,18 @@ public function testUrlRedirectDefaultPorts()
         $controller = $this->createRedirectController(null, $httpsPort);
         $returnValue = $controller->urlRedirectAction($request, $path, false, 'https');
         $this->assertRedirectUrl($returnValue, $expectedUrl);
+        $request->attributes = new ParameterBag(['_route_params' => ['path' => $path, 'scheme' => 'https']]);
+        $returnValue = $controller($request);
+        $this->assertRedirectUrl($returnValue, $expectedUrl);
 
         $expectedUrl = "http://$host:$httpPort$baseUrl$path";
         $request = $this->createRequestObject('https', $host, $httpPort, $baseUrl);
         $controller = $this->createRedirectController($httpPort);
         $returnValue = $controller->urlRedirectAction($request, $path, false, 'http');
         $this->assertRedirectUrl($returnValue, $expectedUrl);
+        $request->attributes = new ParameterBag(['_route_params' => ['path' => $path, 'scheme' => 'http']]);
+        $returnValue = $controller($request);
+        $this->assertRedirectUrl($returnValue, $expectedUrl);
     }
 
     public function urlRedirectProvider()
@@ -205,6 +256,10 @@ public function testUrlRedirect($scheme, $httpPort, $httpsPort, $requestScheme,
 
         $returnValue = $controller->urlRedirectAction($request, $path, false, $scheme, $httpPort, $httpsPort);
         $this->assertRedirectUrl($returnValue, $expectedUrl);
+
+        $request->attributes = new ParameterBag(['_route_params' => ['path' => $path, 'scheme' => $scheme, 'httpPort' => $httpPort, 'httpsPort' => $httpsPort]]);
+        $returnValue = $controller($request);
+        $this->assertRedirectUrl($returnValue, $expectedUrl);
     }
 
     public function pathQueryParamsProvider()
@@ -234,6 +289,10 @@ public function testPathQueryParams($expectedUrl, $path, $queryString)
 
         $returnValue = $controller->urlRedirectAction($request, $path, false, $scheme, $port, null);
         $this->assertRedirectUrl($returnValue, $expectedUrl);
+
+        $request->attributes = new ParameterBag(['_route_params' => ['path' => $path, 'scheme' => $scheme, 'httpPort' => $port]]);
+        $returnValue = $controller($request);
+        $this->assertRedirectUrl($returnValue, $expectedUrl);
     }
 
     public function testRedirectWithQuery()
@@ -247,10 +306,13 @@ public function testRedirectWithQuery()
         $request->query = new ParameterBag(['base' => 'zaza']);
         $request->attributes = new ParameterBag(['_route_params' => ['base2' => 'zaza']]);
         $urlGenerator = $this->getMockBuilder(UrlGeneratorInterface::class)->getMock();
-        $urlGenerator->expects($this->once())->method('generate')->willReturn('/test?base=zaza&base2=zaza')->with('/test', ['base' => 'zaza', 'base2' => 'zaza'], UrlGeneratorInterface::ABSOLUTE_URL);
+        $urlGenerator->expects($this->exactly(2))->method('generate')->willReturn('/test?base=zaza&base2=zaza')->with('/test', ['base' => 'zaza', 'base2' => 'zaza'], UrlGeneratorInterface::ABSOLUTE_URL);
 
         $controller = new RedirectController($urlGenerator);
         $this->assertRedirectUrl($controller->redirectAction($request, '/test', false, false, false, true), '/test?base=zaza&base2=zaza');
+
+        $request->attributes->set('_route_params', ['base2' => 'zaza', 'route' => '/test', 'ignoreAttributes' => false, 'keepRequestMethod' => false, 'keepQueryParams' => true]);
+        $this->assertRedirectUrl($controller($request), '/test?base=zaza&base2=zaza');
     }
 
     public function testRedirectWithQueryWithRouteParamsOveriding()
@@ -264,10 +326,29 @@ public function testRedirectWithQueryWithRouteParamsOveriding()
         $request->query = new ParameterBag(['base' => 'zaza']);
         $request->attributes = new ParameterBag(['_route_params' => ['base' => 'zouzou']]);
         $urlGenerator = $this->getMockBuilder(UrlGeneratorInterface::class)->getMock();
-        $urlGenerator->expects($this->once())->method('generate')->willReturn('/test?base=zouzou')->with('/test', ['base' => 'zouzou'], UrlGeneratorInterface::ABSOLUTE_URL);
+        $urlGenerator->expects($this->exactly(2))->method('generate')->willReturn('/test?base=zouzou')->with('/test', ['base' => 'zouzou'], UrlGeneratorInterface::ABSOLUTE_URL);
 
         $controller = new RedirectController($urlGenerator);
         $this->assertRedirectUrl($controller->redirectAction($request, '/test', false, false, false, true), '/test?base=zouzou');
+
+        $request->attributes->set('_route_params', ['base' => 'zouzou', 'route' => '/test', 'ignoreAttributes' => false, 'keepRequestMethod' => false, 'keepQueryParams' => true]);
+        $this->assertRedirectUrl($controller($request), '/test?base=zouzou');
+    }
+
+    public function testMissingPathOrRouteParameter()
+    {
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionMessage('The parameter "path" or "route" is required to configure the redirect action in "_redirect" routing configuration.');
+
+        (new RedirectController())(new Request([], [], ['_route' => '_redirect']));
+    }
+
+    public function testAmbiguousPathAndRouteParameter()
+    {
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionMessage('Ambiguous redirection settings, use the "path" or "route" parameter, not both: "/foo" and "bar" found respectively in "_redirect" routing configuration.');
+
+        (new RedirectController())(new Request([], [], ['_route' => '_redirect', '_route_params' => ['path' => '/foo', 'route' => 'bar']]));
     }
 
     private function createRequestObject($scheme, $host, $port, $baseUrl, $queryString = '')