ChassisPHP / ChassisPHP

A PHP framework built to simplify CMS site creation.
MIT License
10 stars 27 forks source link

add route caching #98

Open RogerCreasy opened 7 years ago

RogerCreasy commented 7 years ago

This should be straightforward. I think the fastroute dispatcher needs to be switched out in our Router. See the caching section here

RogerCreasy commented 7 years ago

Should the route cache go into a sub-directory of the storage directory? Other ideas?

dusty-wil commented 7 years ago

Hi, I could take a look at this! I can make a sub-directory of the storage dir, or make a "tmp" dir maybe?

RogerCreasy commented 7 years ago

I would prefer a subdirectory of the storage directory. But, what are your thoughts?

dusty-wil commented 7 years ago

I think a subdirectory of storage makes sense. I was thinking a temp folder might indicate shorter-term storage vs. longer term, but it's still a manner of storage, so putting it in the storage directory makes sense. Maybe a cache or temp subdirectory in storage?

RogerCreasy commented 7 years ago

How about routeCache? That should make its purpose very clear to devs.

dusty-wil commented 7 years ago

Sounds good! inside the storage folder? I'll be able to look at this later this evening!

RogerCreasy commented 7 years ago

yes, storage/routeCache/ should work well.

dusty-wil commented 7 years ago

I added the change for this, but when I ran testing on it, I got an issue with the cache file.

PHP Fatal error: Uncaught Error: Call to undefined method Closure::__set_state() in /home/dustyw/projects/ChassisPHP/storage/routeCache/route.cache:7

It turns out this seems to be an issue with built-in php closures not being serializable:

https://github.com/nikic/FastRoute/issues/141

I'm currently looking for a way to address this

RogerCreasy commented 7 years ago

Interesting. Does it work with routes that use a controller rather than a closure directly? i.e. /backend/users

dusty-wil commented 7 years ago

Hmm, good question... I /think/ so. If I'm understanding the issue correctly, and if I'm understanding how they mitigated the issue here:

https://github.com/FrozenNode/Laravel-Administrator/issues/871

I'll have to do a little more research, though, to see how that could be implemented.

RogerCreasy commented 7 years ago

To test the controllers functionality, you could comment out all of the routes that use a closure and see if you get the errors.

If you don't get the errors, we may have to convert closure routes to controllers behind-the-scenes; we need to leave the ability to use closures to allow for simple routes for developers.

Alternatively, we could go another direction. The issue you referenced on fastroute says that a text value works. We could create a class that has an array with text values as keys and the closure as the value (a lookup table). I'm not sure where/when to read the values in, and there would be a bit of converting back-and-forth for the router, but it shouldn't be too difficult.

Thanks for looking at it. I'll look some when I get a chance. May be a day or 2, though.

RogerCreasy commented 7 years ago

After looking at this a bit, here what I think we need to try:

@dusty-wil are you still working on this issue? I don't want to step on what you are doing.

dusty-wil commented 7 years ago

@RogerCreasy I've been having some issues getting phpunit to properly test the Router, so haven't made any good forward progress on this yet (I think this is an issue with my environment setup). I can look into implementing all or part of what you mentioned above, but I won't be able to get to it until this weekend due to school. So, if you want to open it up to someone else or look into it personally, that would be totally understandable!

smallfish500 commented 5 years ago

https://packagist.org/packages/opis/closure I think controller should be -> called. Is this project forgotten?

RogerCreasy commented 5 years ago

Not a forgotten project. Do you want to join us and help out?