thephpleague / route

Fast PSR-7 based routing and dispatch component including PSR-15 middleware, built on top of FastRoute.
http://route.thephpleague.com
MIT License
651 stars 126 forks source link

Make it possible to easily extend Route, Dispatcher, etc. #160

Closed Arcesilas closed 6 years ago

Arcesilas commented 6 years ago

Hi,

I'm writing an extension for League/Route that injects Request, Response, Router and Route parameters into a BaseController to make it easier to access these objects. I've noticed it was painful sometimes to add features, because some objects that need to be extended are instanciated in methods and their instanciations cannot be configured.

For example, the Route object is instanciated in RouteCollection on L#89.

As far as I know, it is not recommended when using DI to hard code objects instanciations, unless you expect them to not be replaced by another instance. And indeed, I would appreciate to be able to change their implementations, in order to add new features.

Would you agree with a PR which instead fetches these objects from the Container, since RouteCollection, for instance, holds it already? Or, do you think it would be worth considering adding some of the features I've implemented in Xocotlah/Router to League/Route ? I'm asking because I don't know how you see the future of League/Route, which features you would consider adding, etc.

Thanks!

philipobenito commented 6 years ago

Because the Route object is essentially just a value object that needs to be created multiple times it's created where it is. The plan is to refactor that to be a factory which will be able to be swapped out with implementations.

As for what you're trying to do, I think you should be able to achieve it with a custom strategy http://route.thephpleague.com/custom-strategies/ easily enough.

As for extra features, I'm just waiting for the middleware PSR to be accepted then I will be doing a full rewrite of internals for version 4 so watch out for my initial plan for that and give any ideas there :-)

philipobenito commented 6 years ago

A little more insight from looking at your package.

Not extending a base controller was a conscious decision so that the application layer is not actually tied to the router in anyway, by using a dependency injection container and passing that to league/route you can have your objects passed to your classes.

It's also good to bear in mind and remember that your controller is not a class, it's just the callable that is invoked, whether that be a class method or any other type of callable. That means that for the controller to have access to the request and response, they need to be passed as arguments of the controller callable itself. This also allows them to follow a middleware signature that makes it easier to fit them in to the stack (be aware though that middleware is likely to follow a single pass rule going forward) so the controller and any other middleware will only be passed the request and will be responsible for building their own response if required.

Arcesilas commented 6 years ago

Thanks for your quick reply!

To override Route and Dispatcher, I've just iverriden the methods in which they are instanciated, so that it uses the class in my namespace. Not a big deal, just one minor problem: if you make any change in the methods I've overriden, I'll need to update.

I've been considering custom strategy to allow controller to only return text, array, Response and let the strategy build the appropriate response (respectively ApplicationStrategy, JsonStrategy, or resend the Response). I had not considered Custom Strategy for the initial purpose... But indeed, it seems to be easier than what I've been doing for now...

You answered again while typing, so, here the answer to your 2nd answer:

I understand completely why you did not use a base controller. And that's absolutely not a drawbadk. Some people just want base controllers, I'm just providing them with a package that make it easier to extend a base controller. My package does not change anything at all in the behavior of League/Route: controllers can still be functions, invokable classes, or classes. It's just in the case they are classes which extend a BaseController, Request, Response, Router and Route parameters are injected. It's up to user to do what they want. Currently, controllers in my package are still invoked with Request, Response and Vars. I'm just making it easier to write a controller (yeah, lazy devs). Also, I think it's easier to read a controller when the code that is present in absolutely each of them is "hidden".

Base controllers is not a feature I would have proposed to be added to League/Route, since it's clearly not the purpose of League/Route, which simply provides several convenient ways to write controllers, depending on the cases (sometimes, just returning a ResponseResponse from an anonymous function in the Route definition is quite enough and does not harm, even when you want to give your controllers a structure and hierarchy).

A feature that I appreciate (and have partially implemented) is building the URL from a route name with parameters. I don't know if you consider it could fit in League/Route or not.

Concerning the other features I wish to provide, they clearly don't fit in the base package. Extension packages could provide convenient tools for everyone depending on how they structure and organize their code, depending on everyone's habits and preferences.

That's why I think it would be nice to make Route easier to extend. Maybe it could be nice, too, to have a section about how to extend the package in the documentation. It's just a piece that can be integrated into a home-made framework, so extending it is important in my opinion.

Thanks again, League/Route is great!

philipobenito commented 6 years ago

The plans I have for v4 covers quite a lot as it will be quite modular and much more extendable than it already is.

Extensions maybe could just be middlewares that run at certain stages of the route/dispatch process, like hooks.

That being said, are you happy for me to close this and keep an eye out for the v4 roadmap in around a month?

Arcesilas commented 6 years ago

Sure! Thanks for your time and quick answers!

Arcesilas commented 6 years ago

Hi,

First, Thank you for suggesting me to use Strategy to accomplish what I wanted to do: it works great. And therefore, I'll rename my package to SmartController, since it does not really extend Route anymore.

I take the liberty of adding one suggestion in this thread: the only thing I want to implement and that require to override RouteCollection (to have control over Route instanciation) is URL generation (based on Route name and parameters). In Laravel (which is not the best example in soome cases), URL Generator is tied to Routing package, which makes sens, since routing know all about Request and Routes.

Do you think such a feature could fit in League/Route (v4) or should it be a third party feature? It's still possible for anyone to extend RouteCollection and override map() (in v3), but I think it could be nice to have this feature natively.