New (experimental) feature in the AFC plugin - Event handler priority

businessdad · October 2014

I needed to be able to determine in which order event handlers are executed and, since Vanilla doesn't yet implement such feature, I decided to give it a shot.

My requirements

The solution must be 100% backward compatible (as per my usual standard). The implementation must not require any modification that would break if the priority mechanism is removed (i.e. handlers must be declared exactly as they are now, no new syntax).
Existing plugins, themes and applications must continue working without modification (with the exception of the ones that need events to be fired in a specific priority, of course).
Handler priority must be optional. When no priority is specified, a default one should be taken.

My solution

In a word: annotations. Annotations are a controversial tool, which gets mixed responses from the development community. I know developers who love them, others who hate them. Personally, I believe that every tool ever invented has its use, in specific time and place, and this is one of that cases.

Simply specify a priority as part of a PHP-DOC block, before the event handler, and the AFC plugin will do the rest, sorting and firing the handlers in the correct order.

How to use the new feature

Installation

Get the development version of the AFC plugin from the BitBucket repository. The branch to take is Feature-NewPluginManager.
Copy the AeliaFoundationClasses folder to your Vanilla installation.
Move to the AeliaFoundationClasses folder and run composer update, to make sure that the autoloader is updated. If you don't have Composer installed, simply follow the instructions on its website to get it.
Install the plugin on your Vanilla site. I'm testing it on Vanilla 2.1.3, since 2.0.x will be dismissed at the end of the year.
For the annotations to work, the AFC plugin must replace the standard PluginManager class with its own. This is done dynamically, so that core files are untouched. To apply the override, copy (or symlink) file AeliaFoundationClasses/bootstrap/bootstrap.late.php to your Vanilla Forums conf folder. If you already have such file in your installation folder, just copy the contents of the file from the AFC plugin to the one you already have (possibly, at the beginning of it).
Done. The event handler priority feature is now available on your site.

How to specify the priority for an event handler

Here comes the challenge! Well, not really. Let's suppose that you have a plugin which implements the Base_Render_Before handler. Normally, your method would look like this.

/**
 * Doc block. As a diligent developer, you surely have added a block like this to every method.
 * 
 * @param Gdn_Controller Sender The controller that fired the event.
 */
public function Base_Render_Before($Sender) {
  // Your code
}

For some reason, you now want to make sure that your handler is fired last, after all handlers with the same name. You will therefore add a priority, as follows:

/**
 * Doc block. As a diligent developer, you surely have added a block like this to every method.
 * 
 * @param Gdn_Controller Sender The controller that fired the event.
 * @priority 100
 */
public function Base_Render_Before($Sender) {
  // Your code
}

Done. Your handler will now be fired last.

FAQ

What are valid values for a priority?

The priority is an integer. Any integer number will work.

What is the default priority for handlers, if not specified?

The default priority is 10. This number has been chosen semi-arbitrarily, as it's the default priority of filters and actions in WordPress.

How can I ensure that a handler is fired earlier than another?

You must know the target handler's priority and set your handler with a higher one. The lower the number, the higher the priority. If you need Handler A to be fired before Handler B, which has a priority of 7, set the priority on Handler A to 6 or less.

What if I decide that I don't actually need the handler priority feature anymore?

That's where the backward compatibility kicks in. If you decide that the handler priority feature is not for you, simply delete the conf/bootstrap.late.php file from your Vanilla installation, and your handlers will work as they always did. The @priority tag will simply be ignored, and you can remove it at your convenience. Or you can leave it there, so that you can get crazy a couple of months later trying to figure out what it means.

The installation is complicated, can it be made simpler?

This feature is experimental, therefore it should be used by developers who are familiar with this kind of stuff. A "ready made, high fat, processed food" version of the plugin containing the feature will be released later on.

R_J · October 2014

Great addition to the functionality! I've built two plugins where I had to change the "priority" by renaming the plugin which is really ugly.

I had to do some lookup on annotations and found that presentation: http://slideshare.net/rdohms/annotations-in-php-they-exist-38513622 and I think that using annotations in such an extension really make sense (though I would hate to see them used as a control structure in official source code)

x00 · October 2014

In python what looks like annotations are decorators. For instance

@login_required
def profile(request):
   render(request, {})

businessdad · October 2014

@x00 said:
In python what looks like annotations are decorators.

Yes, they also exist in C#, Java and many other languages. They are a useful too, like the Stored Procedures for RDBMS, when used appropriately, and a nightmare to maintain when used "just because".

x00 · October 2014

yes decorators are exactly that not annotations, they pass the function to another function.

http://alessiodellamotta.com/2012/10/04/python-decorators-vs-java-annotations/
http://supercoderz.in/2012/05/17/how-python-decorators-are-better-than-java-annotations/

