core icon indicating copy to clipboard operation
core copied to clipboard

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

Open iturgeon opened this issue 4 years ago • 8 comments

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?

iturgeon avatar Jul 08 '20 01:07 iturgeon

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 avatar Jul 08 '20 07:07 WanWizard

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...

WanWizard avatar Jul 08 '20 08:07 WanWizard

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

iturgeon avatar Jul 08 '20 18:07 iturgeon

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.

iturgeon avatar Jul 08 '20 18:07 iturgeon

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

WanWizard avatar Jul 08 '20 18:07 WanWizard

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

iturgeon avatar Jul 09 '20 00:07 iturgeon

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.

iturgeon avatar Jul 09 '20 01:07 iturgeon

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.

WanWizard avatar Jul 09 '20 09:07 WanWizard