WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 7 weeks ago

Last modified 12 days ago

#14994 closed enhancement (fixed)

Introduce a way to identify a hook in progress

Reported by: nacin Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Plugins Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

We have did_action() and current_filter() but I've come across a use case for a hybrid of sorts, doing_action().

Problem is, did_action() returns true the moment the hook is fired. Thus if you need to wait until after the hook is done executing, you need to also check current_filter(). The problem arises when there was another hook called since thne, because current_filter() does not traverse back up the stack.

I considered adding an argument to either of the two other functions mentioned, but I think a new function makes the most sense. did_action() only works for actions. While current_filter() works for all hooks, it does one thing and that is to return the current hook. A new function makes the most sense here.

doing_action() might not be the best name because did_action() only works for actions, but this would work for filters as well. At that point, I might recommend doing_hook().

function doing_action( $action ) {
   global $wp_current_filter;
   return in_array( $action, $'wp_current_filter );
}

The use case was that a plugin was applying the_content on wp_head. That was messing with my footnotes plugin. So I needed to make sure I had completely processed wp_head first, but there was no way to do that. Now I would be able to check if ( did_action('wp_head') && ! doing_action('wp_head') ).

Attachments (4)

14994.diff (1.6 KB) - added by Nacin 3 years ago.
Adds current_action() as a wrapper for current_filter(). Adds doing_action() and doing_filter(). Ignores the call for a counterpart to did_action() for filters (should be a new ticket).
14994.2.diff (1.8 KB) - added by nacin 3 years ago.
14994.3.diff (3.7 KB) - added by ericmann 3 months ago.
Refresh patch and add unit tests
14994.4.diff (3.8 KB) - added by ericmann 3 months ago.
Use current_filter()/current_action() rather than a call to a global variable

Download all attachments as: .zip

Change History (25)

comment:1 ericmann4 years ago

+1 on using doing_hook() as the generic name.

When I first read the title of this ticket, I was very skeptical that this function would have a practical use. After reading your description, though, I can think of at least 3 times I've had to work around the lack of this function to do the same or very similar things. Fantastic idea!

comment:2 scribu4 years ago

It would be nice if we had consistent names and behaviour:

  • current_hook()
  • doing_hook()
  • did_hook()

On did_hook(): Both do_action() and apply_filters() can be called multiple times, so there's no reason to artificially limit ourselves to did_action().

comment:3 Denis-de-Bernardy4 years ago

Isn't there an in_filter() function?

comment:4 jane3 years ago

  • Keywords needs-patch added
  • Type changed from defect (bug) to enhancement

comment:5 jane3 years ago

  • Milestone changed from 3.1 to Future Release

Punting as no patch and entering beta.

comment:6 nacin3 years ago

  • Keywords 3.2-early added

Nacin3 years ago

Adds current_action() as a wrapper for current_filter(). Adds doing_action() and doing_filter(). Ignores the call for a counterpart to did_action() for filters (should be a new ticket).

nacin3 years ago

comment:7 nacin3 years ago

  • Keywords has-patch added; needs-patch 3.2-early removed
  • Milestone changed from Future Release to 3.3

14994.2.diff also adds support for doing_action( $action = null ), where the default would mean it'd let you know whether any action is currently in the stack.

comment:8 nacin2 years ago

  • Milestone changed from 3.3 to Future Release

Too late for this, obviously. I had only brought it into 3.3 for #18548, which was punted.

comment:9 greenshady20 months ago

  • Cc justin@… added

comment:10 nacin3 months ago

#26835 was marked as a duplicate.

comment:11 nacin3 months ago

  • Keywords commit needs-unit-tests added
  • Milestone changed from Future Release to 3.9

ericmann3 months ago

Refresh patch and add unit tests

comment:12 follow-up: nacin3 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Cool :-)

comment:13 in reply to: ↑ 12 ; follow-up: Denis-de-Bernardy3 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

Replying to nacin:

Cool :-)

*Cough*. No…

I hate to be the one highlighting this, but the unit tests are less than useless. They're literally testing trivial calls to php's built-in []=, empty() and in_array().

If you insist on getting unit tests for this, at least mandate proper tests that actually use the relevant WP API, instead of sidestepping it and populating an array in case the internals change:

  1. call add_filter(), twice.
  2. apply_filter() on the hook
  3. Assert, *within the filter* as it is being called (or not), that the two hooks are called (or not).

