Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.x New Dispatcher & Routing Results #2405

Merged
merged 12 commits into from
May 24, 2018
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions Slim/App.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Message\UriInterface;
use Slim\Exception\HttpMethodNotAllowedException;
use Slim\Exception\HttpNotFoundException;
use Slim\Http\Headers;
use Slim\Http\Request;
use Slim\Http\Response;
Expand Down Expand Up @@ -502,23 +504,25 @@ public function respond(ResponseInterface $response)
* @param ResponseInterface $response The most recent Response object
*
* @return ResponseInterface
*
* @throws HttpNotFoundException
* @throws HttpMethodNotAllowedException
*/
public function __invoke(ServerRequestInterface $request, ResponseInterface $response)
{
// Get the route info
$routeInfo = $request->getAttribute('routeInfo');

/** @var \Slim\Interfaces\RouterInterface $router */
$router = $this->getRouter();
/**
* @var DispatcherResults $dispatcherResults
*/
$dispatcherResults = $request->getAttribute('dispatcherResults');

// If routing hasn't been done, then do it now so we can dispatch
if ($routeInfo === null) {
if ($dispatcherResults === null) {
$router = $this->getRouter();
$routingMiddleware = new RoutingMiddleware($router);
$request = $routingMiddleware->performRouting($request);
$routeInfo = $request->getAttribute('routeInfo');
}

$route = $router->lookupRoute($routeInfo[1]);
$route = $request->getAttribute('route');
return $route->run($request, $response);
}

Expand Down
132 changes: 132 additions & 0 deletions Slim/Dispatcher.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
<?php
/**
* Slim Framework (https://slimframework.com)
*
* @link https://github.com/slimphp/Slim
* @copyright Copyright (c) 2011-2018 Josh Lockhart
* @license https://github.com/slimphp/Slim/blob/4.x/LICENSE.md (MIT License)
*/
namespace Slim;

use FastRoute\Dispatcher\GroupCountBased;

class Dispatcher extends GroupCountBased
{
/**
* @var string
*/
private $httpMethod;

/**
* @var string
*/
private $uri;

/**
* @var array|null
*/
private $allowedMethods;

/**
* @param string $httpMethod
* @param string $uri
* @return DispatcherResults
*/
public function dispatch($httpMethod, $uri)
{
$this->httpMethod = $httpMethod;
$this->uri = $uri;

if (isset($this->staticRouteMap[$httpMethod][$uri])) {
return $this->dispatcherResults(self::FOUND, $this->staticRouteMap[$httpMethod][$uri]);
}

$varRouteData = $this->variableRouteData;
if (isset($varRouteData[$httpMethod])) {
$result = $this->dispatchVariableRoute($varRouteData[$httpMethod], $uri);
$dispatcherResults = $this->dispatcherResultsFromVariableRouteResults($result);
if ($dispatcherResults->getRouteStatus() === Dispatcher::FOUND) {
return $dispatcherResults;
}
}

// For HEAD requests, attempt fallback to GET
if ($httpMethod === 'HEAD') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdyt about adding https://github.com/php-fig/http-message-util to avoid hardcoded HTTP methods?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP Methods don't change. I dont think we need to include a package for constants, we aren't javascript.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always include it in for descriptive HTTP status codes and it contains have HTTP methods too.
Seen that is a FIG package I'm ok with it.
But I get your point of view so it's not a big deal 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anywhere, this should be on the Slim\Http package.

if (isset($this->staticRouteMap['GET'][$uri])) {
return $this->dispatcherResults(self::FOUND, $this->staticRouteMap['GET'][$uri]);
}
if (isset($varRouteData['GET'])) {
$result = $this->dispatchVariableRoute($varRouteData['GET'], $uri);
return $this->dispatcherResultsFromVariableRouteResults($result);
}
}

// If nothing else matches, try fallback routes
if (isset($this->staticRouteMap['*'][$uri])) {
return $this->dispatcherResults(self::FOUND, $this->staticRouteMap['*'][$uri]);
}
if (isset($varRouteData['*'])) {
$result = $this->dispatchVariableRoute($varRouteData['*'], $uri);
return $this->dispatcherResultsFromVariableRouteResults($result);
}

$this->allowedMethods = $this->getAllowedMethods($httpMethod, $uri);
if (count($this->allowedMethods)) {
return $this->dispatcherResults(self::METHOD_NOT_ALLOWED);
}

return $this->dispatcherResults(self::NOT_FOUND);
}

/**
* @param $status
* @param null $handler
* @param array $arguments
* @return DispatcherResults
*/
protected function dispatcherResults($status, $handler = null, $arguments = [])
{
return new DispatcherResults($this, $this->httpMethod, $this->uri, $status, $handler, $arguments);
}

/**
* @param array $result
* @return DispatcherResults
*/
protected function dispatcherResultsFromVariableRouteResults($result)
{
if ($result[0] === self::FOUND) {
return $this->dispatcherResults(self::FOUND, $result[1], $result[2]);
}
return $this->dispatcherResults(self::NOT_FOUND);
}

/**
* @param string $httpMethod
* @param string $uri
* @return array
*/
public function getAllowedMethods($httpMethod, $uri)
{
if ($this->allowedMethods !== null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will give the wrong answer if I do this:

$methods1 = $o->getAllowedMethods('POST', '/foo');
$methods2 = $o->getAllowedMethods('GET', '/bar');

At least, I think that $methods2 will be the allowed methods for /foo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, I made a mistake. I fixed this logic in latest commit. Good find!

return $this->allowedMethods;
}

$this->allowedMethods = [];
foreach ($this->staticRouteMap as $method => $uriMap) {
if ($method !== $httpMethod && isset($uriMap[$uri])) {
$this->allowedMethods[] = $method;
}
}

$varRouteData = $this->variableRouteData;
foreach ($varRouteData as $method => $routeData) {
$result = $this->dispatchVariableRoute($routeData, $uri);
if ($result[0] === self::FOUND) {
$this->allowedMethods[] = $method;
}
}

return $this->allowedMethods;
}
}
139 changes: 139 additions & 0 deletions Slim/DispatcherResults.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
<?php
/**
* Slim Framework (https://slimframework.com)
*
* @link https://github.com/slimphp/Slim
* @copyright Copyright (c) 2011-2018 Josh Lockhart
* @license https://github.com/slimphp/Slim/blob/4.x/LICENSE.md (MIT License)
*/
namespace Slim;

