Make WordPress Core

Opened 15 years ago

Last modified 4 years ago

#9968 reopened defect (bug)

dynamically adding/removing filters breaks plugins

Reported by: denis-de-bernardy's profile Denis-de-Bernardy Owned by:
Milestone: Awaiting Review Priority: low
Severity: normal Version:
Component: Plugins Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

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 15 years ago.
9968.2.diff (1.2 KB) - added by Denis-de-Bernardy 15 years ago.
the two reset calls probably aren't needed either

Download all attachments as: .zip

Change History (23)

@Denis-de-Bernardy
15 years ago

the two reset calls probably aren't needed either

#1 @Denis-de-Bernardy
15 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
15 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
15 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
13 years ago

Related: #10535

#5 @scribu
13 years ago

Related: #16245

#6 @scribu
13 years ago

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

#7 @azizur
13 years ago

  • Cc azizur added

#8 @scribu
13 years ago

Related: #17817

#9 @solarissmoke
11 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
11 years ago

  • Cc frederic.demarle@… added

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

#11 @SergeyBiryukov
11 years ago

Related/duplicate: #21169

#12 @nikolov.tmw
9 years 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.


9 years ago

#14 @SergeyBiryukov
9 years ago

#33144 was marked as a duplicate.

#15 @swissspidy
7 years ago

  • Keywords dev-feedback added

Is this still a thing with #17817 now fixed?

#16 @Rarst
7 years ago

  • Resolution set to worksforme
  • Status changed from reopened to closed

Can no longer reproduce as described, think it did get resolved.

#17 @netweb
7 years ago

  • Milestone Future Release deleted

#18 @nprasath002
6 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

I was able to reproduce this with the following code

add_action( 'init', 'first_callback', 777 );
add_action( 'init', 'second_callback', 778 );
add_action( 'init', 'third_callback', 779 );

function first_callback() {
	remove_action( 'init', 'first_callback', 777 );
}

function second_callback() {
	die( 'This want get executed' );
}

function third_callback() {
	// This will get executed
}

#19 @adamsilverstein
6 years ago

I can confirm that testing with the code provided by @nprasath002, the second callback is never executed.

#20 @swissspidy
6 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch needs-testing dev-feedback removed
  • Milestone set to Awaiting Review

This ticket was mentioned in Slack in #core-editor by aduth. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.