comment:14 in reply to: ↑ 13 nacin3 months ago

  • Keywords needs-unit-tests removed

Replying to Denis-de-Bernardy:

If you insist on getting unit tests for this, at least mandate proper tests that actually use the relevant WP API

What ericmann wrote are textbook unit tests. Most of our tests are actually integration tests, yes, and it would help to also have such tests that use the API fully. But there's nothing wrong with the mocking done here.

ericmann3 months ago

Use current_filter()/current_action() rather than a call to a global variable

comment:15 follow-up: ericmann3 months ago

  • Keywords has-unit-tests added

If you insist on getting unit tests for this, at least mandate proper tests that actually use the relevant WP API, instead of sidestepping it and populating an array in case the internals change

No. Unit tests are functions that test the smallest unit of work in a program - in this case the actual functions being added by the ticket. The relevant API you want tested, in the way you want it tested, would be an integration tests - some WP tests are integration tests, some are unit tests, so I can understand the confusion.

To make things a little bit cleaner here, I've modified the original function calls to reference current_filter() and current_action() instead of a global variable. This way, if the internals of the current_*() methods change, it's a cleaner cut.

But mocking the internal behavior of these functions - that depend on a global array - is still necessary, so manually populating the global, even if the function we're testing no longer directly depends on it, is still the way to test the functions themselves and not the entire action/filter API.

I have no problem pointing out that this is proper encapsulation of the proposed functionality (i.e. "separation of concerns" => http://en.wikipedia.org/wiki/Separation_of_concerns separation of concerns), something WP could use a lot more of.

If there are problems with the tests themselves or the patch as a whole, let's discuss.

comment:16 in reply to: ↑ 15 Denis-de-Bernardy3 months ago

Replying to ericmann:

In my own code ghetto, a test is meant to allow, when the underlying implementation details changes, to validate that -- because the tests still pass -- things still work as defined and expected for whoever consumes the API. That is useful. And meaningful.

The suggested tests, in contrast, say nothing about whether the APi works; only about whether the APi's internals are left unchange. Whoever changes the internals will need to rewrite the tests. This means an extra burden on future contributors who decide to do so, with no benefit. It is useless and meaningless.

But anyway... Let's not get that in the way of getting the actual patch checked in... +1 for getting the new functionality into WP.

comment:17 ircbot2 months ago

This ticket was mentioned in IRC in #wordpress-dev by ericmann. View the logs.

comment:18 follow-up: nacin2 months ago

  • Keywords needs-patch needs-unit-tests added; has-patch commit has-unit-tests removed

14994.4.diff changed how these functions were supposed to work. They are not merely a different way of checking the current filter. They are for checking if a filter is in progress. Filters can be nested to create a "stack" — this is to check if the filter is anywhere in the stack. 14994.3.diff seems better.

comment:19 in reply to: ↑ 18 ericmann2 months ago

Replying to nacin:

Filters can be nested to create a "stack" — this is to check if the filter is anywhere in the stack. 14994.3.diff seems better.

In hindsight, I see that you're right with this. The attempt of 14994.4.diff was to remove the dependency on a global array, but _does_ functionally change things since using current_filter() will only return the current filter, rather than checking against all others in the stack.

For this particular change, I think 14994.3.diff is sufficient. In the future, perhaps a current_filters() or similar function (abstracting from the global) could be used as a replacement.

comment:20 nacin7 weeks ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 27294:

Introduce doing_filter() and doing_action() to identify hooks in progress.

did_action() returns true the moment a hook is initially run, leaving you no way to tell if the hook is still in progress. Hooks can be nested and this checks the entire stack, versus current_filter() which only identifies the final hook in the stack. This commit also introduces current_action() for parity.

To tell if a hook has completed, one can use did_action() and ! doing_action() together.

The functions do not require an argument. In that situation, they indicate whether the stack is empty.

props ericmann for the initial unit tests.
fixes #14994.

comment:21 DrewAPicture12 days ago

In 28010:

Improve inline documentation for functionality in wp-includes/plugin.php added in 3.9.

See #14994, #27700.

Note: See TracTickets for help on using tickets.