hgtonight · October 2014

This is a feature I think would be awesome to be in core.

But...

I am definitely one that hates using annotations for anything related to program control. Comments shouldn't change execution if removed, all that jazz.

Have you thought about stuffing a priority array into the plugin info instead? This makes sense semantically. You could even just set a default priority for all your handlers.

I am imagining something like this:

$PluginInfo['TestingGround'] = array(
    'Name' => 'TestingGround',
    'Description' => 'Adds descriptions to the permission edit pages. Designed to help new admins understand what permissions affect.',
    'Version' => '0.1',
    'DefaultPriority' => 2,
    'RegisterHandlerPriority' => array(
      'Gdn_Form_BeforeBodyBox_Handler' => 15,
      'ArticleController_BeforeArticleMeta_Handler' => 9,
      // etc.
    )
);

Thanks for sharing this great work!

x00 · October 2014

@hgtonight that is a good idea, though semantically we are dealing with implicit hooks ideally you would have

object_method_handler(sender, args, priority=3)

obviously your idea is a workable solution, albeit a tad clunky.

hgtonight · October 2014

@x00 said:

object_method_handler(sender, args, priority=3)

Can you get the default value of an argument via reflection?

I agree putting priority in the info section is a little clunky, but remember that this is a rarely needed feature (although I could imagine it being configurable with a nice drag and drop interface in the future).

EDIT - Yes you can: http://php.net/manual/en/reflectionparameter.getdefaultvalue.php. This sounds like the best solution.

businessdad · October 2014

@hgtonight said:
I agree putting priority in the info section is a little clunky, but remember that this is a rarely needed feature (although I could imagine it being configurable with a nice drag and drop interface in the future).

As I mentioned, not everybody likes annotations, but I definitely prefer them to the idea of having the stuff in the plugin info. Annotations have the advantage of being next to the method, are easy to find and to modify. Having to duplicate a method name in a plugin info is also a bad idea, in my opinion.

Also, I absolutely dislike the idea of drag and drop to configure priorities. Handler priority should never be controlled by a GUI, it's something that drives the code depending on specific needs. It's up to a plugin developer to know when an event should be fired, not to a site administrator, and such priority should be hard-coded, not configurable. I can already imagine "Today let's fire this first and see what happens".

A definite no from me (to the point that I would remove such feature if that were ever implemented). It's really a "don't you even dare" type of "no".

It looks like that for the first time since we collaborated, @hgtonight and I hit a "steel wall of conflict".

businessdad · October 2014

@hgtonight said:
I am definitely one that hates using annotations for anything related to program control. Comments shouldn't change execution if removed, all that jazz.

One clarification: the annotations are in the comments just because there is no other way to have them in PHP. Other languages support them natively, before and after method declarations, and they are part of the code. Unfortunately, such syntax is not available, and that's why they end up in the documentation blocks. If there were a way to remove them and use REAL annotations (or decorators), I would definitely remove them from the blocks.

If you want to see it differently, imagine the comment block and the annotations as separate things. Yes, they are in the same container, wrapped by asterisks, but that's just because there is no other way to add them. That is, we are not using comments to control code, we are adding special code that looks like comments. That's just semantics, but it should make them more "palatable".

hgtonight · October 2014

@businessdad said:
Also, I absolutely dislike the idea of drag and drop to configure priorities. Handler priority should never be controlled by a GUI, it's something that drives the code depending on specific needs. It's up to a plugin developer to know when an event should be fired, not to a site administrator, and such priority should be hard-coded, not configurable.

I was thinking more about sorting priority of plugin output to the page. I can't imagine a "proper" plugin that would depend on a specific priority of rendering. Even if there is the implicit need to be rendered last (for closing the current element and starting a new element), adding a priority just provides an arms race.

@businessdad said:
That is, we are not using comments to control code, we are adding special code that looks like comments. That's just semantics, but it should make them more "palatable".

Doesn't taste any better, imo.

Are there plans to add decorators to PHP?

businessdad · October 2014

@hgtonight said:
I was thinking more about sorting priority of plugin output to the page. I can't imagine a "proper" plugin that would depend on a specific priority of rendering. Even if there is the implicit need to be rendered last (for closing the current element and starting a new element), adding a priority just provides an arms race.

I think there's one missing point: I used Base_Render_Before as an example, but, in my case, the handlers that had to be fired before the others had nothing to do with rendering. In fact, rendering was not even in my list of possible uses for priorities, I was more thinking of altering event arguments, a bit like it's done with filters in Captain Hook (WordPress).

If your idea was to allow admins to alter the order of rendering, for example for modules, that's a completely different story, and has nothing to do with the idea behind handlers priority. A full widget system, like the one I designed in 2013 and then put aside indefinitely, would have all sorts of drag & drop stuff, and make Vanilla similar to (once again) WordPress.

