fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

\Cache::delete_all() using memcached driver not clearing all items #2143

Open iturgeon opened 4 years ago

iturgeon commented 4 years ago

I just ran into an issue where delete_all wasn't working as I expected.

The first argument to Cache::delete_all() is $section, with the following description:

Name of a section or directory of the cache, null to delete everything

On fuel/core 1.8.2, I'm getting this behavior using the memcached driver:

\Cache::set('section_name.item', 'value');
\Cache::delete_all(); // expected to remove everything
\Cache::get('section_name.item'); // 'value'

\Cache::delete_all('section_name');
\Cache::get('section_name.item'); // null

Looking at https://github.com/fuel/core/blob/1.8.2/classes/cache/storage/memcached.php#L149

$section is prepended with the configured cache_id.

Then looking at https://github.com/fuel/core/blob/1.8.2/classes/cache/storage/memcached.php#L157

This is the branch for choosing between deleting everything or just the matching section - however, ! empty($section) looks like it can't ever be false because it will always contain the prepended cache_id.

Also note - it won't be an issue if cache.memcached.cache_id is an empty string.

Just checking to see if I'm seeing this correctly?

WanWizard commented 4 years ago

I think you do.

I've checked all drivers, and the apc and redis drivers contain the same code.

My initial reaction, without testing, is that for these three the prefixing shouldn't be there at all, because they work with a cache index per cache_id, so a delete_all() without a $section means delete all index entries for that cache_id.

I've checked the history, turns out this code has been there since it's creation by Dan in 2010, so it has never worked as intended.

WanWizard commented 4 years ago

Correction, I think the cache_id shouldnt be there, so L149 should be

        // determine the section index name
        $section = empty($section) ? '' : '.'.$section;

Are you able to check it, I don't have memcached handy at the moment...

iturgeon commented 4 years ago

@WanWizard I will in a bit, in the middle of prepping a release myself.

iturgeon commented 4 years ago

I suspect it's uncommon to want to call delete_all(), I was doing so in a unit test. The environment recently changed, so I suspect it didn't have a cace_id before, but does now - and mysteriously the test started failing.

WanWizard commented 4 years ago

Ah, yes, an empty cache id would hide the bug... :smirk:

iturgeon commented 4 years ago

Ok, I think I've got something worked out. How do I add/run tests to fuel_core?

iturgeon commented 4 years ago

Well, I got the included tests to run on my project that's using Fuel. I included them in the PR but haven't run them purely in the context of a clean FuelPHP environment.

WanWizard commented 4 years ago

We'll probably have to remove them, a clean environment doesn't have any memcached config (or working backend), so those tests will always fail.