From f4265fb93b75dd3ee97ffb11e35155e682f509c0 Mon Sep 17 00:00:00 2001 From: Pawel Smolinski Date: Sun, 18 Feb 2018 13:48:13 +0000 Subject: [PATCH 1/4] Issue #26171 - allow to use 307/308 HTTP response codes --- .../Controller/RedirectController.php | 48 ++++++++++++------- .../Controller/RedirectControllerTest.php | 41 +++++++++++----- 2 files changed, 59 insertions(+), 30 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php b/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php index 8e6ce84408d09..5467ce85eab63 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php +++ b/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php @@ -46,29 +46,37 @@ public function __construct(UrlGeneratorInterface $router = null, int $httpPort * In case the route name is empty, the status code will be 404 when permanent is false * and 410 otherwise. * - * @param Request $request The request instance - * @param string $route The route name to redirect to - * @param bool $permanent Whether the redirection is permanent - * @param bool|array $ignoreAttributes Whether to ignore attributes or an array of attributes to ignore + * @param Request $request The request instance + * @param string $route The route name to redirect to + * @param bool $permanent Whether the redirection is permanent + * @param bool|array $ignoreAttributes Whether to ignore attributes or an array of attributes to ignore + * @param bool $keepRequestMethod Wheter redirect action should keep HTTP request method * * @throws HttpException In case the route name is empty */ - public function redirectAction(Request $request, string $route, bool $permanent = false, $ignoreAttributes = false): Response + public function redirectAction(Request $request, string $route, bool $permanent = false, $ignoreAttributes = false, bool $keepRequestMethod = false): Response { if ('' == $route) { - throw new HttpException($permanent ? 410 : 404); + throw new HttpException($permanent ? Response::HTTP_GONE : Response::HTTP_NOT_FOUND); } $attributes = array(); if (false === $ignoreAttributes || is_array($ignoreAttributes)) { $attributes = $request->attributes->get('_route_params'); - unset($attributes['route'], $attributes['permanent'], $attributes['ignoreAttributes']); + unset($attributes['route'], $attributes['permanent'], $attributes['ignoreAttributes'], $attributes['keepRequestMethod']); if ($ignoreAttributes) { $attributes = array_diff_key($attributes, array_flip($ignoreAttributes)); } } - return new RedirectResponse($this->router->generate($route, $attributes, UrlGeneratorInterface::ABSOLUTE_URL), $permanent ? 301 : 302); + if ($keepRequestMethod) { + $statusCode = $permanent ? Response::HTTP_PERMANENTLY_REDIRECT : Response::HTTP_TEMPORARY_REDIRECT; + } else { + @trigger_error('Since next major release redirect action will be made with 307/308 HTTP status codes', \E_USER_DEPRECATED); + $statusCode = $permanent ? Response::HTTP_MOVED_PERMANENTLY : Response::HTTP_FOUND; + } + + return new RedirectResponse($this->router->generate($route, $attributes, UrlGeneratorInterface::ABSOLUTE_URL), $statusCode); } /** @@ -80,22 +88,28 @@ public function redirectAction(Request $request, string $route, bool $permanent * In case the path is empty, the status code will be 404 when permanent is false * and 410 otherwise. * - * @param Request $request The request instance - * @param string $path The absolute path or URL to redirect to - * @param bool $permanent Whether the redirect is permanent or not - * @param string|null $scheme The URL scheme (null to keep the current one) - * @param int|null $httpPort The HTTP port (null to keep the current one for the same scheme or the default configured port) - * @param int|null $httpsPort The HTTPS port (null to keep the current one for the same scheme or the default configured port) + * @param Request $request The request instance + * @param string $path The absolute path or URL to redirect to + * @param bool $permanent Whether the redirect is permanent or not + * @param string|null $scheme The URL scheme (null to keep the current one) + * @param int|null $httpPort The HTTP port (null to keep the current one for the same scheme or the default configured port) + * @param int|null $httpsPort The HTTPS port (null to keep the current one for the same scheme or the default configured port) + * @param bool $keepRequestMethod Wheter redirect action should keep HTTP request method * * @throws HttpException In case the path is empty */ - public function urlRedirectAction(Request $request, string $path, bool $permanent = false, string $scheme = null, int $httpPort = null, int $httpsPort = null): Response + public function urlRedirectAction(Request $request, string $path, bool $permanent = false, string $scheme = null, int $httpPort = null, int $httpsPort = null, bool $keepRequestMethod = false): Response { if ('' == $path) { - throw new HttpException($permanent ? 410 : 404); + throw new HttpException($permanent ? Response::HTTP_GONE : Response::HTTP_NOT_FOUND); } - $statusCode = $permanent ? 301 : 302; + if ($keepRequestMethod) { + $statusCode = $permanent ? Response::HTTP_PERMANENTLY_REDIRECT : Response::HTTP_TEMPORARY_REDIRECT; + } else { + @trigger_error('Since next major release redirect action will be made with 307/308 HTTP status codes', \E_USER_DEPRECATED); + $statusCode = $permanent ? Response::HTTP_MOVED_PERMANENTLY : Response::HTTP_FOUND; + } // redirect if the path is a full URL if (parse_url($path, PHP_URL_SCHEME)) { diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php index 4a5beeb5b8f71..45ac82e1470a4 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php @@ -33,21 +33,21 @@ public function testEmptyRoute() $controller->redirectAction($request, '', true); $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown'); } catch (HttpException $e) { - $this->assertSame(410, $e->getStatusCode()); + $this->assertSame(Response::HTTP_GONE, $e->getStatusCode()); } try { $controller->redirectAction($request, '', false); $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown'); } catch (HttpException $e) { - $this->assertSame(404, $e->getStatusCode()); + $this->assertSame(Response::HTTP_NOT_FOUND, $e->getStatusCode()); } } /** * @dataProvider provider */ - public function testRoute($permanent, $ignoreAttributes, $expectedCode, $expectedAttributes) + public function testRoute($permanent, $keepRequestMethod, $ignoreAttributes, $expectedCode, $expectedAttributes) { $request = new Request(); @@ -62,6 +62,7 @@ public function testRoute($permanent, $ignoreAttributes, $expectedCode, $expecte 'permanent' => $permanent, 'additional-parameter' => 'value', 'ignoreAttributes' => $ignoreAttributes, + 'keepRequestMethod' => $keepRequestMethod ), ); @@ -76,7 +77,7 @@ public function testRoute($permanent, $ignoreAttributes, $expectedCode, $expecte $controller = new RedirectController($router); - $returnResponse = $controller->redirectAction($request, $route, $permanent, $ignoreAttributes); + $returnResponse = $controller->redirectAction($request, $route, $permanent, $ignoreAttributes, $keepRequestMethod); $this->assertRedirectUrl($returnResponse, $url); $this->assertEquals($expectedCode, $returnResponse->getStatusCode()); @@ -84,12 +85,16 @@ public function testRoute($permanent, $ignoreAttributes, $expectedCode, $expecte public function provider() { - return array( - array(true, false, 301, array('additional-parameter' => 'value')), - array(false, false, 302, array('additional-parameter' => 'value')), - array(false, true, 302, array()), - array(false, array('additional-parameter'), 302, array()), - ); + return [ + [true, false, false, Response::HTTP_MOVED_PERMANENTLY, ['additional-parameter' => 'value']], + [false, false, false, Response::HTTP_FOUND, ['additional-parameter' => 'value']], + [false, false, true, Response::HTTP_FOUND, []], + [false, false, ['additional-parameter'], Response::HTTP_FOUND, []], + [true, true, false, Response::HTTP_PERMANENTLY_REDIRECT, ['additional-parameter' => 'value']], + [false, true, false, Response::HTTP_TEMPORARY_REDIRECT, ['additional-parameter' => 'value']], + [false, true, true, Response::HTTP_TEMPORARY_REDIRECT, []], + [false, true, ['additional-parameter'], Response::HTTP_TEMPORARY_REDIRECT, []], + ]; } public function testEmptyPath() @@ -101,14 +106,14 @@ public function testEmptyPath() $controller->urlRedirectAction($request, '', true); $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown'); } catch (HttpException $e) { - $this->assertSame(410, $e->getStatusCode()); + $this->assertSame(Response::HTTP_GONE, $e->getStatusCode()); } try { $controller->urlRedirectAction($request, '', false); $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown'); } catch (HttpException $e) { - $this->assertSame(404, $e->getStatusCode()); + $this->assertSame(Response::HTTP_NOT_FOUND, $e->getStatusCode()); } } @@ -119,7 +124,17 @@ public function testFullURL() $returnResponse = $controller->urlRedirectAction($request, 'http://foo.bar/'); $this->assertRedirectUrl($returnResponse, 'http://foo.bar/'); - $this->assertEquals(302, $returnResponse->getStatusCode()); + $this->assertEquals(Response::HTTP_FOUND, $returnResponse->getStatusCode()); + } + + 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(Response::HTTP_TEMPORARY_REDIRECT, $returnResponse->getStatusCode()); } public function testUrlRedirectDefaultPorts() From b955a6f5e63260f30ee0a361827fdce9829d8242 Mon Sep 17 00:00:00 2001 From: Pawel Smolinski Date: Sun, 18 Feb 2018 13:58:18 +0000 Subject: [PATCH 2/4] Changelog update, remove deprecation notes --- src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md | 1 + .../Bundle/FrameworkBundle/Controller/RedirectController.php | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md b/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md index 6d615a013f7a0..fb2f781c0010b 100644 --- a/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md +++ b/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md @@ -10,6 +10,7 @@ CHANGELOG * Added option in workflow dump command to label graph with a custom label * Using a `RouterInterface` that does not implement the `WarmableInterface` is deprecated and will not be supported in Symfony 5.0. * The `RequestDataCollector` class has been deprecated and will be removed in Symfony 5.0. Use the `Symfony\Component\HttpKernel\DataCollector\RequestDataCollector` class instead. + * The `RedirectController` class allows for 307/308 HTTP status codes 4.0.0 ----- diff --git a/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php b/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php index 5467ce85eab63..731de2a88d47a 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php +++ b/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php @@ -72,7 +72,6 @@ public function redirectAction(Request $request, string $route, bool $permanent if ($keepRequestMethod) { $statusCode = $permanent ? Response::HTTP_PERMANENTLY_REDIRECT : Response::HTTP_TEMPORARY_REDIRECT; } else { - @trigger_error('Since next major release redirect action will be made with 307/308 HTTP status codes', \E_USER_DEPRECATED); $statusCode = $permanent ? Response::HTTP_MOVED_PERMANENTLY : Response::HTTP_FOUND; } @@ -107,7 +106,6 @@ public function urlRedirectAction(Request $request, string $path, bool $permanen if ($keepRequestMethod) { $statusCode = $permanent ? Response::HTTP_PERMANENTLY_REDIRECT : Response::HTTP_TEMPORARY_REDIRECT; } else { - @trigger_error('Since next major release redirect action will be made with 307/308 HTTP status codes', \E_USER_DEPRECATED); $statusCode = $permanent ? Response::HTTP_MOVED_PERMANENTLY : Response::HTTP_FOUND; } From 6aad61c45dd97f163988b23d1b15d7e7c413c675 Mon Sep 17 00:00:00 2001 From: Pawel Smolinski Date: Sun, 18 Feb 2018 15:14:59 +0000 Subject: [PATCH 3/4] fabbot-guided changes --- .../Controller/RedirectControllerTest.php | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php index 45ac82e1470a4..df5f365817d69 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php @@ -62,7 +62,7 @@ public function testRoute($permanent, $keepRequestMethod, $ignoreAttributes, $ex 'permanent' => $permanent, 'additional-parameter' => 'value', 'ignoreAttributes' => $ignoreAttributes, - 'keepRequestMethod' => $keepRequestMethod + 'keepRequestMethod' => $keepRequestMethod, ), ); @@ -85,16 +85,16 @@ public function testRoute($permanent, $keepRequestMethod, $ignoreAttributes, $ex public function provider() { - return [ - [true, false, false, Response::HTTP_MOVED_PERMANENTLY, ['additional-parameter' => 'value']], - [false, false, false, Response::HTTP_FOUND, ['additional-parameter' => 'value']], - [false, false, true, Response::HTTP_FOUND, []], - [false, false, ['additional-parameter'], Response::HTTP_FOUND, []], - [true, true, false, Response::HTTP_PERMANENTLY_REDIRECT, ['additional-parameter' => 'value']], - [false, true, false, Response::HTTP_TEMPORARY_REDIRECT, ['additional-parameter' => 'value']], - [false, true, true, Response::HTTP_TEMPORARY_REDIRECT, []], - [false, true, ['additional-parameter'], Response::HTTP_TEMPORARY_REDIRECT, []], - ]; + return array( + array(true, false, false, Response::HTTP_MOVED_PERMANENTLY, array('additional-parameter' => 'value')), + array(false, false, false, Response::HTTP_FOUND, array('additional-parameter' => 'value')), + array(false, false, true, Response::HTTP_FOUND, array()), + array(false, false, array('additional-parameter'), Response::HTTP_FOUND, array()), + array(true, true, false, Response::HTTP_PERMANENTLY_REDIRECT, array('additional-parameter' => 'value')), + array(false, true, false, Response::HTTP_TEMPORARY_REDIRECT, array('additional-parameter' => 'value')), + array(false, true, true, Response::HTTP_TEMPORARY_REDIRECT, array()), + array(false, true, array('additional-parameter'), Response::HTTP_TEMPORARY_REDIRECT, array()), + ); } public function testEmptyPath() From 103c1227949112ab45183f151f0449a8096fa30a Mon Sep 17 00:00:00 2001 From: Pawel Smolinski Date: Sun, 18 Feb 2018 15:28:10 +0000 Subject: [PATCH 4/4] Use direct HTTP status codes instead of constants --- .../Controller/RedirectController.php | 12 ++++---- .../Controller/RedirectControllerTest.php | 28 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php b/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php index 731de2a88d47a..1ed2f2a0afff6 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php +++ b/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php @@ -57,7 +57,7 @@ public function __construct(UrlGeneratorInterface $router = null, int $httpPort public function redirectAction(Request $request, string $route, bool $permanent = false, $ignoreAttributes = false, bool $keepRequestMethod = false): Response { if ('' == $route) { - throw new HttpException($permanent ? Response::HTTP_GONE : Response::HTTP_NOT_FOUND); + throw new HttpException($permanent ? 410 : 404); } $attributes = array(); @@ -70,9 +70,9 @@ public function redirectAction(Request $request, string $route, bool $permanent } if ($keepRequestMethod) { - $statusCode = $permanent ? Response::HTTP_PERMANENTLY_REDIRECT : Response::HTTP_TEMPORARY_REDIRECT; + $statusCode = $permanent ? 308 : 307; } else { - $statusCode = $permanent ? Response::HTTP_MOVED_PERMANENTLY : Response::HTTP_FOUND; + $statusCode = $permanent ? 301 : 302; } return new RedirectResponse($this->router->generate($route, $attributes, UrlGeneratorInterface::ABSOLUTE_URL), $statusCode); @@ -100,13 +100,13 @@ public function redirectAction(Request $request, string $route, bool $permanent public function urlRedirectAction(Request $request, string $path, bool $permanent = false, string $scheme = null, int $httpPort = null, int $httpsPort = null, bool $keepRequestMethod = false): Response { if ('' == $path) { - throw new HttpException($permanent ? Response::HTTP_GONE : Response::HTTP_NOT_FOUND); + throw new HttpException($permanent ? 410 : 404); } if ($keepRequestMethod) { - $statusCode = $permanent ? Response::HTTP_PERMANENTLY_REDIRECT : Response::HTTP_TEMPORARY_REDIRECT; + $statusCode = $permanent ? 308 : 307; } else { - $statusCode = $permanent ? Response::HTTP_MOVED_PERMANENTLY : Response::HTTP_FOUND; + $statusCode = $permanent ? 301 : 302; } // redirect if the path is a full URL diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php index df5f365817d69..a90cc86ddf2d3 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php @@ -33,14 +33,14 @@ public function testEmptyRoute() $controller->redirectAction($request, '', true); $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown'); } catch (HttpException $e) { - $this->assertSame(Response::HTTP_GONE, $e->getStatusCode()); + $this->assertSame(410, $e->getStatusCode()); } try { $controller->redirectAction($request, '', false); $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown'); } catch (HttpException $e) { - $this->assertSame(Response::HTTP_NOT_FOUND, $e->getStatusCode()); + $this->assertSame(404, $e->getStatusCode()); } } @@ -86,14 +86,14 @@ public function testRoute($permanent, $keepRequestMethod, $ignoreAttributes, $ex public function provider() { return array( - array(true, false, false, Response::HTTP_MOVED_PERMANENTLY, array('additional-parameter' => 'value')), - array(false, false, false, Response::HTTP_FOUND, array('additional-parameter' => 'value')), - array(false, false, true, Response::HTTP_FOUND, array()), - array(false, false, array('additional-parameter'), Response::HTTP_FOUND, array()), - array(true, true, false, Response::HTTP_PERMANENTLY_REDIRECT, array('additional-parameter' => 'value')), - array(false, true, false, Response::HTTP_TEMPORARY_REDIRECT, array('additional-parameter' => 'value')), - array(false, true, true, Response::HTTP_TEMPORARY_REDIRECT, array()), - array(false, true, array('additional-parameter'), Response::HTTP_TEMPORARY_REDIRECT, array()), + array(true, false, false, 301, array('additional-parameter' => 'value')), + array(false, false, false, 302, array('additional-parameter' => 'value')), + array(false, false, true, 302, array()), + array(false, false, array('additional-parameter'), 302, array()), + array(true, true, false, 308, array('additional-parameter' => 'value')), + array(false, true, false, 307, array('additional-parameter' => 'value')), + array(false, true, true, 307, array()), + array(false, true, array('additional-parameter'), 307, array()), ); } @@ -106,14 +106,14 @@ public function testEmptyPath() $controller->urlRedirectAction($request, '', true); $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown'); } catch (HttpException $e) { - $this->assertSame(Response::HTTP_GONE, $e->getStatusCode()); + $this->assertSame(410, $e->getStatusCode()); } try { $controller->urlRedirectAction($request, '', false); $this->fail('Expected Symfony\Component\HttpKernel\Exception\HttpException to be thrown'); } catch (HttpException $e) { - $this->assertSame(Response::HTTP_NOT_FOUND, $e->getStatusCode()); + $this->assertSame(404, $e->getStatusCode()); } } @@ -124,7 +124,7 @@ public function testFullURL() $returnResponse = $controller->urlRedirectAction($request, 'http://foo.bar/'); $this->assertRedirectUrl($returnResponse, 'http://foo.bar/'); - $this->assertEquals(Response::HTTP_FOUND, $returnResponse->getStatusCode()); + $this->assertEquals(302, $returnResponse->getStatusCode()); } public function testFullURLWithMethodKeep() @@ -134,7 +134,7 @@ public function testFullURLWithMethodKeep() $returnResponse = $controller->urlRedirectAction($request, 'http://foo.bar/', false, null, null, null, true); $this->assertRedirectUrl($returnResponse, 'http://foo.bar/'); - $this->assertEquals(Response::HTTP_TEMPORARY_REDIRECT, $returnResponse->getStatusCode()); + $this->assertEquals(307, $returnResponse->getStatusCode()); } public function testUrlRedirectDefaultPorts()