class DispatcherResults
{
/**
* @var Dispatcher
*/
protected $dispatcher;

/**
* @var string
*/
protected $httpMethod;

/**
* @var string
*/
protected $uri;

/**
* @var int
* The following statuses are constants from Slim\Dispatcher
* Slim\Dispatcher extends FastRoute\Dispatcher\GroupCountBased
* Which implements the interface FastRoute\Dispatcher where the original constants are located
* Slim\Dispatcher::NOT_FOUND = 0
* Slim\Dispatcher::FOUND = 1
* Slim\Dispatcher::NOT_ALLOWED = 2
*/
protected $routeStatus;

/**
* @var callable|null
*/
protected $routeHandler;

/**
* @var array
*/
protected $routeArguments;

/**
* DispatcherResults constructor.
* @param Dispatcher $dispatcher
* @param $httpMethod
* @param $uri
* @param int $routeStatus
* @param callable|null $routeHandler
* @param array $routeArguments
*/
public function __construct(
Dispatcher $dispatcher,
$httpMethod,
$uri,
$routeStatus,
$routeHandler = null,
$routeArguments = []
) {
$this->dispatcher = $dispatcher;
$this->httpMethod = $httpMethod;
$this->uri = $uri;
$this->routeStatus = $routeStatus;
$this->routeHandler = $routeHandler;
$this->routeArguments = $routeArguments;
}

/**
* @return mixed
*/
public function getDispatcher()
{
return $this->dispatcher;
}

/**
* @return string
*/
public function getHttpMethod()
{
return $this->httpMethod;
}

/**
* @return string
*/
public function getUri()
{
return $this->uri;
}

/**
* @return int
*/
public function getRouteStatus()
{
return $this->routeStatus;
}

/**
* @return callable|null
*/
public function getRouteHandler()
{
return $this->routeHandler;
}

/**
* @param bool $urlDecode
* @return array
*/
public function getRouteArguments($urlDecode = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we not want to url decode the route arguments?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that currently in Slim 3 it's not optional. Why is the choice a method parameter here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per our discussion on slack, we're leaving this as is.

{
if (!$urlDecode) {
return $this->routeArguments;
}

$routeArguments = [];
foreach ($this->routeArguments as $key => $value) {
$routeArguments[$key] = urldecode($value);
}

return $routeArguments;
}

/**
* @return array
*/
public function getAllowedMethods()
{
return $this->dispatcher->getAllowedMethods($this->httpMethod, $this->uri);
}
}
2 changes: 1 addition & 1 deletion Slim/Exception/HttpMethodNotAllowedException.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class HttpMethodNotAllowedException extends HttpSpecializedException
protected $description = 'The request method is not supported for the requested resource.';

/**
* @return string
* @return array
*/
public function getAllowedMethods()
{
Expand Down
3 changes: 2 additions & 1 deletion Slim/Interfaces/RouterInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use RuntimeException;
use InvalidArgumentException;
use Psr\Http\Message\ServerRequestInterface;
use Slim\DispatcherResults;

/**
* Router Interface
Expand Down Expand Up @@ -40,7 +41,7 @@ public function map($methods, $pattern, $handler);
*
* @param ServerRequestInterface $request The current HTTP request object
*
* @return array
* @return DispatcherResults
*
* @link https://github.com/nikic/FastRoute/blob/master/src/Dispatcher.php
*/
Expand Down
Loading