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

Conversation

Projects
None yet
5 participants
@l0gicgate
Copy link
Contributor

commented Feb 23, 2018

As per #2399

As discussed on Slack, there are a few issues with the way FastRoute\Dispatcher returns information after a route has been dispatched.

Issues

  • FastRoute\Dispatcher::dispatch() returns an array with an inconsistent structure
  • It only appends allowed methods to the return value when no allowed methods have been found on the dispatched route.

Changes

  • routeInfo now deprecated and replaced with routingResults
  • RoutingResults is an immutable object instantiated by the Slim Dispatcher which now offers a way to get all a route's allowed methods which was not accessible before via FastRoute\GroupCountBased\Dispatcher

Usage

use Slim\App;

$app = new App();
$app->get('/foo', function (Request $request, Response $response) {
    $routingResults = $request->getAttribute('routingResults');
    $uri = $routingResults->getUri();
    $method = $routingResults->getMethod();
    $routeArguments = $routingResults->getRouteArguments();
    $allowedMethods = $routingResults->getAllowedMethods();
});

Status

  • Implement new Dispatcher & Routing Results
  • Implement FastRoute test suite for Dispatcher
  • Code Review & Discussion
@coveralls

This comment has been minimized.

Copy link

commented Feb 23, 2018

Coverage Status

Coverage increased (+0.2%) to 96.262% when pulling c58a2bb on l0gicgate:4.x-DispatcherResults into 77daca6 on slimphp:4.x.

@akrabat akrabat added the Slim 4 label Feb 25, 2018

@l0gicgate l0gicgate force-pushed the l0gicgate:4.x-DispatcherResults branch from 0704f28 to e2d52a2 Mar 13, 2018

@l0gicgate l0gicgate force-pushed the l0gicgate:4.x-DispatcherResults branch from e2d52a2 to 4ffbf27 May 15, 2018

