register_plugin_hook priority isn't applied globally (Trac #1378) #1378
Comments
|
cash wrote on 39886573-03-29 I don't see anyway to do this differently. Right now, order of hook execution is predictable exactly as described above. Any effort to combine the 4 specificities into a single array of hooks on execution will
If you want to guarantee that your hook runs last: register for all/all with the lowest priority and filter on hook and type so it only runs for the specific hook you are interested in. Any attempt to rewrite the code to make it easier to register for the last hook will likely make it more difficult to do other things or make things unnecessarily complicated. I'll leave this open for Brett to look at. |
|
brettp wrote on 39966409-09-21 Because the order of execution (however odd) can be absolutely determined, I'm holding this one until 2.0. At that time we can re-evaluate the event priority system for better order of execution. |
|
Milestone changed to |
|
Milestone changed to |
|
If the problem is losing original registration order, why don't we have a global counter so each registration has a unique number to resolve disputes? |
|
I seem to run into this every few months... registering for 'all/all' to ensure your hook runs last is all well and good, but now I need my hook to run first. I can hook into 'route', 'all' - but then I can't ensure this hook runs first as any more specific hook runs first regardless of the priority I set. I can parse all of my necessary page handlers first, and register the same hook for them individually in a more specific manner, but that seems messier to me. My opinion is that priority should be honored. |
|
I suggest handlers be stored under the key On trigger, you merge all the hooks/types you want into a new temp var, sort by keys (priority) then walk through each priority, then sort the subarray by keys (registration order) before walking through it. In effect all handlers are on an even playing field, and registration order solves disputes over priorities. |
|
Why was this removed from 2.0.0? Ran into this again... I have a hook on 'route,all' that needs to trigger first and does some security checks and only allows access to specific pagehandlers. In this case it needs to know what is allowed, and disallow everything else. So it can't be registered on specific handlers because we don't know what those are. What I'm getting at is it's really unintuitive despite the fact that it's technically predictable, and it's problematic. It can't be fixed in 1.x but in 2.0 I want priority to actually be the priority. |
|
Maybe we can just add a before and after events here? You could register for the before event and cancel it if the user doesn't have permission. |
|
As for why removed from 2.0: everything was removed from 2.0 at one point. You are welcome to add it back if you feel it should be a blocking issue. |
`getOrderedHandlers()` now orders handlers first by the priority and then by the registration order. Fixes Elgg#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.
|
Better late than never! #8410 |
`getOrderedHandlers()` now orders handlers first by the priority and then by the registration order. Fixes Elgg#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.
`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 Elgg#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.
`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 Elgg#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.
Original ticket http://trac.elgg.org/ticket/1378 on 39884583-04-02 by trac user twall, assigned to unknown.
Elgg version: 1.6
The priority passed in with register_plugin_hook only affects the hook's priority in relation to other hooks registered with the same hook/object type.
To ensure a hook is run first, you must both specify a priority of '0' AND specify both the hook and entity type. To ensure a hook is run last, you must specify a priority of 1000 and use 'all'/'all' as the hook/object type. Using hook names and entity types other than what is appropriate, simply to affect the priority, is what makes me consider this a bug.
The following sets of hooks are executed in order:
'hook'/'type'
'all'/'type'
'hook'/'all'
'all'/'all'
It may not be possible to merge these sets after registration, since the original ordering information is no longer available. However, executing as they do results in the wrong results for priority.
Specificly, my 'check_permissions'/'object' hook will always be overridden by the 'check_permissions'/'all' hook set up by someone else, regardless of priority.
The text was updated successfully, but these errors were encountered: