From 19ad413d5dbbe15cfd6c5860f694e07e9abf8031 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Thu, 20 Jun 2019 17:17:37 +0200 Subject: [PATCH] ResponseFactory: support 302 redirects Needed to match existing Parsoid behavior. Also fixes redirect factory methods mistaking claiming to support relative URLs. Most clients accept a relative URL in the Location header, but the spec requires an absolute one, so better say that. Change-Id: I03f5e776f7629eff6440698655277d8cd01e4a15 --- includes/Rest/ResponseFactory.php | 22 ++++++++++++++++--- .../includes/Rest/ResponseFactoryTest.php | 7 ++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/includes/Rest/ResponseFactory.php b/includes/Rest/ResponseFactory.php index 7ccb612748..d18cdb5d6b 100644 --- a/includes/Rest/ResponseFactory.php +++ b/includes/Rest/ResponseFactory.php @@ -78,7 +78,7 @@ class ResponseFactory { * the new URL in the future. 301 redirects tend to get cached and are hard to undo. * Client behavior for methods other than GET/HEAD is not well-defined and this type * of response should be avoided in such cases. - * @param string $target Redirect URL (can be relative) + * @param string $target Redirect target (an absolute URL) * @return Response */ public function createPermanentRedirect( $target ) { @@ -87,12 +87,28 @@ class ResponseFactory { return $response; } + /** + * Creates a temporary (302) redirect. + * HTTP 302 was underspecified and has been superseded by 303 (when the redirected request + * should be a GET, regardless of what the current request is) and 307 (when the method should + * not be changed), but might still be needed for HTTP 1.0 clients or to match legacy behavior. + * @param string $target Redirect target (an absolute URL) + * @return Response + * @see self::createTemporaryRedirect() + * @see self::createSeeOther() + */ + public function createLegacyTemporaryRedirect( $target ) { + $response = $this->createRedirectBase( $target ); + $response->setStatus( 302 ); + return $response; + } + /** * Creates a temporary (307) redirect. * This indicates that the operation the client was trying to perform can temporarily * be achieved by using a different URL. Clients will preserve the request method when * retrying the request with the new URL. - * @param string $target Redirect URL (can be relative) + * @param string $target Redirect target (an absolute URL) * @return Response */ public function createTemporaryRedirect( $target ) { @@ -106,7 +122,7 @@ class ResponseFactory { * This indicates that the target resource might be of interest to the client, without * necessarily implying that it is the same resource. The client will always use GET * (or HEAD) when following the redirection. Useful for GET-after-POST. - * @param string $target Redirect URL (can be relative) + * @param string $target Redirect target (an absolute URL) * @return Response */ public function createSeeOther( $target ) { diff --git a/tests/phpunit/includes/Rest/ResponseFactoryTest.php b/tests/phpunit/includes/Rest/ResponseFactoryTest.php index 6ccacda717..ae71272f6b 100644 --- a/tests/phpunit/includes/Rest/ResponseFactoryTest.php +++ b/tests/phpunit/includes/Rest/ResponseFactoryTest.php @@ -49,6 +49,13 @@ class ResponseFactoryTest extends MediaWikiTestCase { $this->assertSame( 301, $response->getStatusCode() ); } + public function testCreateLegacyTemporaryRedirect() { + $rf = new ResponseFactory; + $response = $rf->createLegacyTemporaryRedirect( 'http://www.example.com/' ); + $this->assertSame( [ 'http://www.example.com/' ], $response->getHeader( 'Location' ) ); + $this->assertSame( 302, $response->getStatusCode() ); + } + public function testCreateTemporaryRedirect() { $rf = new ResponseFactory; $response = $rf->createTemporaryRedirect( 'http://www.example.com/' ); -- 2.20.1