Remove negative priority on actions

[drrobotnik]

People have found that negative priority on actions can set themselves as the priority. It's clever, but should not be allowed as it makes priorities confusing, causes unintended consequences and can be abused. I consider this a bug as I don't think it was intended to be used this way or it would be mentioned in the docs. My suggestion is to either force a positive value, or check for negative index and change it to default value, 10.

[jdgrimes]

While negative values probably weren't intended when the system was designed, they are being used now, and any change there is going to break some plugins. For the sake of backward compatibility, it might be better to issue a _doing_it_wrong() notice for a few releases first, but not change anything until plugins have had time to adjust.

Priorities for filters were added in [853]. There is no ticket associated with that commit, so we've no way of knowing what Matt had in mind as far as negative priorities.

[drrobotnik]

I'm fine with adding a _doing_it_wrong() notice in order to prevent things from being broken and to give people time to work on another approach.

A quick regex search through WP add_action finds no instances of WP core using negative numbers. There is pretty solid evidence that the intention was to give devs a threshold to work with since the default priority is 10 and WP core usually uses 0-10. But, it's now being cleverly abused.

I'd prefer to modify the priority as it shouldn't "break" anything, they just won't get the priority they want, which is the point of the fix. The cases where I've seen this used is to get around perceived limitations. The problem with this is that they're asserting that they have a higher importance then everything else, including sometimes core WP priority.

How can we move forward with the doing_it_wrong() notice? Patch?

[mordauk]

I can't really see an advantage to changing this. Sure it's not 100% intuitive, but there are no problems caused by it, at least not that I'm aware of.

Even trigging _doing_it_wrong() notices is going to cause some pretty significant issues for a huge number of plugins.

[drrobotnik]

Replying to mordauk:

I can't really see an advantage to changing this. Sure it's not 100% intuitive, but there are no problems caused by it, at least not that I'm aware of.

The intuitiveness is not the issue. It's about establishing a proper queue order. Developers using this know they're using a hack.

Here is a purposely basic example:

add_action( 'wp_print_styles', array( $this, 'implode_frontend_css' ), -1 ); // Run first

... "Run first" until Dev B sees this and says don't implode my css:

add_action( 'wp_print_styles', 'my_css_is_important_dont_implode', -200 ); // Really run first

Even trigging _doing_it_wrong() notices is going to cause some pretty significant issues for a huge number of plugins.

I think thats a bit of an exaggeration. Significant because we add a notice? If there is a proper reason to load before core, then create/propose a proper method for doing so.

[dd32]

I'm not convinced this is a change we need to make, and more over, it's going to break anything already using them, and that backwards compatibility is core to WordPress.

The only benefit raised here, is that some people are not expecting to see a negative number, a problem that is solved as soon as they see a negative number there and understand that it can be used. Plugin Developers should be nice though, and not use insane priorities when it's not needed.

I'm suggesting wontfix here.

[drrobotnik]

The benefit raised here is preventing developers from setting priorities before core and enforcing that they queue properly. There is nothing preventing the developer from dequeuing the core action, running his method, and then re-queuing the core action after. I'd argue that that is the correct way to queue actions.

I've already said that it won't "break" anything, it will correctly adjust the priority fixing the hack. Show me a valid reason to set a negative index.

[boonebgorges]

+1 to wontfix. Changing this will break too many things.

Even if we didn't have to deal with backward compatibility, I'm not sure that disallowing negative priority is a great decision. If 0 (or 1) were the absolute lowest priority, it'd be impossible to use the hook to preempt 0-priority actions without resulting in race conditions. Having the entire number line at our disposal means that this is not an issue. A better idea is for us to promote polite conventions regarding the use of priorities.

[JustinSainton]

Another +1 on wontfix. When attempting to remove_action() on a known negative priority, the suggestions made in the original post would cause that to fail. For that back compat reason alone (aside from the fact that nothing in the Hooks API suggests that negative priorities are forbidden), I vote wontfix.

[nacin]

I've always enjoyed the hidden aspect of negative priorities. :-)

Not only can we not change this, there's no reason to change this.

[jorbin]

​Our ​documentation points out that the priority should be an integer. ​Integers are all numbers in the set {..., -2, -1, 0, 1, 2, ...}. Changing this to only accept positive integers would cause plugins to no longer function in the way that they intend to.

[drrobotnik]

Crushed.

@boonebgorges it's already a priority race.

@justinSainton not if you requeued the action at the same priority, only after your newly added super important function. Again it's a priority race.

[lev0]

I'm glad this was wontfix'd. A concrete example is in wp-includes/default-filters.php: add_action( 'shutdown', 'wp_ob_end_flush_all', 1 );
Without negative priority, how would one delay processing something as much as possible but still be included in the normal output?