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

Custom Dispatcher Implementation for 3.x/4.x #2399

Closed
l0gicgate opened this issue Feb 16, 2018 · 3 comments
Closed

Custom Dispatcher Implementation for 3.x/4.x #2399

l0gicgate opened this issue Feb 16, 2018 · 3 comments

Comments

@l0gicgate
Copy link
Member

l0gicgate commented Feb 16, 2018

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.

Solution
As discussed, I proposed that we implement our own dispatcher based on the logic found in FastRoute\Dispatcher\GroupCountBased. Our dispatcher would return a DispatcherResults object which would have the following methods:

  • ::getStatus() which would return Dispatcher::FOUND, Dispatcher::NOT_ALLOWED or Dispatcher::NOT_FOUND
  • ::getAllowedMethods() which would return the allowed methods for dispatched route. This would be done dynamically as it has to loop through all the routes in order to compute the allowed methods. This is the exact reason why at the moment it is only done when the Dispatcher::NOT_ALLOWED status is being returned. This way, we offset the overhead on the end-user when they call this method.
  • ::getHandler() when the route is found, this is the way we get the handler (callable) associated with the found route.

Implementation
@akrabat mentioned that we could implement this in 3.x and append the DispatcherResults object generated to a request attribute dispatcherResults or dispatchResult and consequently phase out routeInfo on 4.x which is currently being appended while routing is done.

Questions

  • Are you in favor of this implementation?
  • Should we implement it in 3.x and phase out routeInfo in 4.x
  • Should the new attribute be called dispatchResult or dispatcherResults
  • Should the object returned by our dispatcher be DispatcherResults or DispatchResults or something else.

Status

  • Proof of concept of custom Dispatcher can be seen here
  • Need to finalize discussion
@RyanNerd
Copy link
Contributor

RyanNerd commented Feb 16, 2018

  • I am in favor of this implementation. Including the work-around for 3.x.
  • Absolutely phase out routeInfo in 4.x
  • I am in favor of the plural for the attribute: dispatcherResults
  • I am in favor of DispatcherResults (if the attribute is also plural). I'd also be okay with DispatcherInfo

@l0gicgate
Copy link
Member Author

@akrabat @geggleto what are your thoughts here. Perhaps a lot of the community doesn't quite understand what this thread is about since it's not something that's relevant to them. If you two approve, I'll start a PR, it should be a fairly small one.

@akrabat
Copy link
Member

akrabat commented Feb 23, 2018

It works for me. I like @RyanNerd's pluralisation too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants