Merge pull request #43 from clue/redirect-handler

Refactor to move redirects to new `RedirectHandler`

docs/api/app.md

@@ -40,9 +40,9 @@ which also highlights how you can use [request attributes](request.md#attributes
 to access values from URI templates.
 
 An HTTP `GET` request for `/foo` would automatically reject the HTTP request with
-a 404 (Not Found) error response unless this route is registered.
-Likewise, an HTTP `POST` request for `/user` would reject with a 405 (Method Not
-Allowed) error response unless a route for this method is also registered.
+a `404 Not Found` error response unless this route is registered.
+Likewise, an HTTP `POST` request for `/user` would reject with a `405 Method Not
+Allowed` error response unless a route for this method is also registered.
 
 You can route any number of incoming HTTP requests to controller functions by
 using the matching API methods like this:
@@ -71,6 +71,27 @@ you can use this additional shortcut:
 $app->any('/user/{id}', $controller);
 ```
 
+## Redirects
+
+The `App` also offers a convenient helper method to redirect a matching route to
+a new URL like this:
+
+```php
+$app->redirect('/promo/reactphp', 'http://reactphp.org/');
+```
+
+Browsers and search engine crawlers will automatically follow the redirect with
+the `302 Found` status code by default. You can optionally pass a custom redirect
+status code in the `3xx` range to use. If this is a permanent redirect, you may
+want to use the `301 Permanent Redirect` status code to instruct search engine
+crawlers to update their index like this:
+
+```php
+$app->redirect('/blog.html', '/blog', 301);
+```
+
+See [response status codes](response.md#status-codes) for more details.
+
 ## Controllers
 
 The above examples use inline closures as controller functions to make these

docs/api/response.md

@@ -164,12 +164,12 @@ Each HTTP response message contains a status code that describes whether the
 HTTP request has been successfully completed.
 Here's a list of the most common HTTP status codes:
 
-* 200 (OK)
-* 301 (Permanent Redirect)
-* 302 (Temporary Redirect)
-* 403 (Forbidden)
-* 404 (Not Found)
-* 500 (Internal Server Error)
+* `200 OK`
+* `301 Permanent Redirect`
+* `302 Found` (previously `302 Temporary Redirect`)
+* `403 Forbidden`
+* `404 Not Found`
+* `500 Internal Server Error`
 * …
 
 See [list of HTTP status codes](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status) for more details.
@@ -210,7 +210,7 @@ know what you're doing.
 Each controller function needs to return a response object in order to send
 an HTTP response message.
 If the controller functions throws an `Exception` (or `Throwable`) or any other type, the
-HTTP request will automatically be rejected with a 500 (Internal Server Error)
+HTTP request will automatically be rejected with a `500 Internal Server Error`
 HTTP error response:
 
 ```php

src/App.php

@@ -7,7 +7,6 @@
 use React\EventLoop\Loop;
 use React\EventLoop\LoopInterface;
 use React\Http\HttpServer;
-use React\Http\Message\Response;
 use React\Http\Message\ServerRequest;
 use React\Promise\Deferred;
 use React\Promise\PromiseInterface;
@@ -113,18 +112,9 @@ public function map(array $methods, string $route, callable $handler, callable .
         $this->router->map($methods, $route, $handler, ...$handlers);
     }
 
-    public function redirect($route, $target, $code = 302)
+    public function redirect(string $route, string $target, int $code = 302): void
     {
-        return $this->get($route, function () use ($target, $code) {
-            return new Response(
-                $code,
-                [
-                    'Content-Type' => 'text/html',
-                    'Location' => $target
-                ],
-                'See ' . $target . '...' . "\n"
-            );
-        });
+        $this->any($route, new RedirectHandler($target, $code));
     }
 
     public function run()

src/FilesystemHandler.php

@@ -68,12 +68,7 @@ public function __invoke(ServerRequestInterface $request)
         \clearstatcache();
         if ($valid && \is_dir($path)) {
             if ($local !== '' && \substr($local, -1) !== '/') {
-                return new Response(
-                    302,
-                    [
-                        'Location' => \basename($path) . '/'
-                    ]
-                );
+                return (new RedirectHandler(\basename($path) . '/'))();
             }
 
             $response = '<strong>' . $this->html->escape($local === '' ? '/' : $local) . '</strong>' . "\n<ul>\n";
@@ -102,12 +97,7 @@ public function __invoke(ServerRequestInterface $request)
             );
         } elseif ($valid && \is_file($path)) {
             if ($local !== '' && \substr($local, -1) === '/') {
-                return new Response(
-                    302,
-                    [
-                        'Location' => '../' . \basename($path)
-                    ]
-                );
+                return (new RedirectHandler('../' . \basename($path)))();
             }
 
             // Assign MIME type based on file extension (same as nginx/Apache) or fall back to given default otherwise.

src/HtmlHandler.php

@@ -25,6 +25,7 @@ public function statusResponse(int $statusCode, string $title, string $subtitle,
 strong { color: #111827; font-size: 3em; }
 p { margin: .5em 0 0 0; grid-column: 2; color: #6b7280; }
 code { padding: 0 .3em; background-color: #f5f6f9; }
+a { color: inherit; }
 </style>
 </head>
 <body>

src/RedirectHandler.php

@@ -0,0 +1,43 @@
+<?php
+
+namespace FrameworkX;
+
+use Psr\Http\Message\ResponseInterface;
+use React\Http\Message\Response;
+
+/**
+ * @internal
+ */
+class RedirectHandler
+{
+    private $target;
+    private $code;
+    private $reason;
+
+    /** @var HtmlHandler */
+    private $html;
+
+    public function __construct(string $target, int $redirectStatusCode = 302)
+    {
+        if ($redirectStatusCode < 300 || $redirectStatusCode === 304 || $redirectStatusCode >= 400) {
+            throw new \InvalidArgumentException('Invalid redirect status code given');
+        }
+
+        $this->target = $target;
+        $this->code = $redirectStatusCode;
+        $this->reason = \ucwords((new Response($redirectStatusCode))->getReasonPhrase()) ?: 'Redirect';
+        $this->html = new HtmlHandler();
+    }
+
+    public function __invoke(): ResponseInterface
+    {
+        $url = $this->html->escape($this->target);
+
+        return $this->html->statusResponse(
+            $this->code,
+            'Redirecting to ' . $url,
+            $this->reason,
+            "<p>Redirecting to <a href=\"$url\"><code>$url</code></a>...</p>\n"
+        )->withHeader('Location', $this->target);
+    }
+}

tests/AppTest.php

@@ -282,13 +282,13 @@ public function testMapMethodAddsRouteOnRouter()
         $app->map(['GET', 'POST'], '/', function () { });
     }
 
-    public function testRedirectMethodAddsGetRouteOnRouterWhichWhenInvokedReturnsRedirectResponseWithTargetLocation()
+    public function testRedirectMethodAddsAnyRouteOnRouterWhichWhenInvokedReturnsRedirectResponseWithTargetLocation()
     {
         $app = new App();
 
         $handler = null;
         $router = $this->createMock(RouteHandler::class);
-        $router->expects($this->once())->method('map')->with(['GET'], '/', $this->callback(function ($fn) use (&$handler) {
+        $router->expects($this->once())->method('map')->with(['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], '/', $this->callback(function ($fn) use (&$handler) {
             $handler = $fn;
             return true;
         }));
@@ -305,19 +305,22 @@ public function testRedirectMethodAddsGetRouteOnRouterWhichWhenInvokedReturnsRed
 
         /** @var ResponseInterface $response */
         $this->assertInstanceOf(ResponseInterface::class, $response);
+        $this->assertEquals('text/html; charset=utf-8', $response->getHeaderLine('Content-Type'));
+        $this->assertStringMatchesFormat("<!DOCTYPE html>\n<html>%a</html>\n", (string) $response->getBody());
+
         $this->assertEquals(302, $response->getStatusCode());
-        $this->assertEquals('text/html', $response->getHeaderLine('Content-Type'));
         $this->assertEquals('/users', $response->getHeaderLine('Location'));
-        $this->assertEquals("See /users...\n", (string) $response->getBody());
+        $this->assertStringContainsString("<title>Redirecting to /users</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Redirecting to <a href=\"/users\"><code>/users</code></a>...</p>\n", (string) $response->getBody());
     }
 
-    public function testRedirectMethodWithCustomRedirectCodeAddsGetRouteOnRouterWhichWhenInvokedReturnsRedirectResponseWithCustomRedirectCode()
+    public function testRedirectMethodWithCustomRedirectCodeAddsAnyRouteOnRouterWhichWhenInvokedReturnsRedirectResponseWithCustomRedirectCode()
     {
         $app = new App();
 
         $handler = null;
         $router = $this->createMock(RouteHandler::class);
-        $router->expects($this->once())->method('map')->with(['GET'], '/', $this->callback(function ($fn) use (&$handler) {
+        $router->expects($this->once())->method('map')->with(['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], '/', $this->callback(function ($fn) use (&$handler) {
             $handler = $fn;
             return true;
         }));
@@ -334,10 +337,13 @@ public function testRedirectMethodWithCustomRedirectCodeAddsGetRouteOnRouterWhic
 
         /** @var ResponseInterface $response */
         $this->assertInstanceOf(ResponseInterface::class, $response);
+        $this->assertEquals('text/html; charset=utf-8', $response->getHeaderLine('Content-Type'));
+        $this->assertStringMatchesFormat("<!DOCTYPE html>\n<html>%a</html>\n", (string) $response->getBody());
+
         $this->assertEquals(307, $response->getStatusCode());
-        $this->assertEquals('text/html', $response->getHeaderLine('Content-Type'));
         $this->assertEquals('/users', $response->getHeaderLine('Location'));
-        $this->assertEquals("See /users...\n", (string) $response->getBody());
+        $this->assertStringContainsString("<title>Redirecting to /users</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Redirecting to <a href=\"/users\"><code>/users</code></a>...</p>\n", (string) $response->getBody());
     }
 
     public function testRequestFromGlobalsWithNoServerVariablesDefaultsToGetRequestToLocalhost()

tests/FilesystemHandlerTest.php

@@ -267,8 +267,13 @@ public function testInvokeWithValidPathToDirectoryButWithoutTrailingSlashWillRet
 
         /** @var ResponseInterface $response */
         $this->assertInstanceOf(ResponseInterface::class, $response);
+        $this->assertEquals('text/html; charset=utf-8', $response->getHeaderLine('Content-Type'));
+        $this->assertStringMatchesFormat("<!DOCTYPE html>\n<html>%a</html>\n", (string) $response->getBody());
+
         $this->assertEquals(302, $response->getStatusCode());
         $this->assertEquals('.github/', $response->getHeaderLine('Location'));
+        $this->assertStringContainsString("<title>Redirecting to .github/</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Redirecting to <a href=\".github/\"><code>.github/</code></a>...</p>\n", (string) $response->getBody());
     }
 
     public function testInvokeWithValidPathToFileButWithTrailingSlashWillReturnRedirectToPathWithoutSlash()
@@ -282,7 +287,12 @@ public function testInvokeWithValidPathToFileButWithTrailingSlashWillReturnRedir
 
         /** @var ResponseInterface $response */
         $this->assertInstanceOf(ResponseInterface::class, $response);
+        $this->assertEquals('text/html; charset=utf-8', $response->getHeaderLine('Content-Type'));
+        $this->assertStringMatchesFormat("<!DOCTYPE html>\n<html>%a</html>\n", (string) $response->getBody());
+
         $this->assertEquals(302, $response->getStatusCode());
         $this->assertEquals('../LICENSE', $response->getHeaderLine('Location'));
+        $this->assertStringContainsString("<title>Redirecting to ../LICENSE</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Redirecting to <a href=\"../LICENSE\"><code>../LICENSE</code></a>...</p>\n", (string) $response->getBody());
     }
 }

tests/RedirectHandlerTest.php

@@ -0,0 +1,100 @@
+<?php
+
+namespace Framework\Tests;
+
+use FrameworkX\RedirectHandler;
+use PHPUnit\Framework\TestCase;
+use Psr\Http\Message\ResponseInterface;
+
+class RedirectHandlerTest extends TestCase
+{
+    public function testInvokeReturnsResponseWithRedirectToGivenLocation()
+    {
+        $handler = new RedirectHandler('http://example.com/');
+
+        $response = $handler();
+
+        /** @var ResponseInterface $response */
+        $this->assertInstanceOf(ResponseInterface::class, $response);
+        $this->assertEquals('text/html; charset=utf-8', $response->getHeaderLine('Content-Type'));
+        $this->assertStringMatchesFormat("<!DOCTYPE html>\n<html>%a</html>\n", (string) $response->getBody());
+        $this->assertStringMatchesFormat("%a<style nonce=\"%s\">\n%a", (string) $response->getBody());
+        $this->assertStringMatchesFormat('style-src \'nonce-%s\'; img-src \'self\'; default-src \'none\'', $response->getHeaderLine('Content-Security-Policy'));
+
+        $this->assertEquals(302, $response->getStatusCode());
+        $this->assertEquals('http://example.com/', $response->getHeaderLine('Location'));
+        $this->assertStringContainsString("<title>Redirecting to http://example.com/</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<h1>302</h1>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<strong>Found</strong>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Redirecting to <a href=\"http://example.com/\"><code>http://example.com/</code></a>...</p>\n", (string) $response->getBody());
+    }
+
+    public function testInvokeReturnsResponseWithPermanentRedirectToGivenLocationAndCode()
+    {
+        $handler = new RedirectHandler('/', 301);
+
+        $response = $handler();
+
+        /** @var ResponseInterface $response */
+        $this->assertInstanceOf(ResponseInterface::class, $response);
+
+        $this->assertEquals(301, $response->getStatusCode());
+        $this->assertEquals('/', $response->getHeaderLine('Location'));
+        $this->assertStringContainsString("<title>Redirecting to /</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<h1>301</h1>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<strong>Moved Permanently</strong>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Redirecting to <a href=\"/\"><code>/</code></a>...</p>\n", (string) $response->getBody());
+    }
+
+    public function testInvokeReturnsResponseWithCustomRedirectStatusCodeAndGivenLocation()
+    {
+        $handler = new RedirectHandler('/', 399);
+
+        $response = $handler();
+
+        /** @var ResponseInterface $response */
+        $this->assertInstanceOf(ResponseInterface::class, $response);
+
+        $this->assertEquals(399, $response->getStatusCode());
+        $this->assertEquals('/', $response->getHeaderLine('Location'));
+        $this->assertStringContainsString("<title>Redirecting to /</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<h1>399</h1>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<strong>Redirect</strong>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Redirecting to <a href=\"/\"><code>/</code></a>...</p>\n", (string) $response->getBody());
+    }
+
+    public function testInvokeReturnsResponseWithRedirectToGivenLocationWithSpecialCharsEncoded()
+    {
+        $handler = new RedirectHandler('/hello%20w%7Frld?a=1&b=2');
+
+        $response = $handler();
+
+        /** @var ResponseInterface $response */
+        $this->assertInstanceOf(ResponseInterface::class, $response);
+
+        $this->assertEquals(302, $response->getStatusCode());
+        $this->assertEquals('/hello%20w%7Frld?a=1&b=2', $response->getHeaderLine('Location'));
+        $this->assertStringContainsString("<title>Redirecting to /hello%20w%7Frld?a=1&amp;b=2</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<h1>302</h1>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<strong>Found</strong>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Redirecting to <a href=\"/hello%20w%7Frld?a=1&amp;b=2\"><code>/hello%20w%7Frld?a=1&amp;b=2</code></a>...</p>\n", (string) $response->getBody());
+    }
+
+    public function testConstructWithSuccessCodeThrows()
+    {
+        $this->expectException(\InvalidArgumentException::class);
+        new RedirectHandler('/', 200);
+    }
+
+    public function testConstructWithNotModifiedCodeThrows()
+    {
+        $this->expectException(\InvalidArgumentException::class);
+        new RedirectHandler('/', 304);
+    }
+
+    public function testConstructWithBadRequestCodeThrows()
+    {
+        $this->expectException(\InvalidArgumentException::class);
+        new RedirectHandler('/', 400);
+    }
+}

tests/acceptance.sh

@@ -71,14 +71,14 @@ out=$(curl -v $base/users/ 2>&1);    match "HTTP/.* 404"
 out=$(curl -v $base/users/a/b 2>&1); match "HTTP/.* 404"
 
 out=$(curl -v $base/LICENSE 2>&1);              match "HTTP/.* 200" && match -iP "Content-Type: text/plain[\r\n]"
-out=$(curl -v $base/source 2>&1);               match -i "Location: /source/" && match -iP "Content-Type: text/html[\r\n]"
+out=$(curl -v $base/source 2>&1);               match -i "Location: /source/" && match -iP "Content-Type: text/html; charset=utf-8[\r\n]"
 out=$(curl -v $base/source/ 2>&1);              match "HTTP/.* 200"
 out=$(curl -v $base/source/composer.json 2>&1); match "HTTP/.* 200" && match -iP "Content-Type: application/json[\r\n]"
 out=$(curl -v $base/source/LICENSE 2>&1);       match "HTTP/.* 200" && match -iP "Content-Type: text/plain[\r\n]"
-out=$(curl -v $base/source/LICENSE/ 2>&1);      match -i "Location: ../LICENSE"
+out=$(curl -v $base/source/LICENSE/ 2>&1);      match -i "Location: ../LICENSE" && match -iP "Content-Type: text/html; charset=utf-8[\r\n]"
 out=$(curl -v $base/source/LICENSE// 2>&1);     match "HTTP/.* 404"
 out=$(curl -v $base/source//LICENSE 2>&1);      match "HTTP/.* 404"
-out=$(curl -v $base/source/tests 2>&1);         match -i "Location: tests/"
+out=$(curl -v $base/source/tests 2>&1);         match -i "Location: tests/" && match -iP "Content-Type: text/html; charset=utf-8[\r\n]"
 out=$(curl -v $base/source/invalid 2>&1);       match "HTTP/.* 404"
 out=$(curl -v $base/source/bin%00ary 2>&1);     match "HTTP/.* 40[40]" # expects 404, but not processed with nginx (400) and Apache (404)