Improvements to hooks #2541

afbora · 2020-03-25T18:25:48Z

Describe the PR

General note: As this is a large PR, reviewing this from the "Commits" view instead of from the "Files changed" view is recommended to keep the overview. I was very careful to split the PR up into useful commits that make sense on their own, which should help with reviewing in this way. Splitting this PR up into separate PRs was unfortunately not possible as we would then have needed three PRs at minimum, which would be even harder to keep an overview on.

Hook arguments are now named and magically provided as requested (like in controllers). So it's now possible to just request the arguments the hook needs and in any order. Arguments that are not available will be set to null.
Support for wildcard hooks (e.g. page.*:before or page.create:* or *.create:* – all variations are supported!) that will be triggered for all event actions of the given type.
Note: The support for all wildcard variations is included in the fifth commit. If we decide that a simple page.*:before hook without the other variations is enough (which wouldn't be as flexible though), we can simply remove the fifth commit.
New $event object provided as argument for all hooks that contains all information about the triggered event. The $event argument can be used instead of or in addition to the individual hook arguments.

return [
    'hooks' => [
        'page.*:before' => function ($page, $event) {
            var_dump($page);            // $page
            var_dump($event->name());   // 'page.create:before'
            var_dump($event->type());   // 'page'
            var_dump($event->action()); // 'create'
            var_dump($event->state());  // 'before'
            var_dump($event->page());   // $page
            var_dump($event->input());  // $input
        }
    ]
];

Breaking-changes:

The method signature of $app->apply() and $app->trigger() has changed. Instead of a list of arguments, they now receive an associative array with named arguments. $app->apply() now also requires a third argument that tells the method which argument should be modified by the hooks. As both methods are marked for internal use and also not very useful in sites, the only use-case for them so far was hooks defined in plugins. Plugin authors who depend on any of the two changed methods need to update their plugins to support the new signature.
As the hook arguments are now detected by name and not by order, any hooks that don't use the variable naming as documented in the reference will receive null for those arguments instead. I expect that most users already use the naming from the reference as the reference provides copy-paste code snippets. However all hooks should be checked for wrong variable naming. Sometimes the issue will be obvious as the hook will just not work, but it's still better to check beforehand. I have developed a migration checker plugin for this.

Related issues

Implements Wildcard hooks (e.g.: page.*:after) ideas#194

Ready?

Added unit tests for fixed bug/feature
Passing all unit tests
Fixed code style issues with CS fixer and composer fix
Added in-code documentation (if needed)

neildaniels · 2020-03-25T18:36:17Z

This looks awesome! 🔥

afbora · 2020-03-25T19:23:00Z

I added the promise() method to manage the hooks more easily.

#2541

distantnative · 2020-03-28T10:48:58Z

I think I need an actual example to really grasp what this is for...

afbora · 2020-03-28T11:21:47Z

Currently we can manage the hooks in a limited way. To give the simplest example, we cannot use more than one instance from the same hook. With the Hook class, we add flexibility to the hooks and increase what its can do.

Btw i'm thinking of a few new features such as promise() on the hooks: when(), template(), group(), etc...

when()

return [
    'hooks' => [
        'page:before' => function ($hook) {
            // page.create:before
            $hook->when($condition, 'create', function ($page, $input) {
                // your codes
            });
        }
    ]
];

template()

return [
    'hooks' => [
        'page:before' => function ($hook) {
            // page.create:before
            $hook->template('product', 'create', function ($page, $input) {
                // your codes
            });
        }
    ]
];

group()

return [
    'hooks' => [
        'page:before' => function ($hook) {
            $hook->group(function () {
                $this
                    // page.create:before
                    ->promise('create', function ($page, $input) {
                        // your codes
                    })
                    // page.create:before (same action can use multiple)
                    ->promise('create', function ($page, $input) {
                        // your codes
                    })
                    // page.update:before
                    ->promise('update', function ($page, $values, $strings) {
                        // your codes
                    })
                    // page.delete:before
                    ->promise('delete', function ($page) {
                        // your codes
                    });
                });
        }
    ]
];

distantnative · 2020-03-28T13:48:48Z

Looks all very sophisticated, but what exactly for?

afbora · 2020-03-28T14:08:23Z

For example; In a simple e-commerce application, I need to write a code like this to follow the changes or relationships in the products and category pages. It doesn't seem useful to me at all. Indeed, only page:before and page:after enough for this.

return [
    'hooks' => [
        'page.create:before' => function ($page, $input) {
            if ($page->intendedTemplate() === 'product') {
                // your code goes here
            } elseif ($page->intendedTemplate() === 'category') {
                // your code goes here
            }
        },
        'page.create:after' => function ($page) {
            if ($page->intendedTemplate() === 'product') {
                // your code goes here
            } elseif ($page->intendedTemplate() === 'category') {
                // your code goes here
            }
        },
        'page.update:before' => function ($page, $values, $strings) {
            if ($page->intendedTemplate() === 'product') {
                // your code goes here
            } elseif ($page->intendedTemplate() === 'category') {
                // your code goes here
            }
        },
        'page.update:after' => function ($newPage, $oldPage) {
            if ($newPage->intendedTemplate() === 'product') {
                // your code goes here
            } elseif ($newPage->intendedTemplate() === 'category') {
                // your code goes here
            }
        },
        'page.duplicate:before' => function ($originalPage, $input, $options) {
            if ($originalPage->intendedTemplate() === 'product') {
                // your code goes here
            } elseif ($originalPage->intendedTemplate() === 'category') {
                // your code goes here
            }
        },
        'page.duplicate:after' => function ($duplicatePage) {
            if ($duplicatePage->intendedTemplate() === 'product') {
                // your code goes here
            } elseif ($duplicatePage->intendedTemplate() === 'category') {
                // your code goes here
            }
        },
        'page.delete:before' => function ($page) {
            if ($page->intendedTemplate() === 'product') {
                // your code goes here
            } elseif ($page->intendedTemplate() === 'category') {
                // your code goes here
            }
        },
        'page.delete:after' => function ($status, $page) {
            if ($page->intendedTemplate() === 'product') {
                // your code goes here
            } elseif ($page->intendedTemplate() === 'category') {
                // your code goes here
            }
        }
    ]
];

neildaniels · 2020-03-28T18:40:28Z

@distantnative Here's a sample of code that motivated my original feature request. -> queueCacheInvalidation() is a custom page/file/site method, but you could imagine any logic going here.

Kirby::plugin('thestreamable/cacheclearqueue', [
    'hooks' => [
        // Page
        'page.changeNum:after' => function (Page $page, Page $oldPage) {
            $page->queueCacheInvalidation('page.changeNum:after', $oldPage);
        },
        'page.changeSlug:after' => function (Page $page, Page $oldPage) {
            if ($page->isPublished()) {
                $page->queueCacheInvalidation('page.changeSlug:after', $oldPage);
            }
        },
        'page.changeStatus:after' => function (Page $page, Page $oldPage) {
            $page->queueCacheInvalidation('page.changeStatus:after', $oldPage);
        },
        'page.changeTemplate:after' => function (Page $page, Page $oldPage) {
            if ($page->isPublished()) {
                $page->queueCacheInvalidation('page.changeTemplate:after', $oldPage);
            }
        },
        'page.changeTitle:after' => function (Page $page, Page $oldPage) {
            if ($page->isPublished()) {
                $page->queueCacheInvalidation('page.changeTitle:after', $oldPage);
            }
        },
        'page.create:after' => function (Page $page) {
            if ($page->isPublished()) {            
                $page->queueCacheInvalidation('page.create:after');
            }
        },
        'page.delete:after' => function (bool $force, Page $oldPage) {
            if ($oldPage->isPublished()) {            
                $oldPage->queueCacheInvalidation('page.delete:after');
            }
        },
        'page.duplicate:after' => function (Page $page) {
            if ($page->isPublished()) {
                $page->queueCacheInvalidation('page.duplicate:after');
            }
        },
        'page.update:after' => function (Page $page, Page $oldPage) {
            if ($page->isPublished()) {            
                $page->queueCacheInvalidation('page.update:after', $oldPage);
            }
        },
        
        // File
        'file.changeName:after' => function (File $file, File $oldFile) {
            $file->queueCacheInvalidation('file.changeName:after', $oldFile);
        },
        'file.changeSort:after' => function (File $file, File $oldFile) {
            $file->queueCacheInvalidation('file.changeSort:after', $oldFile);
        },
        'file.create:after' => function (File $file) {
            $file->queueCacheInvalidation('file.create:after');
        },
        'file.delete:after' => function (bool $force, File $oldFile) {
            $oldFile->queueCacheInvalidation('file.delete:after');
        },
        'file.replace:after' => function (File $file, File $oldFile) {
            $file->queueCacheInvalidation('file.replace:after', $oldFile);
        },
        'file.update:after' => function (File $file, File $oldFile) {
            $file->queueCacheInvalidation('file.update:after', $oldFile);
        },
        
        // Site
        'site.update:after' => function (Site $site, Site $oldSite) {
            $site->queueCacheInvalidation('site.update:after', $oldSite);
        },
    ],
]);

With this PR, it could be condensed down to something resembling:

Kirby::plugin('thestreamable/cacheclearqueue', [
    'hooks' => [
        'page:after' => function ($hook) {
            if ($hook->page()->isPublished() || !in_array($hook->action(), [
                'changeSlug',
                'changeTemplate',
                'changeTitle',
                'create',
                'delete',
                'duplicate',
                'update',
            ]) {
                $hook->page()->queueCacheInvalidation($hook->name(), $hook->oldPage());
            }
        },
        
        'file:after' => function ($hook) {
            $hook->file()->queueCacheInvalidation($hook-name(), $hook->oldFile());
        },
        
        'site:after' => function ($hook) {
            $hook->site()->queueCacheInvalidation($hook-name(), $hook->oldSite());
        },
    ],
]);

It's less code, yes, but the more interesting aspect is that: as new hooks are added, this code will automatically be involved in those hooks. If there's a new file hook added in a future version, this will continue being notified about that hook.

neildaniels · 2020-03-28T18:48:59Z

Admittedly though, I don't find the promise() method and proposed when(), template(), group() methods particularly compelling.

Considering that something like page:after needs to be a callback, adding more callbacks within that callback seems unnecessary—especially because you cannot pass in callbacks from out side the plugin setup.

Personally, I find this:

Kirby::plugin('thestreamable/testplugin', [
    'hooks' => [
        'page:after' => function ($hook) {
            $hook->promise('create', function ($page) {
                // do something
            });
        },
    ],
]);

much less clear than simply doing:

Kirby::plugin('thestreamable/testplugin', [
    'hooks' => [
        'page:after' => function ($hook) {
            if ($hook->action() == 'create') {
                // do something
            }
        },
    ],
]);

And, in a "single" use case like that, I think the existing syntax preferable.

Kirby::plugin('thestreamable/testplugin', [
    'hooks' => [
        'page.create:after' => function ($hook) {
            // do something
        },
    ],
]);

lukasbestle

I agree with Neil that the current implementation is a bit too complex and does too much. I think we should keep this as simple as possible for now to make it easier to understand. Which means that the Hook should probably only have the methods it absolutely needs.

Regarding implementation:

Now that I think about it, I'm not sure about the different arguments for "old-style" hooks and the new global hooks. What we could do is to use ReflectionFunction like in the Toolkit\Controller class to determine at run-time which arguments each individual hook would like to receive. We could even support arbitrary argument orders that way and it would also be possible to receive $hook in addition to some or all of the individual arguments.
Triggering both the individual as well as the "global" hooks should be automated in the $app->trigger() method. So $app->trigger() would ideally only take a Hook object and then determine the hooks to call from that.
I'm not sure how we can make this feature work with $app->apply(). To make it all consistent, everything we change about $app->trigger() should also be changed for $app->apply(). But that's going to be a bit difficult.

#2541

afbora · 2020-04-01T12:41:03Z

Implemented ReflectionClass to access parameters with names (reverted associative arrays).
Refactored App::trigger method that support getting Hook object
For now i have no idea about what should i do for App::apply method

I do not fully agree with you about like promise(), when(), template(), group() helper methods. You don't want to constantly write if else or switch cases in the codes. This situation gets the readability of the codes first and then your spirits :) These helper methods will not disrupt the simple usage and current process.

My personal opinion is; It just will help developers by adding flexibility.

#2541

lukasbestle · 2020-04-05T15:41:45Z

In case you are wondering: I've rebased this PR against develop to see the current unit test results, I didn't change anything else so far.

I've taken a look at your updated implementation. A bit of feedback:

Instead of having an inline closure inside $app->trigger(), the method can recursively call itself to trigger the wildcard hook.
I think you misunderstood the reflection part. I meant that the hook callback (defined in a plugin or in the config) is dynamically checked for the params it wants, not the constructor of the Hook class (like the Controller class that checks which params the individual controller wants). This check can only be done in the $app->trigger() method for each hook function it triggers individually.
We probably actually need the param names for this that you have removed again in the last commit.

@afbora Do you want to continue with the implementation or do you want me to take over? Either way is fine for me.

Regarding the helper methods:

I see your point, but the issue with closures is that you need to manually pass through all variables to them with function(...) use(...). People will forget that and that's likely going to lead to quite a few support requests. Given that the helper methods don't add functionality you can't already achieve with built-in PHP features (they are just syntactic sugar), I think this is not worth it.

However we can reconsider this later, but only after this PR is merged. This PR is going to be quite complex in itself and the more features we can keep out of it for later, the better we'll be able to keep an overview. I hope you understand.

lukasbestle · 2020-04-05T19:45:36Z

Nope. :)

getkirby/ideas#194

Allows for arbitrary argument orders in hook functions as well as for a new optional `$event` argument for every hook. NOTE: Breaking-change as the method signature of $app->trigger() and $app->apply() has changed. Implements getkirby/ideas#194.

getkirby/ideas#194

lukasbestle · 2020-04-13T20:45:32Z

This PR is now ready for review. I have also updated the first post of this PR with the details on what exactly has changed, how to use it and which changes are breaking-changes.

I'm really looking forward to your feedback! In my opinion the named arguments, the $event object and the wildcard hooks are three features that are very powerful and incredibly flexible when used together (or even on their own). The breaking-changes are something to be aware of, but I think they are a one-time effort that is completely worth it in the long run. After all there aren't many other breaking-changes in 3.4.0 so far, so the migration should be doable. 🙂

Thanks a lot to @afbora for the initial inspiration. My implementation works quite differently than the original draft by @afbora, but the draft included a lot of really awesome ideas I could use in the final implementation. ❤️👍😘

src/Cms/App.php

lukasbestle · 2020-04-14T14:13:57Z

To ease the migration to 3.4, I have created a hook migration checker plugin that we can include as a release attachment on GitHub to automate checking the hook arguments for sites and plugins:

And once the issues are fixed:

distantnative · 2020-04-14T16:28:22Z

If we are breaking hooks anyways... could we incorporate this somehow? #2425

lukasbestle · 2020-04-14T16:57:33Z

@distantnative Oh, absolutely. It should actually be very easy to implement now as we no longer need to worry about argument order. I can take a look once this PR is merged.

src/Cms/App.php

afbora · 2020-04-15T10:07:07Z

@lukasbestle I always love your committees ❤️ You thought of everything 🚀 There is nothing left from my initial PR 😄

I haven't seen any issue or missing in a few tests at my local. Everything looks perfect. I'm looking forward to the ideas waiting in your draft 👀

Co-Authored-By: Ahmet Bora <[email protected]>

bastianallgeier · 2020-05-08T14:08:33Z

The wildcards are fantastic and it's awesome that you created the migration tool, @lukasbestle!!

afbora added the type: feature ⭐️ label Mar 25, 2020

afbora self-assigned this Mar 25, 2020

afbora added a commit that referenced this pull request Mar 25, 2020

Add promise method description

b3098f4

#2541

afbora added a commit that referenced this pull request Mar 25, 2020

Write clean commit() method

6405cba

#2541

afbora added a commit that referenced this pull request Mar 25, 2020

Fix doc typos

79c1bd8

#2541

afbora added a commit that referenced this pull request Mar 25, 2020

Rewrite for wildcard hook

f06ca2b

#2541

afbora added a commit that referenced this pull request Mar 25, 2020

Rewrite UserActions for wildcard hook

af7ce7f

#2541

afbora added a commit that referenced this pull request Mar 25, 2020

Add wildcard hooks for user auth

a277adc

#2541

afbora added the missing: test 🧐 label Mar 27, 2020

lukasbestle reviewed Mar 29, 2020

View reviewed changes

afbora added a commit that referenced this pull request Apr 1, 2020

Some changes reverted and refactored hook process

f39a804

#2541

lukasbestle pushed a commit that referenced this pull request Apr 5, 2020

Add promise method description

1258e3e

#2541

lukasbestle pushed a commit that referenced this pull request Apr 5, 2020

Write clean commit() method

af3731f

#2541

lukasbestle pushed a commit that referenced this pull request Apr 5, 2020

Fix doc typos

6eac0d1

#2541

lukasbestle force-pushed the feature/194-wildcard-hooks branch from f39a804 to d18fe4b Compare April 5, 2020 15:12

lukasbestle pushed a commit that referenced this pull request Apr 5, 2020

Rewrite for wildcard hook

cfcf1c3

#2541

lukasbestle pushed a commit that referenced this pull request Apr 5, 2020

Rewrite UserActions for wildcard hook

2b79c0d

#2541

lukasbestle pushed a commit that referenced this pull request Apr 5, 2020

Add wildcard hooks for user auth

97161c8

#2541

lukasbestle pushed a commit that referenced this pull request Apr 5, 2020

Some changes reverted and refactored hook process

d18fe4b

#2541

afbora assigned lukasbestle Apr 5, 2020

lukasbestle force-pushed the feature/194-wildcard-hooks branch 2 times, most recently from cbad0dc to 3bdfffb Compare April 13, 2020 19:19

lukasbestle changed the title ~~Wildcard hooks~~ Improvements to hooks Apr 13, 2020

lukasbestle marked this pull request as ready for review April 13, 2020 19:25

lukasbestle added 5 commits April 13, 2020 22:17

New Kirby\Cms\Event class

1f20dda

getkirby/ideas#194

Adapt core code to trigger()/apply() changes

4597a8f

getkirby/ideas#194

Add support for wildcard hooks

28c5646

getkirby/ideas#194

Wildcard hooks: Full wildcard variation support

617cbdf

getkirby/ideas#194

lukasbestle force-pushed the feature/194-wildcard-hooks branch from 3bdfffb to 617cbdf Compare April 13, 2020 20:36

lukasbestle removed the missing: test 🧐 label Apr 13, 2020

distantnative reviewed Apr 14, 2020

View reviewed changes

src/Cms/App.php Show resolved Hide resolved

lukasbestle mentioned this pull request Apr 14, 2020

Pass $used flag to route:after hook #2418 #2425

Merged

4 tasks

afbora commented Apr 15, 2020

View reviewed changes

src/Cms/App.php Outdated Show resolved Hide resolved

src/Cms/App.php Outdated Show resolved Hide resolved

Fix docblocks

29c3e24

Co-Authored-By: Ahmet Bora <[email protected]>

lukasbestle mentioned this pull request Apr 24, 2020

recursive changeDiruri hook for pages and files getkirby/ideas#538

Closed

neildaniels mentioned this pull request Apr 24, 2020

Add 'cache.pages.flush:before' and 'cache.pages.flush:after' hooks #2582

Closed

4 tasks

Merge branch 'develop' into feature/194-wildcard-hooks

434f4e8

bastianallgeier self-requested a review May 8, 2020 14:07

bastianallgeier approved these changes May 8, 2020

View reviewed changes

bastianallgeier merged commit f9873ca into develop May 8, 2020

bastianallgeier deleted the feature/194-wildcard-hooks branch May 8, 2020 14:08

afbora mentioned this pull request May 8, 2020

Add docs for 3.4.0 getkirby/getkirby.com#954

Closed

Uh oh!

Improvements to hooks #2541

Improvements to hooks #2541

Uh oh!

Conversation

afbora commented Mar 25, 2020 • edited by lukasbestle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the PR

Related issues

Ready?

Uh oh!

neildaniels commented Mar 25, 2020

Uh oh!

afbora commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

distantnative commented Mar 28, 2020

Uh oh!

afbora commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

distantnative commented Mar 28, 2020

Uh oh!

afbora commented Mar 28, 2020

Uh oh!

neildaniels commented Mar 28, 2020

Uh oh!

neildaniels commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukasbestle left a comment

Choose a reason for hiding this comment

Uh oh!

afbora commented Apr 1, 2020

Uh oh!

lukasbestle commented Apr 5, 2020

Uh oh!

lukasbestle commented Apr 5, 2020

Uh oh!

lukasbestle commented Apr 13, 2020

Uh oh!

Uh oh!

lukasbestle commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

distantnative commented Apr 14, 2020

Uh oh!

lukasbestle commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afbora commented Apr 15, 2020

Uh oh!

bastianallgeier commented May 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

afbora commented Mar 25, 2020 •

edited by lukasbestle

Loading

afbora commented Mar 25, 2020 •

edited

Loading

afbora commented Mar 28, 2020 •

edited

Loading

neildaniels commented Mar 28, 2020 •

edited

Loading

lukasbestle commented Apr 14, 2020 •

edited

Loading

lukasbestle commented Apr 14, 2020 •

edited

Loading