WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#8723 closed defect (bug) (fixed)

Instances of same class that call variable hook first generate same wp_filter_id

Reported by: bkrausz Owned by: jacobsantos
Milestone: 2.8 Priority: normal
Severity: major Version: 2.7
Component: Plugins Keywords: trac-attention
Focuses: Cc:
PR Number:

Description

Best shown by example:

<?php
class Foo {
  var $i;
  function Foo($i) {
    $this->i = $i;
    add_action('something'.$i, array(&$this, 'bar'));
    add_action('init', array(&$this, 'baz'));
  }
  function bar() {}
  function baz() {
    echo $this->i;
  }
}

new Foo(1);
new Foo(2);
?>

This results in only seeing "2". This is because _wp_filter_build_unique_id sets $function[0]->wp_filter_id to 0 for both. The following line is the issue:

$count = isset($wp_filter[$tag][$priority]) ? count((array)$wp_filter[$tag][$priority]) : 0;

Since add_action is being called for the instance for a unique $tag in its first instance, $wp_filter[$tag][$priority] is empty for both instances...there should be some way to prevent this.

This may seem like a rare case, but I had to hunt this down for my plugin when the first action was:

add_action('wp_ajax_add-' . $this->slug, array(&$this, 'ajax_add'));

The fix is trivial: call add_action on something with a static name first, but is difficult to discover.

Attachments (3)

patch.txt (1.1 KB) - added by bkrausz 11 years ago.
Proposed patch
8723.patch (1.7 KB) - added by hakre 11 years ago.
Repaired and extended Patch
plugin.diff (2.3 KB) - added by wnorris 10 years ago.

Download all attachments as: .zip

Change History (18)

#1 @jacobsantos
11 years ago

  • Owner set to jacobsantos

#2 @jacobsantos
11 years ago

  • Status changed from new to assigned

#3 @westi
11 years ago

  • Cc westi added

@bkrausz
11 years ago

Proposed patch

#4 @bkrausz
11 years ago

Explanation, from wp-hackers:

What about using a static var that's incremented with each read to fill wp_filter_id? That guarantees it will be unique amongst other objects. It could track class names so the id stays smaller, but I see no benefit to that...

#5 @janeforshort
11 years ago

  • Keywords has-patch needs-testing added

#6 @Denis-de-Bernardy
11 years ago

  • Keywords needs-patch added; has-patch removed

broken patch

#7 @hakre
11 years ago

  • Keywords has-patch tested added; needs-patch needs-testing removed

There are two ways to handle it. You can prevent that your class becomes the same wp_filter_id over and over again by specifying it yourself before registering any hooks.

  /* constructor of class Foo */
  function Foo($i) {
    $this->i = $i;
    $this->wp_filter_id = 'Foobar' . $i;
    ...

the patch has been fixed. The patch ensures that objects who do not have such a default wp_filter_id set get a unique value attached now (that is the intended functionality) even for the same class.

additionally i corrected the docblocks a bit to clarify parameter types and return type description.

@hakre
11 years ago

Repaired and extended Patch

#8 @Denis-de-Bernardy
11 years ago

  • Keywords commit added
  • Severity changed from normal to major

#9 @ryan
10 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [11409]) Make sure filter IDs are unique. Props bkrausz, hakre. fixes #8723

#10 follow-up: @wnorris
10 years ago

  • Keywords commit removed
  • Milestone changed from 2.8 to 2.8.1
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from 2.7 to 2.8

unfortunately, this patch has a major problem with objects that don't already have wp_filter_id set. One value is stored as wp_filter_id on the object($filter_id_count++), but an entirely different value is appended to the index that gets returned (isset($wp_filter[$tag][$priority]) ? count((array)$wp_filter[$tag][$priority]) : 0;). Therefore, it's completely impossible to remove a filter on an object that was added in this way.

It seems like everyone is okay with the idea of using a static counter variable. If that's the case, then there is no need to mess around with the array size of $wp_filter[$tag][$priority]... just use the counter. And if $priority isn't being used in this manner, then it is no longer needed. This actually greatly simplifies this method, as the following patch shows.

@wnorris
10 years ago

#11 @Denis-de-Bernardy
10 years ago

@wpnorris: you sure this works with static classes?

class foo {
  function bar() {
  }
}

add_action('hook', array('foo', 'bar'));

I might be wrong, but I believe foo->wp_filter_id++ is fatal in some php versions.

#12 @Denis-de-Bernardy
10 years ago

  • Keywords needs-testing added; tested removed

mm, nevermind that. I suppose it'll work fine since there already is a line like that in the existing code.

#13 in reply to: ↑ 10 @hakre
10 years ago

  • Keywords has-patch needs-testing removed
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Version changed from 2.8 to 2.7

Replying to wnorris:
I think you're mixing two things here. The one thing is that you use(d) a (unstable) defintion of array keys in your code. That's the one side and that is a bug in your code IMHO.

The other side is, that this tickets fix was for a function called _wp_filter_build_unique_id which job it is to return a unique id per filter. The only aim of that function is to provide a unique id per filter per request.

That function had a bug. That bug was fixed.

The problem you face might be related to the bugfix but should not go into this ticket. Feel free to open a new ticket, reference this one and write, that a bug has been introduced by that certain commit in your opinion. You can then reference the new ticket here as well.

#14 @hakre
10 years ago

  • Keywords trac-attention added

Please set milestone back to 2.8

#15 @Denis-de-Bernardy
10 years ago

  • Milestone changed from 2.8.1 to 2.8
Note: See TracTickets for help on using tickets.