Skip to content

[12.x] Fix adding setTags method on new cache flush events #55405

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.

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

Merged
merged 1 commit into from
Apr 18, 2025

Conversation

erikn69
Copy link
Contributor

@erikn69 erikn69 commented Apr 14, 2025

Why

In #55121 (comment), Taylor said that the new CacheFlushed or CacheFlushing events should not extend CacheEvent. This makes sense since the CacheEvent is for events with a specific $key.

However, that means that we now have cache events that do not share a common ancestor and these events cannot have their tags set in TaggedCache (or RedisTaggedCache).

What

This PR attempts to fix this issue by adding a TaggedCacheEvent abstract class with a setTags method and making CacheEvent, CacheFlushing, and CacheFlushed extend it.

@taylorotwell
Copy link
Member

I still find this confusing. I don't think CacheEvent should extend TaggedCacheEvent if we are going to re-architect things. Frankly I don't even know what we are trying to solve here. Can we ground it in reality a bit by discussing what end-user problem we are trying to solve or what you're trying to do in a package that is impossible right now in the current state of the code?

@taylorotwell taylorotwell marked this pull request as draft April 15, 2025 18:53
@Levivb
Copy link

Levivb commented Apr 15, 2025

At the moment I can't update past Laravel 12.3.0 because of a dispatched event on which ->setTags() is called during a clear on a Cache repository supporting tags, but that method doesn't existing on the event

CacheFlushing and CacheFlushed events are introduced in https://github.com/laravel/framework/pull/55142/files

To ground it in reality, the framework contains a bug which ideally should be solved :)

@systemsolutionweb
Copy link

Can we ground it in reality a bit by discussing what end-user problem we are trying to solve

A bug

Error: Call to undefined method Illuminate\Cache\Events\CacheFlushing::setTags()

@taylorotwell
Copy link
Member

Can we just add setTags to those events then? I don't really like CacheEvent extending TaggedCacheEvent - that doesn't totally make sense to me from an object hierarchy point of view, even if it does basically match what is in the code now.

@taylorotwell
Copy link
Member

Temporarily added this: 5291e19

@erikn69 erikn69 force-pushed the patch-5 branch 2 times, most recently from faadfe2 to e6fbf99 Compare April 16, 2025 17:58
@erikn69 erikn69 marked this pull request as ready for review April 16, 2025 20:57
@taylorotwell taylorotwell merged commit bcb3f58 into laravel:12.x Apr 18, 2025
58 checks passed
@erikn69 erikn69 deleted the patch-5 branch April 21, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken CacheFlushing event on TaggedCache Missing flushed cache event on Illuminate\Cache\TaggableStore
4 participants