WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#12492 closed defect (bug) (fixed)

Improper use of a variable variable ($$) in remove_all_filters()

Reported by: chrisjean Owned by: westi
Milestone: 3.0 Priority: normal
Severity: normal Version: 3.0
Component: Plugins Keywords: has-patch tested commit
Focuses: Cc:

Description

In wp-includes/plugin.php's remove_all_filters function are the following lines of code:

if( false !== $priority && isset($$wp_filter[$tag][$priority]) )
    unset($wp_filter[$tag][$priority]);

The $$ is a variable variable, which is clearly a typo in this case given the next line. I first found this when the following warning was generated by trying to make use of the priority option of remove_all_actions:

Notice: Array to string conversion in /opt/lampp/htdocs/trunk/wp-includes/plugin.php on line 225

Since this amounts to checking for variables of type $false, $10, or any other given argument, which won't exist, the second argument is currently useless. Thus, until the supplied patch is implemented, it won't be possible to use remove_all_filters or remove_all_actions to remove all hooks of a specific priority.

A sample plugin showing this bug is attached. The sample plugin hooks the same function to the template_redirect action three times with priorities of default, 20, and 30. It then uses the remove_all_actions function (which simply calls remove_all_filters) to remove all template_redirect hooks for priority 10. This should result in the hooked function being called twice, but produces nothing as all the hooks are cleared.

With my supplied patch applied, the hooked function is called twice, echoing "This should render twice." each time. Thus, the expected result is achieved with this patch.

Attachments (3)

12492-example.php (576 bytes) - added by chrisbliss18 6 years ago.
Example plugin that shows bug
12492-patch.diff (510 bytes) - added by chrisbliss18 6 years ago.
Bug fix
12492.patch (1.1 KB) - added by hakre 6 years ago.
Some code cleanups added.

Download all attachments as: .zip

Change History (7)

@chrisbliss186 years ago

Example plugin that shows bug

@chrisbliss186 years ago

Bug fix

comment:1 @hakre6 years ago

Thanks for reporting, looks like this is unnoticed since a longer period, westi put that code in back some time.

Reference: r8660

Reviewed, report is valid IMHO.

comment:2 follow-up: @chrisbliss186 years ago

  • Cc gaarai@… added

Yeah. It's an unfortunate typo that has been there as long as the function has been around.

I did a search through trunk for other $$ typos and didn't see any. Personally, I'd like to see the $$ uses removed. The same functionality can be derived in numerous ways. The main issue with variable variables is that they are difficult to distinguish from a bug and make code clarity very difficult.

@hakre6 years ago

Some code cleanups added.

comment:3 in reply to: ↑ 2 @hakre6 years ago

Replying to chrisbliss18:

Yeah. It's an unfortunate typo that has been there as long as the function has been around.

ack.

I did a search through trunk for other $$ typos and didn't see any. Personally, I'd like to see the $$ uses removed. The same functionality can be derived in numerous ways. The main issue with variable variables is that they are difficult to distinguish from a bug and make code clarity very difficult.

true.

comment:4 @nacin6 years ago

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

(In [13567]) Fix typo in remove_all_filters() that prevented hooks from being removed by priority. fixes #12492, props chrisbliss18.

Note: See TracTickets for help on using tickets.