Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ordering to event handlers #552

Merged
merged 1 commit into from Feb 16, 2014
Merged

Conversation

Chris--S
Copy link
Collaborator

Allows a sequence integer to be supplied when registering to handle
an event. When processing an event, handlers (hooks) will be
executed in ascending order of their sequence number. If two or
more handlers have the same sequence number their order of
execution is undefined.

A handler wanting to be first, should use -PHP_INT_MAX.
A handler wanting to be last can use PHP_INT_MAX.
There may be conflicts if two or more plugins use these values in
the expectation that they will guarantee being first or last.

Allows a sequence integer to be supplied when registering to handle
an event.  When processing an event, handlers (hooks) will be
executed in ascending order of their sequence number.  If two or
more handlers have the same sequence number their order of
execution is undefined.

A handler wanting to be first, should use -PHP_MAX_INT.
A handler wanting to be last can use PHP_MAX_INT.
There may be conflicts if two or more plugins use these values in
the expectation that they will guarantee being first or last.
splitbrain added a commit that referenced this pull request Feb 16, 2014
@splitbrain splitbrain merged commit db7110f into master Feb 16, 2014
@michitux
Copy link
Collaborator

Some questions regarding this:

  • Is it safe in PHP to sort an array that is currently processed in an foreach loop? I think foreach operates on a copy but I'm not 100% sure.
  • Couldn't we rather use a key in the form $seq.'-'.count($this->_hooks[$event.'_'.$advise]) and then sort by key instead of sorting using a custom function (I imagine this should be faster)
  • I wonder if it would make more sense to sort the handlers when they are added. For certain events like ACTION_ACT_PREPROCESS this decreases the performance but for other events like AUTH_ACL_CHECK the event might be called hundreds of times in a row (imagine the sitemap) and in this case the array shouldn't be sorted again. Maybe we could also add a flag if the array has already been sorted after the last new event handler and sort it only if the flag hasn't been set.

@Chris--S
Copy link
Collaborator Author

  1. Yup, foreach can operate an on an array.
  2. I did think of that, but I think the code is clearer this way. I don't think there will be many handlers of individual events. So I don't think performance will be an issue.
  3. Good point. Will amend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants