Hi guys,
here is my share of thoughts about the callbacks. I am not going to
comment at all on the editable-inline feature, more using it as an
example at time.
Firstly, I think it’s important to draw a line between callbacks and
hooks, they are not the same thing. My rough definition of both would
be:
Callback: A callable that is generating a result
from a trigger. In Moodle code, the core will often sit in the middle
just to be the link between the trigger and the result. There is never
more than one callback called at once, and never less than one. When the
callback does not exist, the code can not run.
Hook: An optional set of callables. The code will
execute just fine when there are no hooks, and if there are there can be
more than one. The hooks are extending the functionality of the code
that implements them.
Following Damyon’s foot steps, here are the requirements for callbacks that I can think of:
- Flexible, developers can organise their code however they want. (As Damyon #1)
- Fast, identifying the callback. (Damyon #2)
- Easy to implement, in core.
- Easy to use, well documented.
- Secure.
I don’t really mind backwards compatibility to be broken, we need to
break things to make them better. Also we can’t make things faster if we
have to follow two paths (new and old). As long as there is a clear
justification and simple migration process it should be fine to not be
backwards compatible.
Why I don’t like classes and interfaces?
I really do not think callbacks should be defined by interfaces or abstract classes. Interfaces are particularly bad because:
- They can not never be changed, or will heavily break backwards compatibility
- The risk of method names clashes increases (I want to use the interfaces on any existing class)
- Using the interfaces on an existing class can not work, only I know how to instantiate my object
- If the interface defines static methods, then classes are just placeholders for functions
- If I properly use the interfaces (e.g. mod_assign implements
callback), then ‘$class instanceof interface’ is not very secure any
more.
Classes are not better because you still need to create a class, and
you can not have more than one callback per class. And just as
interfaces they are just placeholders for static methods, or they can be
instantiated but that probably has no value.
More importantly, I find that both concepts encourage developers to
duplicate their code, thus reducing maintainability and security. What I
am trying to say is that the callback callbable should only ever
receive arguments, forward those to the main ‘controller’, then return
the result. A class or an interface to do that is a serious overkill.
And encouraging splitting the code in different places is a serious
problem too.
Also, if we take the example of editable-inline thingy, why don’t we
implement the methods ‘can_edit()’, and ‘validate_param()’. All the
plugins will have to check the permissions, and validate the parameters,
and maybe do other things as well… So, we add those to the
interface/class and now clearly all the logic that has already been
implemented elsewhere has to be duplicated in the callback class.
I vote against interfaces and subclasses.
So what do we do?
We need callbacks to be flexible, so developers should be able to
place them wherever they want. But it needs to be secure, we can not
really accept a callable name to be passed around by the client. We also
need good documentation, which means that it should be easy for the
developer to find out what a callback receives, and what it should
return. And it needs to be blazing fast, we don’t want to start loading
an entire PHP file to check if a single function exists.
Let’s store the callbacks that a plugin defines in a file,
db/callbacks.php. The plugin developers will have to state the name of
their callback, what they are meant to be used with, and the callable to
use (let’s assume it is autoloaded). Here is an example:
$callbacks = array(
'mod_plugin:core_do_that' => array(
'\mod\plugin\my_favourite_class::my_do_that',
'accepts' => 'core_do_that'
);
)
In core, we would have something like this:
$callbackname = 'mod_plugin:core_do_that'; // Provided by client, insecure.
$callbackargs = array();
$callbackinfo = get_callback_info($callbackname);
$callable = $callbackinfo->callable;
if ($callbackinfo->accepts != 'core_do_that' || empty($callable) || !is_callable($callable)) {
// Abort!
die();
}
call_user_func_array($callable, $callbackargs);
With that, we probably have a good security as
‘mod_plugin:core_do_that’ defines what callback it can be used with
(accepts), and we can not substitute it with a random callback or
callable. It’s also simple because my callable can be defined anywhere,
and it is fast because the list of callables will be cached somewhere.
We still have a problem though, how do we document the callback
mechanism, and its arguments? We could force ourselves to write
documentation, but let’s be honest, that never works. An other option is
to create one class, but this time the hard work is on the core
developer, not on the part of the user of the callback.
I have not thought about this extensively, but here is an example:
namespace core\callback;
abstract base {
public __construct($args) {
foreach ($args as $key => $value) {
if (developer mode) {
}
$this->$key = $value;
}
}
}
class do_that extends base {
public $name;
public $title;
public $description;
}
And if I update the core example above, we would have this:
$callbackname = 'mod_plugin:core_do_that'; // Provided by client, insecure.
$callbackinfo = get_callback_info($callbackname);
$callable = $callbackinfo->callable;
if ($callbackinfo->accepts != 'core_do_that' || empty($callable) || !is_callable($callable)) {
// Abort!
die();
}
$callbackarg = new \core\callback\do_that(array(
'name' => 'A name',
'title' => 'A title',
'description' => 'A description'
));
call_user_func($callable, $callbackarg);
Potentially, we could have also have something like:
$callback = new \core\callback\do_that(array(
'name' => 'A name',
'title' => 'A title',
'descriptuon' => 'A description'
));
$callbackname = 'mod_plugin:core_do_that'; // Provided by client, insecure.
$callback->call($callbackname); // Validates the callbackname.
This looks very similar to events, but has a different purpose and is
a lot less complex. It’s also, like the observers, very gentle with
developers who implement the callables.
I realise that callbacks should have a specific and limited purpose,
if not they would probably be better as whole new API, or as hooks, or
who knows. E.g. if you want to find all the plugins that accept a certain type of callback you are probably thinking of hooks instead.
Why am I wrong?
After writing all this I tried to apply (in my head) the solution to
two examples, the inline-editing, and the javascript forms. In the case
of inline-editing my example works fine, but not so much for forms.
Simply, the form requires an arbitrary list of arguments that will
differ depending on the callback. That is very interesting, callbacks
can receive very different arguments even within the same API.
So, I think I was wrong to suggest the \core\callback\base class. It will only work when the API enforces the arguments that a callback is supposed to receive.
To summarise, I would:
- Define the callbacks in a callback file
- Attach a ‘type’ name to the callback trigger
- Check the ‘type’ of the callback callable prior to running it
- Document how the API works, just like any other API, without the need for a new class
That’s it, that’s my opinion for the day. It’s not unlikely that I will disagree with myself next week.
Thanks for reading!
Fred
