getkirby / ideas

This is the backlog of ideas and feature requests from the last two years. Use our new feedback platform to post your new ideas or vote on existing ideas.
https://feedback.getkirby.com
20 stars 0 forks source link

Delete only the expired files cache #540

Open ronaldtorres opened 4 years ago

ronaldtorres commented 4 years ago

Delete only the expired cache files, as if an a key is not called with the get method this will not be deleted by expiration time and the flush method also delete the not expired files.

I think this may can be a solution in the class FileCache.php

  /**
     * Remove all the expired files
     *
     * @return bool
     */
    public function removeExpired(): bool
    {
        $keys = $this->keys();

        foreach ($keys as $key) {
            if ($this->expired($key)) {
                $this->remove($key);
            }
        }

        return true;
    }

    /**
     * Get all the cached file keys
     *
     * @return array
     */
    public function keys(): array
    {
        $files = Dir::files($this->root);
        $keys = [];

        $ext = $this->options['extension'];
        $ext = $ext ? ".$ext" : '';

        foreach ($files as $file) {
            $keys[] = str_replace($ext, '', $file);
        }

        return $keys;
    }
lukasbestle commented 4 years ago

I thought about this for a while and I agree it makes sense in general like with our session garbage collector (side note: to keep it consistent, the method would be called collectGarbage()).

However I'm not sure if we can make this work well for the file cache: Kirby would need to open every single file to check the expiry date as the timestamp is not already part of the filename (unlike the sessions), which would make the process slow. So this garbage collection is not something that could happen automatically in the background (again unlike the sessions).

This in turn means that the method would need to be called manually by a cronjob script – at which point the question is if it's not easier to use a different cache driver like APCu, Memcached or Redis, which delete expired entries automatically.

The file cache driver is meant as a simple solution for page cache as it's easy to setup and works quite well for the reduced set of cache entries (it can be assumed that each page is requested ever so often, which will update the cache automatically). If you really need cleanup of expired entries, one of the other drivers is probably the better solution.

ronaldtorres commented 4 years ago

Thank you for your response!

I totally agree that using other cache driver like Memcached or APCu is the best solution.

But, what do you recommend doing, if you are making a plugin that you want to share and the only cache you can use with certainty is the file cache, because you don't know if the other users have Memcached, APCu or any other installed.

Obviously providing to users the option to choose which cache driver to use...

lukasbestle commented 4 years ago

What kind of data is the plugin going to store in the cache?

I'd say what to do depends on this question: If it's not absolutely necessary to garbage-collect the data, then I would just go without garbage-collection (much less work for users to set up: Cronjob etc.). If you absolutely need garbage-collection, you can do it from your plugin even without support in the core ($cache->root() is a public method and can therefore be accessed from your plugin). But in this case, you will need to document how your plugin's users can set that up.

ronaldtorres commented 4 years ago

Hello @lukasbestle, I'm saving some temporary strings.

Thanks for the suggestion. I want to find a solution that don't leave the Kirby's philosophy and patterns. However I'm going to explore a bit your idea.

Maybe is the solution just is suggest to the user that use a cache drive like APCu or any other :sweat_smile:

Thanks a lot for your help!