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

Clear menu-cache when deleting items #4083

Merged
merged 5 commits into from May 1, 2019
Merged

Clear menu-cache when deleting items #4083

merged 5 commits into from May 1, 2019

Conversation

emptynick
Copy link
Collaborator

When a menu-item is destroyed, the save method is not called, thus the menu-cache is not cleared.
This PR adds cache-clearing when an item/menu is deleted

Fixes #4071

@AdrienPoupa
Copy link
Contributor

Instead of overriding Eloquent's save/destroy function, you could use events: https://laravel.com/docs/5.8/eloquent#events

@emptynick
Copy link
Collaborator Author

But this would introduce a whole new file to just call one method.
I prefer this way instead.

@MrCrayon
Copy link
Collaborator

@emptynick please reconsider your approach, using events doesn't always imply creating an observer, you can just add it in the model boot method:

    public static function boot()
    {
        parent::boot();

        static::deleted(function ($model) {
           $model->removeMenuFromCache();
        });

        static::saved(function ($model) {
            $model->removeMenuFromCache();
        });
    }

Also I think your destroy method will only remove Cache from the first Menu in the really remote case someone is going to delete more than one menu at the same time.

@emptynick
Copy link
Collaborator Author

Those static-anonymous functions have a lot of disadvantages.
They are static, so calling $model->removeMenuFromCache(); is not going to work if removeMenuFromCache isn't static as well.
If you make it static, you can't access properties without using hackish get/set method built-in to Eloquent and so on.

I made it observed by another class now.

@MrCrayon
Copy link
Collaborator

They are static, so calling $model->removeMenuFromCache(); is not going to work if removeMenuFromCache isn't static as well.

$model is passed as parameter, it works for me and it's the way I always used it and saw in examples.

I made it observed by another class now.

good 👍

@emptynick
Copy link
Collaborator Author

$model is passed as parameter, it works for me and it's the way I always used it and saw in examples.

Yes, but for menu-items you need to get the parent menu by calling a not-static method menu() and so on.

@MrCrayon
Copy link
Collaborator

$model->menu->removeMenuFromCache();

also works from MenuItem

@fletch3555
Copy link
Collaborator

My preference is leaning toward the events approach.

@emptynick
Copy link
Collaborator Author

Done

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.

Failed to Remove Menu Item from Menu Builder
4 participants