Configurable FastRoute Caching

[lupo-lupov]

Enabling FastRoute cache can be made.

There are 2 new parameters in main settings that are with same meaning like FastRoute cache aprameters, but prefixed with 'router':

• routerCacheDisabled - turn on/off route cache (true|false)
• routerCacheFile - file path to be cached (string - file path with writeable directory)

Cache is disabled by default

[tuupola]

Double negatives give headache. To turn on cache you must use routerCacheDisabled = false. It would be more readable to have routerCacheEnabled = true.

[lupo-lupov]

I totally agree with you, but don't you think we'll lose consistency with actual FastRoute parameters? Isn't keeping their names the same, makes it more clear, that they are passed straight to the FastRoute function?

[tuupola]

Oh sorry. Yes you are correct, consistency is better. Did not think this through.

[akrabat]

This setting may not exist if someone is using their own Container.

[akrabat]

Can we have unit tests for this please?

[ghost]

I'll do my best for the tests :)

What exactly do you mean

This setting may not exist if someone is using their own Container.

If I understand, if someone is using their own Container, he/she should add instance of Router (\Slim\Interfaces\RouterInterface) and if he\she wants to use cache, the way is to use public method Slim/Router::setCacheFilePath to do it.

[lupo-lupov]

Sorry for my last post (it was not from my personal, but my company account).
Here are the unit tests.
Any additional proposals are appreciated.

[akrabat]

What exactly do you mean

This setting may not exist if someone is using their own Container.

I mean that you can't assume that those array keys will exist in the settings array and will need to test for their existence.

[vlakoff]

I vote for setting name routerCacheEnabled rather than routerCacheDisabled, considering Slim doesn't cache by default, and FastRoute cachedDispatcher caches by default but has to be explicitly chosen.

[silentworks]

I am not for this PR, I intentionally left our route caching from the core of the Framework due to the issues it could cause for someone who doesn't understand what it does. This would lead to us having more issues to attend to due to people enabling cache this easy. If you don't remove the cached route file on each deploy, you end up with a broken system. And for this reason I left this out and was planning to add to the documentation how to get it working along with these warnings.

[vlakoff]

What are the performance gains? If they are not significant I would favor simplicity too (one less thing to manage).

[lupo-lupov]

Yes KISS principle.
I had a doubts, is it against slim philosophy or not...
Always dealing with cache and log directories on deployment is a little bit tricky.
Maybe it's not worth when managing light projects, but from the other side, the developer should know what he/she's doing and such option should make him think :)

[silentworks]

Thought about this a bit more and I agree with you on KISS. Can you update the PR please and we will look to get this merged in for 3.3.0.

[SvenRtbg]

I have two thoughts:

1. More configuration in an array? Is there no other way?
2. Caching the routing is great, but what I really need is a way to deploy a precompiled cache file, because I don't like writeable directories on a production server. Currently there is such a check in Slim, so in production the directory has to be writeable even though the file is already there.

[lupo-lupov]

Hi
About 1 - it's Slim style default configuration - my personal opinion is also the same (better array configurations instead of specific configuration class/object)
About 2 - you are right, I understand your case, but what if someone enable cache, but directory is not writeable... he'll never understand that actually is not using cache - any suggestions ?

[dopesong]

Throw exception when not able to create cache file - do not check is dir writable or not. Because like @SvenRtbg said I would prefer to precompile cache file during deploy process :)

[SvenRtbg]

The \FastRoute\cachedDispatcher() function does not explicitly check whether the cache file is writeable. It also does not throw an exception itself, it would probably trigger a PHP error if file_put_contents() cannot write to the cache file. However, if writing to a cache fails, it should not really affect the operation.

If Slim wants to add nicer behavior and notify the user about the problem, it should check if the cache file exists - it not, the directory should be writeable. Also, an existing cache file should be PHP code returning an array... in fact there are so many details that the FastRoute code takes care of, I think dealing with details of caching should be the task of FastRoute. If the current implementation does not correctly report the problem, send a pull request to FastRoute to deal with it more nicely, and then just use it.

[akrabat]

Thank you. I rebased, which is why this doesn't look like it has been merged, even though it has been.

Note that I removed the routerCacheDisabled and allow setting routerCacheFile to false to disable FastRoute caching.