WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 20 months ago

Last modified 4 months ago

#14280 closed enhancement (wontfix)

add_filter should accept an array of tags

Reported by: sivel Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Plugins Keywords: has-patch early needs-testing dev-feedback
Focuses: Cc:

Description

Currently when you want to hook a single callback into multiple filters/actions you create an array and loop through it calling add_action/filter each time. It would reduce code and make it easier to do this by just having add_filter accept an array of strings as it's first argument. In addition tests have shown that it also improves overall performance of WP by using this.

I talked to nacin on IRC recently about this and he was concerned about negative performance impacts that this may have, since most of our add_action/filter calls are for single tags. I did some performance benchmarking and found the following results:

With having add_filter accept an array it was 0.000002002716 seconds slower for single actions, but 0.000007534 seconds faster for a 4 action loop. Without add_filter accepting an array each iteration of a foreach in the way we are currently handling it takes 0.00000885725 seconds, accepting an array each iteration takes 0.00000697374 seconds. Which is a 0.000001884 improvement during add_action/filter iterations.

Overall on a default install of WP 3.1-alpha this modification actually improved performance by 0.00048398971558 seconds faster by having add_filter handle all of the looping.

Anyway, this is an early patch and likely could use a bit of testing and independent benchmarking.

At the moment I have not added this functionality to remove_action/filter and am not really sure we need it.

Please provide feedback, test and provide your own benchmarks.

Attachments (1)

14280.diff (10.5 KB) - added by sivel 4 years ago.

Download all attachments as: .zip

Change History (13)

sivel4 years ago

comment:1 sivel4 years ago

  • Keywords has-patch early needs-testing dev-feedback added

comment:2 scribu4 years ago

Something similar has been proposed before: #8640

comment:3 sivel3 years ago

  • Milestone changed from 3.1 to Future Release

comment:4 scribu20 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Marked #21591 as dup.

I don't see this:

add_action( array( 'sanitize_post', 'sanitize_user' ), 'my_sanitizer' );

as a significant enough improvement over this:

foreach ( array( 'sanitize_post', 'sanitize_user' ) as $hook ) {
  add_action( $hook, 'my_sanitizer' );
}

It's the kind of syntactic sugar that makes you fat: tasty, but low in nutritional value.

comment:5 joehoyle20 months ago

If you are using closures, this isn't just syntactic sugar.

foreach ( array( 'sanitize_post', 'sanitize_user' ) as $hook ) {
  add_action( $hook, function() { return $foo; } );
}

This will create 2 closure objects, whereas add_action supporting an array would only require 2 references to the same closure. (less memory usage, shared scope etc)

You could always do

$func = function() { return $foo };

foreach ( array( 'sanitize_post', 'sanitize_user' ) as $hook ) {
  add_action( $hook, $func );
}

But that feels like it starts to get messy. I would rather create my own add_actions function than do the above, so I don't think it's a stretch to just add the functionality to add_action

comment:6 scribu20 months ago

Then create your own add_actions() helper; nothing wrong with that. Just don't force it on everyone else that might not need it.

comment:7 joehoyle20 months ago

It would be a silent addition to add_action, of course you wouldn't have to pass an array. But more importantly I am not trying to make anyone, of course I do create my own functions. I don't submit ideas to trac to force other people to code the way I do, but rather if I find it useful then other people may do too, and it may add value for WordPress core. sheesh.

comment:8 follow-up: nacin20 months ago

PHP is not JavaScript, but even in JS, you assign these things to variables when it makes sense to do so. Closures are nice for one-off things. This is no longer so simple once you require adding it to multiple hooks. Why not a top-level function?

Also of note, if you do use a closure but don't assign it to an accessible variable, it becomes impossible to remove that action later without calling remove_all_actions(). That is lame, and makes closures little suited beyond testing and internal implementations. For distributed work, they are best avoided.

I don't have a specific objection to this, and PHP 5.4 [ ] syntax makes it somewhat elegant. But it does complicate an API that is, at the moment, quite simple.

comment:9 scribu20 months ago

It would be a silent addition to add_action, of course you wouldn't have to pass an array.

That's not what I meant.

I don't submit ideas to trac to force other people to code the way I do, but rather if I find it useful then other people may do too, and it may add value for WordPress core. sheesh.

I am not accusing you of anything. Just saying that it's not actually that useful, so it should remain a utility that devs can implement in a few seconds, as needed.

comment:10 in reply to: ↑ 8 ; follow-up: joehoyle20 months ago

Replying to nacin:

Yeah, totally agree they have certain applications, though I personally think they are slightly more useful that you say above. IMO the best time to use them is something like:

add_action( 'init', function() {
    register_post_type( ... );
} );

So many plugins do the above with add_action( 'init', 'my_plugin_init' ); that doesn't make sense to be a function. It's not reusable, it's not meant to be added / remove by third parties, so probably shouldn't be added as a top level function. Without closures you end up loads of once-use weirdly named internal functions for callbacks, making it much more difficult to have the codebase coherent.

For distributed code, totally get the remove_action problem, but 95% of the code I write is for internal implementations.

comment:11 in reply to: ↑ 10 nacin6 months ago

Replying to joehoyle:

So many plugins do the above with add_action( 'init', 'my_plugin_init' ); that doesn't make sense to be a function. It's not reusable, it's not meant to be added / remove by third parties, so probably shouldn't be added as a top level function. Without closures you end up loads of once-use weirdly named internal functions for callbacks, making it much more difficult to have the codebase coherent.

So, I certainly see where this sentiment is coming from, and am inclined to agree. But I'm not sure it scales well beyond a single hook. Once you need to attach the same callback/closure to multiple hooks, I think that's generally a really good indicator that we've crossed (or at least approached) the point of reusability.

comment:12 dd324 months ago

#26704 was marked as a duplicate.

Note: See TracTickets for help on using tickets.