Moodle in English: Hooks as replacement for lib.php callbacks

Hooks as replacement for lib.php callbacks

by Petr Skoda - Monday, 13 June 2022, 7:21 PM

Hello developers,

long time ago there was a proposal in MDL-44078 to add support for hooks as a replacement for lib.php plugin callbacks. Hooks main purpose would be to allow two-way communication between 2 or more plugins.

I would like to propose it again because I believe that hooks using autoloaded classes would be much easier to use than poorly documented lib.php plugins callbacks.

For more details see MDL-74954, in short the hook classes would be stored in some_plugin\hook\ namespace extending \core\hook\base class. Other plugins could register hook callbacks by adding file db/hooks.php. Overall it would be very similar to events.

<?php 
// Contents of file admin/tool/recyclebin/db/hooks.php 
$callbacks = [
    [
        'hook' => core\hook\course_delete_pre::class,
        'callback' => 'tool_recyclebin\hook_callbacks::course_delete_pre',
        'priority' => 0, // Execute as the last one to get more consistency.
    ],
];

All hook meta data would be included in the hook class, so we could even create a simple administrative page that lists all available hooks, their callbacks and even deprecated lib.php functions.

hooks overview page

Average of ratings: Useful (1)

Re: Hooks as replacement for lib.php callbacks

by Petr Skoda - Tuesday, 21 June 2022, 6:58 PM

I've added some basic docs for proposed Hook API using the new devdocs system https://github.com/skodak/devdocs/blob/MDL-74954/docs/apis/core/hooks/index.md

I must say the new docs system for developers is pretty neat.

Average of ratings: -

Re: Hooks as replacement for some one-to-many lib.php callbacks

by Petr Skoda - Wednesday, 22 June 2022, 5:04 AM

To make it clear, I am proposing to only replace some one-to-many callbacks looking for any plugin type (*) described in https://docs.moodle.org/dev/Callbacks, the use cases are often classed as "hacks". Some good examples for conversion to hooks are after_config, after_require_login and extend_navigation.

There there are some one-to-many callbacks that should not be considered for conversion to hooks, for example supports_logstore which is used for reports only.

I am going to create a list of current callbacks where hooks would be more beneficial than plugin feature interfaces already proposed by Andrew .

Average of ratings: Useful (1)

Re: Hooks as replacement for lib.php callbacks

by Petr Skoda - Tuesday, 2 August 2022, 2:13 PM

Hi,

it seems that the file naming db/hooks.php might be a bit confusing, maybe we should rename it to db/hook_callbacks.php to make it clear for developers what it actually defines?

This is similar to events, I think instead of db/events.php I should have named the files db/event_listeners.php to make it clear what it is, but it is way too late for that.

Average of ratings: -

Re: Hooks as replacement for lib.php callbacks

by Mark Johnson - Tuesday, 2 August 2022, 3:56 PM

I agree that hook_callbacks.php is clearer than hooks.php.

Average of ratings: -

Re: Hooks as replacement for lib.php callbacks

by Michael Hughes - Tuesday, 2 August 2022, 4:38 PM

Given "hooks as a replacement for lib.php plugin callbacks" doesn't this then uncomfortable merge these 2 terms in to a hybridised thing?

