Make WordPress Core

Opened 6 years ago

Last modified 4 months ago

#9968 reopened defect (bug)

dynamically adding/removing filters breaks plugins

Reported by: Denis-de-Bernardy Owned by:
Milestone: Future Release Priority: low
Severity: normal Version:
Component: Plugins Keywords: has-patch needs-testing
Focuses: Cc:


noticed this while doing something similar to this:

add_action('foo', 'bar1', 10);
add_action('foo', 'bar2', 20);

function bar1() {
  remove_filter('foo', 'bar1', 10);

in the above, bar2() doesn't get called. it is because of the usage of next()/current() to loop through filters.

attached patch uses a foreach loop instead, to make it work.

Attachments (2)

9968.diff (1.1 KB) - added by Denis-de-Bernardy 6 years ago.
9968.2.diff (1.2 KB) - added by Denis-de-Bernardy 6 years ago.
the two reset calls probably aren't needed either

Download all attachments as: .zip

Change History (16)

6 years ago

the two reset calls probably aren't needed either

#1 @Denis-de-Bernardy
6 years ago

oh, and, in case you're wondering why this is useful, it's to cope with output buffers, with an ob_start on the one hand side that removes itself (so as to not get triggered more than once), and an ob_flush on the other that removes itself for the same reason.

#2 @dd32
6 years ago

  • Keywords needs-testing added; tested commit removed
  • Milestone changed from 2.8 to Future Release
  • Priority changed from high to low
  • Severity changed from blocker to normal

We've been down this route a few times over the years i think.

See #5338 for the latest such example.

Any changes to the Plugin API would require extensive testing as well.

#3 @Denis-de-Bernardy
6 years ago

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

closing as wontfix then. the issue raised against the foreach in that other ticket is the same as mine, but the other way around. arbitration probably goes towards backwards compat.

#4 @hakre
5 years ago

Related: #10535

#5 @scribu
5 years ago

Related: #16245

#6 @scribu
5 years ago

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

#7 @azizur
5 years ago

  • Cc azizur added

#8 @scribu
4 years ago

Related: #17817

#9 @solarissmoke
3 years ago

Akismet is probably the most widely used plugin that does this (calls remove_action on a currently active filter) and it breaks a lot of other plugins that use that filter ( Bug report and trac ticket).

If this is difficult to fix outright, perhaps we could raise a debug error when remove_action is called on an active filter? At least that way plugin developers would get a warning when they're doing it wrong.

#10 @Chouby
3 years ago

  • Cc frederic.demarle@… added

Just ran into the same issue of a plugin broken by another one due to this.

#11 @SergeyBiryukov
2 years ago

Related/duplicate: #21169

#12 @nikolov.tmw
10 months ago

Might be a silly question, but why not just remove

if ( empty( $GLOBALS['wp_filter'][ $tag ][ $priority ] ) ) {
	unset( $GLOBALS['wp_filter'][ $tag ][ $priority ] );

From remove_filter()? Because I believe the problem occurs when you remove a priority key(once there are more callbacks for that priority for the given action/filter) from the array.

In my testing commenting-out unset(... fixed the issue altogether.

However I'm not familiar enough with the inner-workings of the filter API to foresee whether doing so will create any side effects.

I believe that having an empty priority key should not be too much of an issue, since we're using foreach( (array) current( $wp_filter[$tag[ )... ) in order to go through the different priorities, which means that if a key is empty, then simply nothing will happen(you'll move to the next priority in the stack).

I was just debugging a conflict between plugins and calling remove_action() was the culprit. To be honest, having a buggy implementation of remove_filter() and remove_action() is not really good(because sometimes you do need to have filters/actions that only run once).

Feedback from someone with more insight would be appreciated :)

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

7 months ago

#14 @SergeyBiryukov
4 months ago

#33144 was marked as a duplicate.

Note: See TracTickets for help on using tickets.