Permalink
Browse files

fix(events): All hook/event handlers are now weighted properly

`getOrderedHandlers()` now orders handlers first by the priority and then
by the registration order. This also bumps the priority of “all” handlers
in attempt to emulate the calling order before this change.

Fixes #1378

BREAKING CHANGE:
To ensure your handler is called last, you must give it the highest priority
of all matching handlers. To ensure your handler is called first, you must
give it the lowest priority of all matching handlers. Registering with the
keyword “all” no longer has any effect on calling order.
  • Loading branch information...
mrclay committed Jun 2, 2015
1 parent 3c0f642 commit 3e6a28984bcbcb8e0698ca716fac924dc25fd10e
View
@@ -57,7 +57,7 @@ logged in during the [login, user] event?
Before Events have names ending in ":before" and are triggered before
something happens. Like traditional events, handlers can cancel the
-event by returning `false`.
+event by returning ``false``.
After Events, with names ending in ":after", are triggered after
something happens. Unlike traditional events, handlers *cannot* cancel
@@ -262,3 +262,13 @@ by giving the static version of the callback:
Even though the event handler references a dynamic method call, the code above will successfully
remove the handler.
+
+Handler Calling Order
+---------------------
+
+Handlers are called first in order of priority, then registration order.
+
+.. note::
+
+ Before Elgg 2.0, registering with the ``all`` keywords caused handlers to be called later, even
+ if they were registered with lower priorities.
@@ -4,7 +4,7 @@
* type.
*/
-elgg_register_plugin_hook_handler('all', 'system', 'example_plugin_hook_handler');
+elgg_register_plugin_hook_handler('all', 'system', 'example_plugin_hook_handler', 600);
// This function will be called for any hook of type 'system'
function example_plugin_hook_handler($hook, $type, $value, $params) {
View
@@ -156,6 +156,19 @@ Plugins should use the class ``Elgg\Application`` to boot Elgg. Typical usage:
require_once dirname(dirname(__DIR__)) . '/autoloader.php';
(new \Elgg\Application)->bootCore();
+Event/Hook calling order may change
+-----------------------------------
+
+When registering for events/hooks, the ``all`` keyword for wildcard matching no longer has any effect
+on the order that handlers are called. To ensure your handler is called last, you must give it the
+highest priority of all matching handlers, or to ensure your handler is called first, you must give
+it the lowest priority of all matching handlers.
+
+If handlers were registered with the same priority, these are called in the order they were registered.
+
+To emulate prior behavior, Elgg core handlers registered with the ``all`` keyword have been raised in
+priority. Some of these handlers will most likely be called in a different order.
+
``export/`` URLs are no longer available
----------------------------------------
@@ -115,7 +115,6 @@ public function getViews($viewtype = 'default') {
// view handlers
$handlers = _elgg_services()->hooks->getAllHandlers();
-
$filtered_views = array();
if (!empty($handlers['view'])) {
$filtered_views = array_keys($handlers['view']);
@@ -359,12 +358,15 @@ protected function buildHandlerTree($all_handlers) {
$root = elgg_get_root_path();
foreach ($all_handlers as $hook => $types) {
- foreach ($types as $type => $handlers) {
- array_walk($handlers, function (&$callable, $priority) use ($root) {
- $description = $this->describeCallable($callable, $root);
- $callable = "$priority: $description";
- });
- $tree[$hook . ',' . $type] = $handlers;
+ foreach ($types as $type => $priorities) {
+ foreach ($priorities as $priority => $handlers) {
+
+ array_walk($handlers, function (&$callable) use ($root, $priority) {
+ $description = $this->describeCallable($callable, $root);
+ $callable = "$priority: $description";
+ });
+ $tree[$hook . ',' . $type] = $handlers;
+ }
}
}
@@ -14,7 +14,19 @@
*/
abstract class HooksRegistrationService {
- private $handlers = array();
+ const REG_KEY_PRIORITY = 0;
+ const REG_KEY_INDEX = 1;
+ const REG_KEY_HANDLER = 2;
+
+ /**
+ * @var int
+ */
+ private $next_index = 0;
+
+ /**
+ * @var array [name][type][] = registration
+ */
+ private $registrations = [];
/**
* @var \Elgg\Logger
@@ -44,24 +56,13 @@ public function registerHandler($name, $type, $callback, $priority = 500) {
if (empty($name) || empty($type) || !is_callable($callback, true)) {
return false;
}
-
- if (!isset($this->handlers[$name])) {
- $this->handlers[$name] = array();
- }
-
- if (!isset($this->handlers[$name][$type])) {
- $this->handlers[$name][$type] = array();
- }
-
- // Priority cannot be lower than 0
- $priority = max((int) $priority, 0);
-
- while (isset($this->handlers[$name][$type][$priority])) {
- $priority++;
- }
-
- $this->handlers[$name][$type][$priority] = $callback;
- ksort($this->handlers[$name][$type]);
+
+ $this->registrations[$name][$type][] = [
+ self::REG_KEY_PRIORITY => $priority,
+ self::REG_KEY_INDEX => $this->next_index,
+ self::REG_KEY_HANDLER => $callback,
+ ];
+ $this->next_index++;
return true;
}
@@ -77,23 +78,26 @@ public function registerHandler($name, $type, $callback, $priority = 500) {
* @access private
*/
public function unregisterHandler($name, $type, $callback) {
- if (isset($this->handlers[$name]) && isset($this->handlers[$name][$type])) {
- $matcher = $this->getMatcher($callback);
-
- foreach ($this->handlers[$name][$type] as $key => $handler) {
- if ($matcher) {
- if (!$matcher->matches($handler)) {
- continue;
- }
- } else {
- if ($handler != $callback) {
- continue;
- }
- }
+ if (empty($this->registrations[$name][$type])) {
+ return false;
+ }
+
+ $matcher = $this->getMatcher($callback);
+
+ foreach ($this->registrations[$name][$type] as $i => $registration) {
- unset($this->handlers[$name][$type][$key]);
- return true;
+ if ($matcher) {
+ if (!$matcher->matches($registration[self::REG_KEY_HANDLER])) {
+ continue;
+ }
+ } else {
+ if ($registration[self::REG_KEY_HANDLER] != $callback) {
+ continue;
+ }
}
+
+ unset($this->registrations[$name][$type][$i]);
+ return true;
}
return false;
@@ -103,15 +107,29 @@ public function unregisterHandler($name, $type, $callback) {
* Returns all registered handlers as array(
* $name => array(
* $type => array(
- * $priority => callback, ...
+ * $priority => array(
+ * callback,
+ * callback,
+ * )
* )
* )
*
* @access private
* @return array
*/
public function getAllHandlers() {
- return $this->handlers;
+ $ret = [];
+ foreach ($this->registrations as $name => $types) {
+ foreach ($types as $type => $registrations) {
+ foreach ($registrations as $registration) {
+ $priority = $registration[self::REG_KEY_PRIORITY];
+ $handler = $registration[self::REG_KEY_HANDLER];
+ $ret[$name][$type][$priority][] = $handler;
+ }
+ }
+ }
+
+ return $ret;
}
/**
@@ -122,7 +140,7 @@ public function getAllHandlers() {
* @return boolean
*/
public function hasHandler($name, $type) {
- return isset($this->handlers[$name][$type]);
+ return !empty($this->registrations[$name][$type]);
}
/**
@@ -136,25 +154,42 @@ public function hasHandler($name, $type) {
* @access private
*/
public function getOrderedHandlers($name, $type) {
- $handlers = array();
+ $registrations = [];
- if (isset($this->handlers[$name][$type])) {
- if ($name != 'all' && $type != 'all') {
- $handlers = array_merge($handlers, array_values($this->handlers[$name][$type]));
+ if (!empty($this->registrations[$name][$type])) {
+ if ($name !== 'all' && $type !== 'all') {
+ array_splice($registrations, count($registrations), 0, $this->registrations[$name][$type]);
}
}
- if (isset($this->handlers['all'][$type])) {
- if ($type != 'all') {
- $handlers = array_merge($handlers, array_values($this->handlers['all'][$type]));
+ if (!empty($this->registrations['all'][$type])) {
+ if ($type !== 'all') {
+ array_splice($registrations, count($registrations), 0, $this->registrations['all'][$type]);
}
}
- if (isset($this->handlers[$name]['all'])) {
- if ($name != 'all') {
- $handlers = array_merge($handlers, array_values($this->handlers[$name]['all']));
+ if (!empty($this->registrations[$name]['all'])) {
+ if ($name !== 'all') {
+ array_splice($registrations, count($registrations), 0, $this->registrations[$name]['all']);
}
}
- if (isset($this->handlers['all']['all'])) {
- $handlers = array_merge($handlers, array_values($this->handlers['all']['all']));
+ if (!empty($this->registrations['all']['all'])) {
+ array_splice($registrations, count($registrations), 0, $this->registrations['all']['all']);
+ }
+
+ usort($registrations, function ($a, $b) {
+ // priority first
+ if ($a[self::REG_KEY_PRIORITY] < $b[self::REG_KEY_PRIORITY]) {
+ return -1;
+ }
+ if ($a[self::REG_KEY_PRIORITY] > $b[self::REG_KEY_PRIORITY]) {
+ return 1;
+ }
+ // then insertion order
+ return ($a[self::REG_KEY_INDEX] < $b[self::REG_KEY_INDEX]) ? -1 : 1;
+ });
+
+ $handlers = [];
+ foreach ($registrations as $registration) {
+ $handlers[] = $registration[self::REG_KEY_HANDLER];
}
return $handlers;
View
@@ -606,8 +606,8 @@ function access_test($hook, $type, $value, $params) {
$events->registerHandler('ready', 'system', 'access_init');
// For overrided permissions
- $hooks->registerHandler('permissions_check', 'all', 'elgg_override_permissions');
- $hooks->registerHandler('container_permissions_check', 'all', 'elgg_override_permissions');
+ $hooks->registerHandler('permissions_check', 'all', 'elgg_override_permissions', 600);
+ $hooks->registerHandler('container_permissions_check', 'all', 'elgg_override_permissions', 600);
$hooks->registerHandler('unit_test', 'system', 'access_test');
};
View
@@ -302,8 +302,8 @@ function actions_init() {
elgg_register_simplecache_view('js/languages/en');
- elgg_register_plugin_hook_handler('action', 'all', 'ajax_action_hook');
- elgg_register_plugin_hook_handler('forward', 'all', 'ajax_forward_hook');
+ elgg_register_plugin_hook_handler('action', 'all', 'ajax_action_hook', 600);
+ elgg_register_plugin_hook_handler('forward', 'all', 'ajax_forward_hook', 600);
}
return function(\Elgg\EventsService $events, \Elgg\HooksRegistrationService $hooks) {
View
@@ -202,8 +202,8 @@ function _elgg_admin_init() {
// maintenance mode
if (elgg_get_config('elgg_maintenance_mode', null)) {
- elgg_register_plugin_hook_handler('route', 'all', '_elgg_admin_maintenance_handler');
- elgg_register_plugin_hook_handler('action', 'all', '_elgg_admin_maintenance_action_check');
+ elgg_register_plugin_hook_handler('route', 'all', '_elgg_admin_maintenance_handler', 600);
+ elgg_register_plugin_hook_handler('action', 'all', '_elgg_admin_maintenance_action_check', 600);
elgg_register_css('maintenance', elgg_get_simplecache_url('css', 'maintenance'));
elgg_register_menu_item('topbar', array(
@@ -71,5 +71,5 @@ function elgg_register_class($class, $location) {
}
return function(\Elgg\EventsService $events, \Elgg\HooksRegistrationService $hooks) {
- $events->registerHandler('upgrade', 'all', '_elgg_delete_autoload_cache');
+ $events->registerHandler('upgrade', 'all', '_elgg_delete_autoload_cache', 600);
};
View
@@ -21,7 +21,7 @@ function _elgg_comments_init() {
elgg_register_plugin_hook_handler('permissions_check', 'object', '_elgg_comments_permissions_override');
elgg_register_plugin_hook_handler('email', 'system', '_elgg_comments_notification_email_subject');
- elgg_register_event_handler('update:after', 'all', '_elgg_comments_access_sync');
+ elgg_register_event_handler('update:after', 'all', '_elgg_comments_access_sync', 600);
elgg_register_page_handler('comment', '_elgg_comments_page_handler');
View
@@ -406,7 +406,7 @@ function _elgg_metadata_test($hook, $type, $value, $params) {
return function(\Elgg\EventsService $events, \Elgg\HooksRegistrationService $hooks) {
/** Call a function whenever an entity is updated **/
- $events->registerHandler('update', 'all', 'metadata_update');
+ $events->registerHandler('update', 'all', 'metadata_update', 600);
$hooks->registerHandler('unit_test', 'system', '_elgg_metadata_test');
};
@@ -286,7 +286,7 @@ function _elgg_notifications_smtp_thread_headers($hook, $type, $returnvalue, $pa
*/
function _elgg_notifications_init() {
elgg_register_plugin_hook_handler('cron', 'minute', '_elgg_notifications_cron', 100);
- elgg_register_event_handler('all', 'all', '_elgg_enqueue_notification_event');
+ elgg_register_event_handler('all', 'all', '_elgg_enqueue_notification_event', 700);
// add email notifications
elgg_register_notification_method('email');
View
@@ -837,6 +837,6 @@ function _elgg_river_init() {
return function(\Elgg\EventsService $events, \Elgg\HooksRegistrationService $hooks) {
$events->registerHandler('init', 'system', '_elgg_river_init');
- $events->registerHandler('disable:after', 'all', '_elgg_river_disable');
- $events->registerHandler('enable:after', 'all', '_elgg_river_enable');
+ $events->registerHandler('disable:after', 'all', '_elgg_river_disable', 600);
+ $events->registerHandler('enable:after', 'all', '_elgg_river_enable', 600);
};
@@ -176,7 +176,7 @@ function test_acl_access_hook($hook, $type, $value, $params) {
return $value;
}
- elgg_register_plugin_hook_handler('access:collections:write', 'all', 'test_acl_access_hook');
+ elgg_register_plugin_hook_handler('access:collections:write', 'all', 'test_acl_access_hook', 600);
// enable security since we usually run as admin
$ia = elgg_set_ignore_access(false);
@@ -131,7 +131,7 @@ function can_write_to_container_test_hook($hook, $type, $value, $params) {
}
}
- elgg_register_plugin_hook_handler('container_permissions_check', 'all', 'can_write_to_container_test_hook');
+ elgg_register_plugin_hook_handler('container_permissions_check', 'all', 'can_write_to_container_test_hook', 600);
$this->assertTrue(can_write_to_container($user->guid, $object->guid));
elgg_unregister_plugin_hook_handler('container_permissions_check', 'all', 'can_write_to_container_test_hook');
Oops, something went wrong.

0 comments on commit 3e6a289

Please sign in to comment.