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

Can't call multiple dispatches on a single instance #290

Closed vfsoraki closed 3 years ago

vfsoraki commented 3 years ago

Pretty much what the title says.

The root of the problem is that the dispatch function is calling prepRoutes inside Router. With a second call to dispatch on the same instance, we get an exception from FastRoute that the same routes are being added again, which is obvious.

A simple fix would be to have a flag in the Router class and check it at the beginning of prepRoutes method, to see if routes are already prepared or not. If prepared, skip the method.

My use case is that I'm using this router with ReactPHP. I don't want to instantiate a new Router for every request. The same router should compile and cache routes so the same Router instance can be used to handle them all.

I'm looking for some thoughts on this.

vfsoraki commented 3 years ago

Implemented in https://github.com/vfsoraki/route/tree/feature/multiple-calls-on-same-instance.

A review would be welcome.

jpmc commented 3 years ago

Phil made an unmerged PR that tackles this very concern (Link here #283 )

This gives it plenty of relatively minor updates, and bumps up the minimum PHP version, but includes a cached router to support React and the like.

vfsoraki commented 3 years ago

@philipobenito I looked into cached router implementation @jpmc mentioned.

From what I understand, the cached router caches routes in a file and reads them back every time a request a dispatched. This seems inefficient especially when running in async environments.

Having an option to cache routes in memory would be great.

philipobenito commented 3 years ago

I was aiming to get this release out last week but I’ve had flu, so now aiming for a release some point in the next two weeks.

I have no concerns about the implementation as long as it’s used in the intended way, but I need to give some thought to possible misuse and any safety concerns there.

I also need to do some more extensive testing, I’d appreciate help with that if you want to try it out.

All being well I will also implement several storage drivers for the cache with an easy way to implement more for users using anything not supported initially.

Sent from ProtonMail Mobile

On Thu, Jan 14, 2021 at 04:09, Vahid Fazlollahzade notifications@github.com wrote:

@philipobenito I looked into cached router implementation @jpmc mentioned.

From what I understand, the cached router caches routes in a file and reads them back every time a request a dispatched. This seems inefficient especially when running in async environments.

Having an option to cache routes in memory would be great.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

philipobenito commented 3 years ago

Closing as included in imminent release