Skip to content

Conversation

@AndrewMast
Copy link
Contributor

Why

Alternative solution to #55157. Comes from discussion on #55142.

In #55121, 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 interface with a setTags method and making CacheEvent, CacheFlushing, and CacheFlushed implement it.

An alternative solution to this would be to allow CacheFlushing and CacheFlushed extend CacheEvent with an empty key (see #55157).

@GrahamCampbell
Copy link
Collaborator

I think most drivers don't support tags for flush, and just delete the entire cache.

@erikn69
Copy link
Contributor

erikn69 commented Mar 25, 2025

Great, let's wait for the review

@AndrewMast
Copy link
Contributor Author

This honestly isn't a great implementation, and probably should be marked as a breaking change (even though its just a signature change). But then if we did that, we might as well make a breaking change to CacheEvent and move $key into a KeyedCacheEvent and have the other events extend it.

If a maintainer knows what way would make the most sense (and would be accepted), let me know and I could update this PR or create a new PR.

@taylorotwell
Copy link
Member

Frankly I'm not sure what problem we are trying to solve here. I thought we just wanted events when the cache is flushing, which we now have.

@erikn69
Copy link
Contributor

erikn69 commented Mar 27, 2025

@taylorotwell hi, but on TaggableStore the flushed event is not firing
$cache->tags(['tag_key'])->flush(); // no event dispatched
$cache->tags(['tag_key'])->clear(); // bug on setTags

@AndrewMast AndrewMast deleted the fix/tagged-cache-events branch March 27, 2025 17:33
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.

5 participants