@@ -266,11 +263,13 @@ protected function createDispatcher()
if ($this->cacheFile) {
$this->dispatcher = \FastRoute\cachedDispatcher($routeDefinitionCallback, [
'dispatcher' => '\\Slim\\Dispatcher',

This comment has been minimized.

Copy link
@geggleto

geggleto May 16, 2018

Contributor

We should use the class constant here

This comment has been minimized.

Copy link
@geggleto

geggleto May 16, 2018

Contributor

This however makes the app not extendable and a user cannot replace the dispatcher

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate May 16, 2018

Author Contributor

Actually the Router can be extended and this method overridden. When instantiating the RoutingMiddleware you can pass a router to it which could be the router of your choice. An alternative would be to pass settings to the Router constructor.

/**
* @var int
*
* Not Found = 0

This comment has been minimized.

Copy link
@damianopetrungaro

damianopetrungaro May 16, 2018

What do you think about using an Enum for this $routeStatus or if it's an overhead use constants instead of comments for valid statuses?

This comment has been minimized.

Copy link
@geggleto

geggleto May 16, 2018

Contributor

It probably should be a class constant

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate May 16, 2018

Author Contributor

The routeStatus is what comes from Fastroute. We can convert those to constants. Should the constants be on the Dispatcher or Dispatcher results and what should they be named? FOUND, NOT_FOUND, NOT_ALLOWED?

This comment has been minimized.

Copy link
@damianopetrungaro

damianopetrungaro May 16, 2018

Yup, class constant or Enum will be more explicit so for me both are good.

This comment has been minimized.

Copy link
@damianopetrungaro

damianopetrungaro May 16, 2018

1: FOUND, NOT_FOUND, NOT_ALLOWED sounds good to me.
2: You use them in DispatcherResults so it may be the right place (imho use an Enum and inject it is the best approach)

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate May 16, 2018

Author Contributor

Actually, no need to make changes. This PR extends FastRoute\Dispatcher\GroupCountBased class which implements FastRoute\Dispatcher interface which has those constants. They can be used as Slim\Dispatcher::FOUND, Slim\Dispatcher::NOT_FOUND, Slim\Dispatcher::NOT_ALLOWED

I will amend the comment in DispatcherResults to reference where those integers come from.

*/
public function getRouteArguments($urlDecode = true)
{
if ($urlDecode) {

This comment has been minimized.

Copy link
@damianopetrungaro

damianopetrungaro May 16, 2018

Avoid nested statement:

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

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

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

This comment has been minimized.

Copy link
@damianopetrungaro

damianopetrungaro May 16, 2018

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

This comment has been minimized.

Copy link
@geggleto

geggleto May 16, 2018

Contributor

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

This comment has been minimized.

Copy link
@damianopetrungaro

damianopetrungaro May 16, 2018

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 😄

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate May 16, 2018

Author Contributor

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

}
public function testSetDispatcher()
{
$this->router->setDispatcher(\FastRoute\simpleDispatcher(function ($r) {
$r->addRoute('GET', '/', function () {
});
}));
}, ['dispatcher' => '\\Slim\\Dispatcher']));

This comment has been minimized.

Copy link
@damianopetrungaro

damianopetrungaro May 16, 2018

Use Dispatcher::class instead of hardcoded namespace, if it will change we have to change it in a lot of places

@@ -362,7 +364,7 @@ public function testRouteCacheFileCanBeDispatched()
$method->setAccessible(true);
$dispatcher = $method->invoke($this->router);
$this->assertInstanceOf('\FastRoute\Dispatcher', $dispatcher);
$this->assertInstanceOf('\Slim\Dispatcher', $dispatcher);

This comment has been minimized.

Copy link
@damianopetrungaro
$class = new \ReflectionClass($this->router);
$prop = $class->getProperty('dispatcher');
$prop->setAccessible(true);
$this->assertInstanceOf('\FastRoute\Dispatcher', $prop->getValue($this->router));
$this->assertInstanceOf('\Slim\Dispatcher', $prop->getValue($this->router));

This comment has been minimized.

Copy link
@damianopetrungaro
@@ -205,19 +207,19 @@ public function testCreateDispatcher()
$class = new \ReflectionClass($this->router);
$method = $class->getMethod('createDispatcher');
$method->setAccessible(true);
$this->assertInstanceOf('\FastRoute\Dispatcher', $method->invoke($this->router));
$this->assertInstanceOf('\Slim\Dispatcher', $method->invoke($this->router));

This comment has been minimized.

Copy link
@damianopetrungaro
protected function getDataGeneratorClass()
{
return 'FastRoute\\DataGenerator\\GroupCountBased';

This comment has been minimized.

Copy link
@damianopetrungaro

damianopetrungaro May 16, 2018

Are we including it as dependency?
We may use namespace here too instead of hardcoded 😄

{
protected function getDispatcherClass()
{
return 'Slim\\Dispatcher';

This comment has been minimized.

Copy link
@damianopetrungaro
$routeArguments[$k] = urldecode($v);
}
switch ($routeStatus) {
default:

This comment has been minimized.

Copy link
@akrabat

akrabat May 17, 2018

Member

default should be last in a switch statement. Arguably, it should throw an Exception as it should never happen.

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate May 17, 2018

Author Contributor

What kind of exception would you like to be thrown here? Just a regular exception or RuntimeException

This comment has been minimized.

Copy link
@akrabat

akrabat May 17, 2018

Member

Runtime is fine as it should never happen :)

}
}
return $allowedMethods;

This comment has been minimized.

Copy link
@akrabat

akrabat May 17, 2018

Member

Should we be caching the list of allowed methods in an instance variable keyed by the function's parameters? It appears that we currently have to do this same work twice when we're in a method not allowed scenario.

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate May 17, 2018

Author Contributor

As per one of our previous discussions, this avoids the initial performance hit of looping through every single route. It is being returned on demand instead of initially doing it every time we dispatch. This is also the way Fastroute does it.

This comment has been minimized.

Copy link
@akrabat

akrabat May 17, 2018

Member

I understand that.

Given getAllowedMethods() has been called, why don't we keep a copy of the calculated $allowedMethods in a local member variable, so that we don' have to do all the work again if getAllowedMethods() is called again with exactly the same parameters?

From what I can tell if we're in a Method Not Allowed situation, we call getAllowedMethods() on line 68 of Dispatcher.php and then again in line 83 of RoutingMiddleware.php, so it appears to me that we're doing the same work twice.

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

This comment has been minimized.

Copy link
@akrabat

akrabat May 17, 2018

Member

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

This comment has been minimized.

Copy link
@l0gicgate

This comment has been minimized.

Copy link
@akrabat

akrabat May 17, 2018

Member

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

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate May 17, 2018

Author Contributor

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

* @return array
*
* @link https://github.com/nikic/FastRoute/blob/master/src/Dispatcher.php
* @return DispatcherResults

This comment has been minimized.

Copy link
@akrabat

akrabat May 17, 2018

Member

We need to add

$uri = rawurldecode($uri);

before calling the dispatcher's dispatch() in this method. Not sure if it should be a separate PR or not.

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate May 17, 2018

Author Contributor

This commit cc0c0b3 applies the requested change.

@damianopetrungaro

This comment has been minimized.

Copy link

commented May 17, 2018

@l0gicgate btw before merging could you please squash all your commits?

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2018

@damianopetrungaro I'm not the one merging, @akrabat will take care of that!

@damianopetrungaro

This comment has been minimized.

Copy link

commented May 17, 2018

You can do it also on your local branch:
git rebase -i HEAD~{number of commit to squash} and clean a little bit the history for this PR 😄

l0gicgate added some commits Feb 23, 2018

Fix DispatcherTest line length warning and increase test coverage
Add url decoding functionality to getRouteArguments() method
Fix HttpMethodNotAllowedException return type in comment
Reference $routeStatus constant origin and fix nested conditional

Use class constant instead of string reference

Use class constant instead of string reference

Use class constant instead of string reference

Fix switch default case in RoutingMiddleware
Use rawurldecode() on URI before dispatching in Router
Add allowed methods caching in Dispatcher

Add test case for routes with international characters

Add test case for international characters in route arguments

@l0gicgate l0gicgate force-pushed the l0gicgate:4.x-DispatcherResults branch from 99144ab to 9c50cb9 May 17, 2018

@akrabat

This comment has been minimized.

Copy link
Member

commented May 18, 2018

FWiW, I am not a fan of rebasing to clean history. Rebasing to remove a commit that's a typo fix is fine, but I personally prefer to see the route that was taken to get to where we are now.

*/
public function getAllowedMethods($httpMethod, $uri)
{
if ($this->allowedMethods !== null) {

This comment has been minimized.

Copy link
@akrabat

akrabat May 18, 2018

Member

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

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate May 19, 2018

Author Contributor

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

@l0gicgate l0gicgate changed the title 4.x New Dispatcher & Dispatcher Results 4.x New Dispatcher & Routing Results May 24, 2018

@akrabat akrabat merged commit c58a2bb into slimphp:4.x May 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

akrabat added a commit that referenced this pull request May 24, 2018

akrabat added a commit that referenced this pull request May 24, 2018

@damianopetrungaro

This comment has been minimized.

Copy link

commented May 24, 2018

Great work @l0gicgate !

@l0gicgate l0gicgate deleted the l0gicgate:4.x-DispatcherResults branch Jul 13, 2018

@akrabat akrabat added this to the 4.0 milestone Aug 24, 2018

@l0gicgate l0gicgate referenced this pull request Apr 25, 2019

Merged

Slim 4 Alpha Release #2665

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.