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

New routing #591

Open
WinterSilence opened this issue Dec 18, 2014 · 7 comments
Open

New routing #591

WinterSilence opened this issue Dec 18, 2014 · 7 comments

Comments

@WinterSilence
Copy link

At this stage, discuss common issues.

My version
PSR-1, PSR-2, Namespaces
Separate classes (Router and Route).
Remove dependency (Kohana::cache, URL::site, Arr::get).
Fix and optimize methods (compile, uri, matches).

Route:
Сlass simplified for convenience and speed up: only one filter for the route, removed setters (properties sets at create). In most cases this is enough,if necessary, you can extend class.

@enov
Copy link
Contributor

enov commented Dec 18, 2014

We can't have PSR-1 and PSR-2 at this point, @WinterSilence.

I would go one step further to define the interfaces for those classes.

  • Make Router interface dependent on Route interface
  • Make the Request dependent on Router interface

This way we can have multiple implementations of Route:

  • Route_Legacy (the route we currently have with directory and prefix
  • Route_Dynamic (the route we want to acheive in Namespaces for controllers modules and helpers #571 with FCQN controller maybe with namespace and suffix Controller
  • Route_Static (a static route that does not get affected by a controller parameter, it just routes to the controller and action defined in that route)

Also, we can have multiple implementation of Router interface, one of them would be to load from config.

Moreover:

  • add a process method to Router, so that processing will be handled inside the Router from Request and should return matched Route along with the controller.

Add unit tests :)

Cheers!

@WinterSilence
Copy link
Author

@enov

At this stage, discuss common issues.

I don't suggest using these classes, this is only sketches. At first, we need to discuss the principles of classes. I just described my classes to highlight the major changes.

@enov
Copy link
Contributor

enov commented Dec 18, 2014

Alright @WinterSilence. 👍 for separating router and route.

@WinterSilence
Copy link
Author

@enov

Make Router interface dependent on Route interface

I agree, but we need to make a list of the main methods.

Moreover: add a process method to Router, so that processing will be handled inside the Router from Request and should return matched Route along with the controller.

I dont understand, you can write an example?

Add unit tests

Too early for this.

Add sketches for your version - interfaces and dummy-classes (empty methods)

@enov
Copy link
Contributor

enov commented Dec 18, 2014

add a process method to Router

I was saying if we can remove this method from Request and put it where it belongs:

https://github.com/kohana/core/blob/3.3/master/classes/Kohana/Request.php#L457

@WinterSilence
Copy link
Author

Update: added interfaces for routes (IRoute), requests (IRequest) and public function Router::process(IRequest $request)

@enov
Copy link
Contributor

enov commented Dec 19, 2014

  • Do not prefix interfaces names with I, we do not have a naming convention for that. Just choose the most generic name possible, in this case Route and Router are sufficient.
  • Add a Router interface with process, get_route and set_route
  • At this point, we should not mess with Request (also there are interfaces for that already), so maybe process should only care about $uri and method? Is there anything else Router needs to process from Request?

@neo22s neo22s added this to the 5.0.0 milestone Mar 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants