Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#22250 closed enhancement (wontfix)

Useful helper function, add_action_with_args()

Reported by: rzen Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: close
Focuses: Cc:

Description

About a year ago, Rarst released a plugin named Advanced Hooks API (http://wordpress.org/extend/plugins/advanced-hooks-api/) that adds a number of (experimental) wrappers that allow one to hook more elaborate events without coding intermediary functions. One of these wrappers in particular (add_action_with_args) would save theme and plugin devs an annoying amount of work writing dummy functions to simply hook a callback that requires a few specific params.

I pulled just that functionality out of his plugin, distilled it to it's simplest form, and added it in to plugin.php. I think this functionality has legs, and a number of others agree, but some are not convinced it has a home in core. I would LOVE to see it there, as I said it would eliminate a ton of unnecessary work, but I present it to you who are smarter than me to talk through why it should/shouldn't be.

Here are some sample use-cases: https://gist.github.com/3928941

Attachments (1)

22250.diff (1.6 KB) - added by rzen 9 years ago.

Download all attachments as: .zip

Change History (21)

@rzen
9 years ago

#1 @mordauk
9 years ago

  • Cc pippin@… added

I could definitely get behind this.

#2 @DrewAPicture
9 years ago

  • Cc xoodrew@… added

#3 @DrewAPicture
9 years ago

This sounds very close to #22218 which was wontfixed. The existing plugin is a good proof of concept, though I'm not sure if core would find extra benefit in it.

#4 @rzen
9 years ago

  • Cc brian@… added

Similar, but different because it leaves add_action alone and lean. Admittedly, the separate helper class is a bit cumbersome, but this is just to get the discussion going.

Edit: ha! Didn't realize the wontfix ticket was only from four days ago, nor that scribu suggested essentially the same code demoed here (courtesy Rarst). Classic!

Last edited 9 years ago by rzen (previous) (diff)

#5 follow-up: @DrewAPicture
9 years ago

Feel free to correct me on any of this.

Isn't it possible that using this helper in core could actually make it more difficult to extend certain functionality? I mean, I'm no great shakes of a developer but it seems like most of the time functions are abstracted so we have the ability to manipulate returned values in callbacks, such as through filters, separate from the actions.

The way I read this is that add_action_with_args() would essentially allow rolling a callback + args into add_action, without the added abstraction layer. I can see how it might 'save time', but I can also see how it might create an an extensibility barrier.

#6 follow-up: @TJNowell
9 years ago

It should be kept in mind that this functionality isn't the only method of doing things, e.g. when designing a car, adding more wheels and mechanisms to go over a hill might work, but so does getting out the car and walking..

My main issue with this is if somebody uses it, and does something like this:

function __echo_text( $input ) { echo $input; }

add_action( 'wp_footer', '__echo_text', 10, 'Some super important text that the client hates to such a degree that they want to eat your babies and wear your intestines.' );

Requiring forks of said codebase, and additional maintenance to support changes ( perhaps the plugin or framework developer has decided it is not an issue, or feels that the text in question is necessary for functionality to work, perhaps you're spanish and that super important text is english and there's no translation support ).

Ofc one could add a filter on this, but if so one shouldn't trust the developer at the other end to do this, and it still leaves the issue of what the filter should be called etc

#7 @SergeyBiryukov
9 years ago

#22218 was marked as a duplicate.

#8 in reply to: ↑ 6 @rzen
9 years ago

Replying to TJNowell:

My main issue with this is if somebody uses it, and does something like this ... perhaps you're spanish and that super important text is english and there's no translation support

I would argue that's an issue with anything an inexperienced dev uses, not specific to this function. They can pass crummy code and non-localized strings using add_action and add_filter already. However, you presented an excellent piece I overlooked: a simple remove_action equivalent for this function.

What are some other opinions?

#9 in reply to: ↑ 5 ; follow-up: @rzen
9 years ago

Replying to DrewAPicture:

I can see how it might 'save time', but I can also see how it might create an an extensibility barrier.

What sort of extensibility barriers do you foresee?

#10 in reply to: ↑ 9 @DrewAPicture
9 years ago

  • Keywords dev-feedback added

Replying to rzen:

Replying to DrewAPicture:

I can see how it might 'save time', but I can also see how it might create an an extensibility barrier.

What sort of extensibility barriers do you foresee?

I already outlined one of them. I view this as skipping the ability-to-filter step, a valid purpose a lot of the "dummy" functions serve.

My understanding of the benefits of something like this is limited, however. I'm curious to see what some of the more experienced folks have to say.

#11 @scribu
9 years ago

  • Keywords close added; 2nd-opinion dev-feedback removed

I think we should wait for PHP 5.3 to become more widespread:

add_action( 'wp_footer', function() {
  get_sidebar( 'footer' );
} );

#12 @scribu
9 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Yes, anonymous functions are also hard to unhook, but the syntax is a lot clearer; with add_action_with_args() you just have a mess of parameters.

Either way, it just doesn't make sense to add a helper that would become obsolete after a few releases.

#13 @scribu
9 years ago

Oh, and if you find yourself doing something like this:

foreach ( get_option( 'footer_sidebars' ) as $sidebar_id ) {
  add_action( 'wp_footer', function() use ( $sidebar_id ) {
    get_sidebar( $sidebar_id );
  } );
}

you should consider refactoring your code so that it looks more like this:

add_action( 'wp_footer', function() {
  foreach ( get_option( 'footer_sidebars' ) as $sidebar_id ) {
    get_sidebar( $sidebar_id );
  }
} );

Callbacks are not free.

Last edited 9 years ago by scribu (previous) (diff)

#14 @nacin
9 years ago

Perhaps come a requirements shift to 5.3, we come up with a way to more easily remove closures. Namespacing, anyone?

#15 @scribu
9 years ago

A la jQuery.on('click.mynamespace')? Sounds fun.

#16 @rzen
9 years ago

Good point there, scribu. I've been leery to use anon functions because I'm not positive what version of PHP users are running. Then again, I'd be leery to use add_action_with_args() for the fact that users then need to determine "do I use remove_action or remove_action_with_args" (or whatever other solution it would have needed).

Thanks for the feedback, everyone!

#17 @scribu
9 years ago

Actually, namespaces would be useful right now: #22256

#18 @Rarst
9 years ago

For the record I have worked out removal in Advanced Hooks API ( http://plugins.trac.wordpress.org/browser/advanced-hooks-api/trunk/class-r-hook-handler.php#L152 ), but it is still not something I see added to core.

For me it's primarily test bed for functionality and syntax of different extensions while disregarding robustness and performance of technical implementation. Although I do have limited use of it in production and hadn't seen serious issues from it so far.

#19 @Rarst
9 years ago

  • Cc contact@… added

This ticket was mentioned in Slack in #core by liamhogan. View the logs.


7 years ago

Note: See TracTickets for help on using tickets.