Doesn't taste any better, imo.

Somehow I still think that you see them as "code in comments".

Are there plans to add decorators to PHP?

I don't think so. The idea was rejected in the past, although I don't remember why. You would not use them, anyway, so you should not be overly concerned about that.

businessdad · October 2014

@x00 said:
hgtonight that is a good idea, though semantically we are dealing with implicit hooks ideally you would have
object_method_handler(sender, args, priority=3)

By the way, I evaluated this idea, and I rejected it because the priority is not an argument to be passed to the handler, but to the handlers manager. If there is one place where I think the priority should not be, is precisely amongst the arguments.

The way I see it, a handler should know that is has been called, not when. "You have been called when you were supposed to be called" is all that it should know. Since these calls are implicit, the priority must be passed as one of the parameters during parsing, therefore an object_method_handler_priority syntax (e.g. Base_Render_Before_50) would conform to current logic, but look ugly and confusing.

R_J · October 2014

My first thought have been that using comments is a really bad idea because I thought there will be problems with APC and OpCache when you try to get comments but from the slide I've linked I've learned that OpCache ignores comments but caches docblocks. So they are availabe, no matter what you think about them. And I've also learned that since PHP 5.1 docblocks are accessable from reflections. So it seems to be "simply" a design question.

As long as this is something that is used by a third party library, I think it is great that is not very obstrusive and since I see docblocks as documentation and not as code, I think it is a nicce place to stre that information there.

But in what way is an annotation different from a trivial constant? I cannot see the slightest difference. "Oh no, don't store that information with a define() construct - use a docblock for that!" Nope. Their purpose is exactly the same, they just do it in different ways.

Using docblocks for that might look like a nice idea, but to my opinion that is only because docblocks are short and every information you put there, is easy to find and read. But the more control structures you will put in the docblock the harder it will be to read them.

Docblocks are for documentation and code is for coding. No need to mix that up.

If that plugin prio feature feature will ever be implemented, I'd like to have something similar to modules AssetTarget so that I can set it with a line of code in my function calls. Something simple like $this->SetPrio(50); would be nice and clean.

A sort with a GUI would be great. No plugin author can test his plugin with all plugins that are out there and you can never know what will be created in the future. Just think of two plugins that add HTML to the same place. Maybe some board admins like to have content A above content B and other board admins like to be content B on top of content A. That shouldn't require a coder. If you could achieve that with a simple drag'n'drop interface, that'll be awesome!

x00 · October 2014

The way I see it, a handler should know that is has been called, not when. "You have been called when you were supposed to be called" is all that it should know. Since these calls are implicit, the priority must be passed as one of the parameters during parsing, therefore an object_method_handler_priority syntax (e.g. Base_Render_Before_50) would conform to current logic, but look ugly and confusing.

Actually this was my first thought. I guess ugly is subjective. We get use to a particular code style. It can be odd to get used to something new.

I don't particularly like Ruby, but one convention in rails, implicit helper functions with natural language name e.g. distance_of_time_in_words

object_method_handler_priority_3(sender, args)

x00 · October 2014

http://php.net/manual/en/reflectionclass.getdoccomment.php

But will not work with bytecode caching.

R_J · October 2014

What a coincidence - here's a current article about annotations in an external xml file: http://webmozarts.com/2014/10/24/defining-php-annotations-in-xml/

x00 · October 2014

That is ugly as hell. Worse than native or docblocks

Really annotations are just a means to an end. Like hgtonight said, this isn't really what annotations are intended for

hgtonight · October 2014

If running new code on old manager is absolutely necessary, you could just have a helper handler to switch to the new handler. Assuming new handler is object_method_handler_priority you could also have:

public function object_method_handler($sender) {
  $this->object_method_handler_priority($sender);
}

I don't really see this as necessary since old code would still work with the new priority system.

Again, I don't really see this as a pressing issue. If your plugin specifically requires another addon to be run first, you could sort the priority to execute your plugin after the required addon in the event of a shared hook.

This would again reduce the priority to implicit ordering based on the Plugin Info array.

Alternatively, you could modify the first addon to fire an event on the shared event hook and just hook into that in the second addon.

What do you think about setting execution order based on the requirements tree (application, plugin, theme)?

Linc · October 2014

@hgtonight said:
This is a feature I think would be awesome to be in core.

I don't want to be the killjoy, but I figured I better say something before this turns into a pull request. We've purposefully eschewed this feature since the original design of the framework and I don't see that changing.

A clever and well-reasoned implementation, tho.

x00 · October 2014

Vanilla 1 issue was the order was being abused, it was an exercise in top trumps, but that was plugin level. I can't remember what happened hook level.