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.

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

[12.x] Add CacheFlushed Event #55142

Merged
merged 7 commits into from
Mar 24, 2025

Conversation

tech-wolf-tw
Copy link
Contributor

@tech-wolf-tw tech-wolf-tw commented Mar 23, 2025

Description:
Introduces CacheFlushed event dispatching when calling $cache->flush(). Enables monitoring of full cache clearance for logging, auditing, or reactive workflows. Maintains backward compatibility.

This PR resolves this issue

@tech-wolf-tw tech-wolf-tw changed the title [12.x] Fix: can() method does not work for route groups [12.x] Add CacheFlushed Event Mar 23, 2025
@AndrewMast
Copy link
Contributor

Do these events even need to extend a class? Might be best to have everything contained in the final event classes, in my opinion.

@tech-wolf-tw tech-wolf-tw requested a review from cosmastech March 23, 2025 12:34
@taylorotwell taylorotwell merged commit ef3e8c2 into laravel:12.x Mar 24, 2025
39 checks passed
@erikn69
Copy link
Contributor

erikn69 commented Mar 24, 2025

CacheFlushFailed?

I also understand that it can be flushed using tags,
but it does not allow adding tags as an argument

$cache->tags(['my_tag'])->flush();

public function __construct($storeName, $key, array $tags = [])

/**
* Fire an event for this cache instance.
*
* @param \Illuminate\Cache\Events\CacheEvent $event
* @return void
*/
protected function event($event)
{
parent::event($event->setTags($this->tags->getNames()));
}

So , wouldn't $event->setTags give an Exception?
I'm just concerned, I don't know if it works correctly in Tags

@erikn69
Copy link
Contributor

erikn69 commented Mar 24, 2025

Barryvdh\Debugbar\DataCollector\CacheCollector::onCacheEvent(): Argument #1 ($event) must be of type Illuminate\Cache\Events\CacheEvent, Illuminate\Cache\Events\CacheFlushed given, called in /var/www/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php on line 460

I think it would be more consistent if it extended from Illuminate\Cache\Events\CacheEvent, look laravel debugbar DataCollector/CacheCollector.php#L33

@AndrewMast
Copy link
Contributor

@erikn69 Taylor specifically said here that he thinks it should not extend CacheEvent.

@erikn69
Copy link
Contributor

erikn69 commented Mar 25, 2025

@AndrewMast, @taylorotwell could say that, but it seems I'll have to highlight the signature.

* @param \Illuminate\Cache\Events\CacheEvent $event

* Fire an event for this cache instance.
*
* @param \Illuminate\Cache\Events\CacheEvent $event
* @return void
*/
protected function event($event)
{
parent::event($event->setTags($this->tags->getNames()));
}

I think it goes without saying why CacheEvent should be used, because the other option would be to change signatures here and in other packages like laravel-debugbar

@AndrewMast
Copy link
Contributor

AndrewMast commented Mar 25, 2025

Hey, please tone down the passive aggressiveness.

Why don't you open an PR that addresses this issue? There is no easy solution to this issue.

The CacheEvent and its many implementations contain a $key property that does not make sense to contain in the CacheFlushed/CacheFlushing events. Taylor made a good point that extending the current CacheEvent doesn't make sense.

As you pointed out a few days ago, the CacheFlushed/CacheFlushing events do not contain a setTags method, so adjusting the event method signature does not make sense as it would throw an exception. A PR could be created to fix the missing method.

The "best" solution that I can think of is to remove the $key property from CacheEvent, add a KeyedCacheEvent class that reimplements this property, and have all of the implementations now extend this new class. This, however, is 1000% a breaking change and would need to be target master. Scratch that, I think creating an interface might be the best way.

Or we could simply implement setTags in the new events and create an interface to identify the cache events that way.

Edit: I just noticed you opened a PR, good work. I guess I need to eat my own words. Might want to change the signature to something like parent::__construct($storeName, '', $tags); to match similar code in other existing implementations of CacheEvent. But Taylor might shoot this down again due to the same thing of extending CacheEvent. The implementations that "ignore" $key implement their own $keys and usually store the first of the keys into $key.

@erikn69
Copy link
Contributor

erikn69 commented Mar 25, 2025

Hey, please tone down the passive aggressiveness.

Sorry, I'm using Google Translate and I don't know if it translates exactly what I want to express.

Why don't you open an PR that addresses this issue?

#55157

Or we could simply implement setTags in the new events and create an interface to identify the cache events that way.

could you open a PR that way? I don't understand it correctly, but I imagine that it would also change the signature for other packages.

parent::__construct($storeName, '', $tags);

Great

Taylor might shoot this down again due to the same thing of extending CacheEvent

It is the most likely thing to happen

@AndrewMast
Copy link
Contributor

@erikn69 My attempt to fix it is #55169

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