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

Multiple hooks for one action can be added, but they have to be currently deleted completely #131

Closed
mvorisek opened this issue Mar 13, 2020 · 6 comments · Fixed by #133
Closed

Comments

@mvorisek
Copy link
Member

See https://github.com/atk4/core/blob/develop/src/HookTrait.php#L98

Specific hook should be possible to delete by name and priority.

@PhilippGrashoff
Copy link

very good one! So we need an optional ident to set to a hook so we can identify it for removal, e.g.
$this->removeHook('afterSave', 'createNotifications'); Or how would you approach accessing each hook individually?

@mvorisek
Copy link
Member Author

@PhilippGrashoff "optional ident" would be the best, but as different priority can be set, for most of the purposes optional "priority parameter in remove method" would be enought and implementing should be very easy + there would be 100% backcompatibility. Wdyt?

@DarkSide666
Copy link
Member

Can you give me one real-life use-case when deleting hooks is useful?
I think it's the same as with model conditions which sets model scope - no need to actually delete them.

@mvorisek
Copy link
Member Author

mvorisek commented Mar 16, 2020

@DarkSide666 I have some custom persistence manager. And if this manager is used, hook is added to the managed object when processing and removed when done.

When the object is not processed thru this manager instance, the hook should never be fired. Of course, it can we solved differently, but I do not see any issue with this approach.

With this PR also one-time hooks can be written very easily like:

$ind = onHook($spot, function() use(&$ind) { removeHook($ind); });

@PhilippGrashoff
Copy link

PhilippGrashoff commented Mar 16, 2020

@DarkSide666 a very simple example is if you use save() in an afterSave() hook. This leads to an infinite save() after save().
This is taken from my code to ship around that:

  
    /**
     *
     */
    public function setCalendarCache(?string $cacheRow, string $cache)
    {
        $this->_exceptionIfThisNotLoaded();
        if ($cacheRow) {
            $this->set('calendar_row_cache', $cacheRow);
        }
        $this->set('calendar_cache', $cache);
        //work on clone as all the hooks take a lot of time and lead to infinite save()s
        $clone = clone($this);
        $clone->removeHook('afterSave');
        $clone->removeHook('beforeSave');
        $clone->removeHook('beforeUpdate');
        $clone->removeHook('afterUpdate');
        $clone->reload_after_save = false;
        $clone->save();
    }

Actually, that code even shows a better example by stopping hooks from executing extra code if all you want really is a plain save() operation....

@DarkSide666
Copy link
Member

I see. So, yes, maybe that can be handy in these situations, but still should be used with care.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants