Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
iturgeon opened this issue Jul 8, 2020 · 8 comments · May be fixed by #2144
Open

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

iturgeon opened this issue Jul 8, 2020 · 8 comments · May be fixed by #2144

Comments

@iturgeon
Copy link
Contributor

iturgeon commented Jul 8, 2020 •
edited

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

iturgeon commented Jul 8, 2020

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

@iturgeon
Copy link
Contributor Author

iturgeon commented Jul 8, 2020

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
Copy link
Member

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

@iturgeon
Copy link
Contributor Author

iturgeon commented Jul 9, 2020

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

@iturgeon iturgeon linked a pull request Jul 9, 2020 that will close this issue
Open
@iturgeon
Copy link
Contributor Author

iturgeon commented Jul 9, 2020 •
edited

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix memcached cache delete_all() iturgeon/core
2 participants
@iturgeon @WanWizard