Callback: a callback or callback function is any reference to executable code that is passed as an argument to another piece of code (https://en.wikipedia.org/wiki/Callback_(computer_programming)

Hook: Code that handles intercepted function calls, events or messages is called a hook. (https://en.wikipedia.org/wiki/Hooking)

For me "hook_callbacks" is an uncomfortable mashing together. Something that indicates purpose rather than mechanism would (IMHO) be better:

* extends.php

* extensions.php

Even the "original" hooks.php to me sort of makes sense, especially if that's the only / specific technique that is being used / going to end up with.

But to be fair a "painting the bike shed" conversation

as long as the docs clearly state the purpose of a given named file it shouldn't matter, the filename itself shouldn't be responsible for doing that on it's own.

Average of ratings: Useful (2)

Re: Hooks as replacement for lib.php callbacks

by Petr Skoda - Tuesday, 2 August 2022, 5:06 PM

Good point Michael, the original hooks design was updated to be more compliant with the PSR-14. The main trouble is we already have events in Moodle, so I ended up with this mapping:

PSR-14	Hooks
Event	Hook
Listener	Hook callback
Emitter	Hook emitter
Dispatcher	Hook dispatcher (implemented in Hook manager)
Listener Provider	Hook callback provider (implemented in Hook manager)

see proposed dev docs at https://github.com/skodak/devdocs/blob/MDL-74954/docs/apis/core/hooks/index.md for more details

Average of ratings: -

Re: Hooks as replacement for lib.php callbacks

by Michael Hughes - Tuesday, 2 August 2022, 5:48 PM

I think this is why for me simply "hooks" is sufficient, therefore "db/hooks.php" and they are different from "callbacks".

Average of ratings: Useful (1)

Re: Hooks as replacement for lib.php callbacks

by Tim Hunt - Tuesday, 2 August 2022, 7:02 PM

Do we need this file at all?

In lib.php, we had a hook ..._extends_settings_navigation, and to implement the hook, you just created that function and it worked.

In future, we want auto-loaded classes, not function, but shouldn't it work the same? If you implement a class with the right name in the right namespace, with the right base class/interface, it should just work. At least, that is what I would do.

Average of ratings: Useful (2)

Re: Hooks as replacement for lib.php callbacks

by Petr Skoda - Tuesday, 2 August 2022, 7:44 PM

Hi Tim, it guess it is this file or a complex class + methods naming standards used to find which plugin wants to register hook callbacks.

Option 1 - db/hook(_callback)s.php file

1. Developer that wants to add a hook callbacks uses arbitrary class where he adds a method with arbitrary name. The only requirement for callback is that it accepts hook instance as parameter and returns void.

2. Then developer just adds entry to db/hook_callbacks.php file.

We already have something similar in Moodle - events. In fact I was copying code and logic from events when I was doing the initial proof-of-concept patch.

Option 2 - no hooks file

1. We would have to define naming standard for class that holds all callbacks in a plugin - simple, for example \some_plugin\hook_callbacks

2. We would have to decide on mapping standard from full hook class name to a callback method name - not trivial due to backslashes in class names., or there would have to be a method that returns the mapping of hooks to callback methods.

3. Lastly we would need a way to describe default callback priorities - I guess that could be method in the hook_callbacks class

During runtime we would have to use PHP introspection to find out what callbacks each plugin registered.

Adding interfaces to callbacks from the plugin where the hook is triggered would be a bad idea in my opinion because you could end up with fatal errors after plugin uninstallation.

Which one is better?

No idea. For me I do not care, anything that would get this into Moodle core is fine for me. My guess would be that option 1 with hooks file in db folder is more flexible and easier to use for developers, but I understand some devs might have a different preference.

Pleas note that option 2 does not allow a plugin to have two callbacks for the same hook with different priority. I was also considering wildcard callbacks (which I rejected for now), I suppose those might not be feasible with rigid naming standards in option 2 in the future.

I can guarantee the option 1 is a viable option (earlier design was used in a competing product for a few years), but I am not sure if we could make a simple future-proof naming standard for option 2 before Moodle 4.1 release.

Average of ratings: -

Re: Hooks as replacement for lib.php callbacks

by Petr Skoda - Tuesday, 2 August 2022, 8:06 PM

Or did you just mean to move the contents of the db/hooks.php to a method in autoloaded class?

Option 3 example

This class would replace db/hooks.php in Option 1, the rest would be pretty much the same.

<?php
namespace mod_activity;

class hook_callbacks implements \core\hook\callbacks {
    public static function get_callbacks(): array {
        return [
            [
                'hook' => \mod_activity\hook\installation_finished::class,
                'callback' => 'local_stuff\local\hook_callbacks::activity_installation_finished',
                'priority' => 500,
            ],
        ];
    }
}

Average of ratings: -

Re: Hooks as replacement for lib.php callbacks

by Mark Johnson - Tuesday, 2 August 2022, 9:16 PM

Honestly, I think all these approaches have their merits. Option 2 would be nice, but as you say it opens up further discussion. Option 3 probably balances the "make it nice" with "get it done".

Edit: Although I think it that example should probably be \mod_activity\hook\callbacks rather than \mod_activity\hook_callbacks. I dont think a core API has the right to reserve a class name in the plugin's top level namespace?

Average of ratings: Useful (1)

Re: Hooks as replacement for lib.php callbacks

by Petr Skoda - Tuesday, 2 August 2022, 10:15 PM

I do like "\mod_activity\hook\callbacks"!

hmm we could also create a class for callback description so that it is not a simple array, but something that helps developers to define the callbacks correctly:

<?php
namespace local_stuff\hook;

class callbacks implements \core\hook\callbacks {
    public static function get_callbacks(): array {
        return [
            new \core\hook\callback(
                \mod_activity\hook\installation_finished::class,
                'local_stuff\local\hook_callbacks::activity_installation_finished',
                500
            ),
        ];
    }
}

Average of ratings: Useful (1)

Re: Hooks as replacement for lib.php callbacks

by Tim Hunt - Friday, 5 August 2022, 3:13 PM

I am thinking 'none of the above'.

I al thinking that, for each specific hook, or group of related hook, there would be a class you implement if you want to intercept that. For example, if you want to use the https://docs.moodle.org/dev/Output_callbacks callbacks, then you have to create .../classes/output/page_rendering_callbacks.php in your plugin, extending a particular base class, so you only have to override the methods you actually want (e.g. before_header).

This means that the all plugins implement the hook consistently, making it easy to global search for uses.

I know there is a developer meme "if in dobut, add another level of interection", but that is not always right. Sometimes it is necessary flexibility, but if it is not, it us just unnecessary obfuscation.

Average of ratings: -

Re: Hooks as replacement for lib.php callbacks

by Petr Skoda - Friday, 5 August 2022, 8:20 PM

If I understand it correctly, Tim is proposing:

Option 4

The callback lookup would be searching for a class in a fixed subnamespace in each plugin. We would need a non-trivial rule for mapping of hook class name to an observer namespace. I was not able to find any simple and elegant solution, this is the best I could do as an example:

<?php

namespace local_stuff\hook\callback\mod_activity;

class installation_finished implements \core\hook\callback {
    public static function execute(\mod_activity\hook\installation_finished $hook): void {
        // do something with $hook
    }

    public static function get_priority(): int {
        return 100;
    }
}

I believe that naming rules in option 2 and 4 would be very difficult to finalise in short amount of time we have before 4.1, so if anybody is in favour of these please come up with exact naming rules now, I failed to do so myself and I am not planning to work on them.

Average of ratings: -

Re: Hooks as replacement for lib.php callbacks

by Tim Hunt - Friday, 5 August 2022, 10:14 PM

You have failed to understand what I am suggesting in several important ways.

Hooks are not a major thing in themeslves. They are a way of getting things done, but the importnat thing is what you are getting done, not the fact it is a hook. Having a core_hook component makes about as much sense to me as having a core_phpfunction or core_foreach component.

Hooks are (if this proposal are accepted) become part of the way that different components can communicate (in a way that does not make spaghetti).

Go back and look at my previous post. I was suggesting that the way to implement a paticular core_output hook is to write the class local_stuff\output\page_rendering_callbacks. To make it clear, the base class should be core_output\page_rendering_callbacks_base. (base class is better than interface here, because there are 5 methods available, and you probalby only want to implement one, so the base class can give 'do nothing' implementaions for the rest.

Also, i strongly reject the argument "we should rush something that might end up sucking into 4.1 because there is not time to do better". If this is worth doing, this is worth doing properly. If you can't do it properly in time for 4.1, then it will have to go into 4.2.

Average of ratings: Useful (1)

Re: Hooks as replacement for lib.php callbacks

by Petr Skoda - Friday, 5 August 2022, 11:13 PM

My understanding is that Moodle has a need for something like PSR-14. The best example would be callbacks like after_config, after_require_login, override_webservice_execution. The current plugin callbacks have known problems.

I do not think the discussion here should be about the details of conversion of all existing callbacks. In my opinion each of the current callbacks will need to be discussed separately, I do expect there will be some arguments there.

You are right Tim that some of callbacks may not be good candidates for hooks conversion. I do not know what is the best way to solve page rendering customisations. Your example with core_output\page_rendering_callbacks_base looks like a completely different style of API that is not related to PSR-14.

Postponing merging into 4.2 is an option too. I can do the coding in the next few months, but I do not know if I will have time and funding to do it later next year. I do not want to get into situation where we introduce a new style of API and do not convert/deprecate all legacy uses before the release. I have put a lot of emphasis on the backwards compatibility and migration tools in the current patch.

Anyway thanks a lot for your feedback Tim and everybody else. I am going to create a new set of patches to illustrate option 3 and add a few better examples of hook conversions.

Average of ratings: -

Re: Hooks as replacement for lib.php callbacks

by Tim Hunt - Saturday, 6 August 2022, 1:40 AM

PSR-14 (https://www.php-fig.org/psr/psr-14/ - I had to Google it) seems to be about events and observers. Moodle already has that, right? So, just to say I am now completely confused about what you are trying to achieve and why.

Average of ratings: -

Re: Hooks as replacement for lib.php callbacks

by Petr Skoda - Saturday, 6 August 2022, 3:13 AM

PSR-14 events do not match Moodle events, they have different uses. I had to come up with a different name "hook" to implement PSR-14 support in Moodle. The differences between hooks and Moodle events are:

1. events are triggered after something, hooks can happen at any time
2. events are one one-way communication only, event data cannot be modified; hooks allow communication in any direction, hook instance data can be modified
3. all events are logged; hooks are not logged
4. hooks can be used in installation and upgrades, use in low level api is ok, transactions do not affect hooks, hooks can be even used in phpunit internals
5. hooks have low overhead; events may take memory in db transactions, logging cannot be skipped for events

see

https://github.com/skodak/devdocs/blob/MDL-74954/docs/apis/core/hooks/index.md for name mapping of hook patch compared to PSR-14 events.

Average of ratings: -

General developer forum

Hooks as replacement for lib.php callbacks

Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for some one-to-many lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Option 1 - db/hook(_callback)s.php file

Option 2 - no hooks file

Which one is better?

Re: Hooks as replacement for lib.php callbacks

Option 3 example

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Option 4

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Re: Hooks as replacement for lib.php callbacks

Empowering educators to improve our world

Moodle

Support

Get Involved

Contributions

Downloads

Tracker

Development

Empowering educators to improve our world