WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 6 weeks 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:

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 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)

@Denis-de-Bernardy6 years ago

@Denis-de-Bernardy6 years ago

the two reset calls probably aren't needed either

comment:1 @Denis-de-Bernardy6 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.

comment:2 @dd326 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.

comment:3 @Denis-de-Bernardy6 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.

comment:4 @hakre5 years ago

Related: #10535

comment:5 @scribu5 years ago

Related: #16245

comment:6 @scribu5 years ago

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

comment:7 @azizur4 years ago

  • Cc azizur added

comment:8 @scribu4 years ago

Related: #17817

comment:9 @solarissmoke2 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.

comment:10 @Chouby2 years ago

  • Cc frederic.demarle@… added

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

comment:11 @SergeyBiryukov2 years ago

Related/duplicate: #21169

comment:12 @nikolov.tmw7 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 :)

comment:13 @slackbot4 months ago

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

comment:14 @SergeyBiryukov6 weeks ago

#33144 was marked as a duplicate.

Note: See TracTickets for help on using tickets.