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

register_plugin_hook priority isn't applied globally (Trac #1378) #1378

Closed
elgg-gitbot opened this Issue Feb 16, 2013 · 11 comments

Comments

Projects
None yet
4 participants
@elgg-gitbot

elgg-gitbot commented Feb 16, 2013

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.

@elgg-gitbot

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

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

  1. make things more complicated
  2. likely make it harder to predict when a plugin hook will run

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.

elgg-gitbot commented Feb 16, 2013

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

  1. make things more complicated
  2. likely make it harder to predict when a plugin hook will run

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.

@elgg-gitbot

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

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.

elgg-gitbot commented Feb 16, 2013

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.

@elgg-gitbot

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

Milestone changed to Elgg 2.0 by brettp on 39966409-09-21

elgg-gitbot commented Feb 16, 2013

Milestone changed to Elgg 2.0 by brettp on 39966409-09-21

@elgg-gitbot

This comment has been minimized.

Show comment
Hide comment
@elgg-gitbot

elgg-gitbot Feb 16, 2013

Milestone changed to Future Release by cash on 41784027-04-12

elgg-gitbot commented Feb 16, 2013

Milestone changed to Future Release by cash on 41784027-04-12

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 24, 2013

Member

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?

Member

mrclay commented May 24, 2013

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?

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Aug 21, 2013

Member

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.
Here's the situation - I need to hook into 'route', 'something' - where the something is unknown and is determined by plugin settings.

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.

Member

beck24 commented Aug 21, 2013

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.
Here's the situation - I need to hook into 'route', 'something' - where the something is unknown and is determined by plugin settings.

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.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Aug 21, 2013

Member

I suggest handlers be stored under the key $this->handlers[$hook][$type][$priority][$i] where $i is a global counter increasing with each registration.

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.

Member

mrclay commented Aug 21, 2013

I suggest handlers be stored under the key $this->handlers[$hook][$type][$priority][$i] where $i is a global counter increasing with each registration.

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.

@ewinslow ewinslow modified the milestones: Elgg 2.0.0, Long Term Future Release Jul 6, 2014

@ewinslow ewinslow added the major label Jul 6, 2014

@ewinslow ewinslow removed this from the Elgg 2.0.0 milestone Jul 10, 2014

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Nov 15, 2014

Member

Why was this removed from 2.0.0?

Ran into this again...
I would really like to be able to register a handler for a priority and know that it's going to trigger when I expect it to. Just got hit with this again. Situation:

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.
Then we have plugins that use the route hook to override specific pages - in this case group_tools has a handler on 'route, group'. So the group_tools handler will trigger before my security handler regardless of priorities, and it will output its page before my code can even execute.

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.

Member

beck24 commented Nov 15, 2014

Why was this removed from 2.0.0?

Ran into this again...
I would really like to be able to register a handler for a priority and know that it's going to trigger when I expect it to. Just got hit with this again. Situation:

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.
Then we have plugins that use the route hook to override specific pages - in this case group_tools has a handler on 'route, group'. So the group_tools handler will trigger before my security handler regardless of priorities, and it will output its page before my code can even execute.

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.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Nov 15, 2014

Member

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.

Member

ewinslow commented Nov 15, 2014

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.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Nov 15, 2014

Member

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.

Member

ewinslow commented Nov 15, 2014

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.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jun 3, 2015

fix(events): All hook/event handlers are now weighted properly
`getOrderedHandlers()` now orders handlers first by the priority and then
by the registration order.

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.
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jun 3, 2015

Member

Better late than never! #8410

Member

mrclay commented Jun 3, 2015

Better late than never! #8410

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jun 3, 2015

fix(events): All hook/event handlers are now weighted properly
`getOrderedHandlers()` now orders handlers first by the priority and then
by the registration order.

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.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jun 4, 2015

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.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jun 9, 2015

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.

@mrclay mrclay closed this in #8410 Jun 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment