WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#15296 closed defect (bug) (wontfix)

Filters don't check that the function is callable

Reported by: aaroncampbell Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Performance Keywords:
Focuses: Cc:
PR Number:

Description

This is a problem that I found in Shopp. Basically, they used add_filter('redirect_canonical', 'canonical_home'); when it should have been add_filter('redirect_canonical', array( $this, 'canonical_home' ));, so the function added to redirect_canonical didn't actually exist. However, it's still called and NULL is always returned, so the canonical redirects completely stopped working.

It seems like we should add an is_callable test to either apply_filters or add_filter. I'd prefer to see it on apply_filters simply because more filters are added than are actually run.

As Nacin pointed out, this is additional execution and our filters need to stay lean and mean. Still, I see both sides to this so I thought I'd try to get some feedback from other devs.

Change History (8)

#1 @scribu
9 years ago

It seems like we should add an is_callable test to either apply_filters or add_filter. I'd prefer to see it on apply_filters simply because more filters are added than are actually run.

There might be cases where the callback being added won't be available until the apply_filters() call.

The current behaviour triggers an error, making it easier to debug.

We could do the test ourselves and trigger our own error, but what's the point?

#2 @aaroncampbell
9 years ago

I know that it's really the fault of the plugin author, and should really be something they are more careful about. However, I also know that we do have things in core to try to keep plugin authors from doing stupid things. I'm just on the fence over whether it's worth it since this could potentially have a big impact.

#3 @aaroncampbell
9 years ago

Oh, and I hadn't thought about the callback not being available when the filter is added. I was really thinking this would go into apply_filters, which would avoid that issue. However, I'd like to see some stats on what is used most in a default install.

#4 @nacin
9 years ago

Argument for:

  • Prevents silent failures by skipping the filter.
  • We code defensively for plugins in many other places.

Arguments against:

  • Speed. We need these functions to be fast as possible.
  • The callback may not be available on add_filter, and apply_filters runs 1500 times on a typical pageload. (300 times for add_filter -- per westi in IRC.)
  • Silent failures do not help plugin authors.

#5 @nacin
9 years ago

When the callback doesn't exist, an E_WARNING is generated. I see no reason to guard against an obvious error such as that.

#6 follow-up: @aaroncampbell
9 years ago

  • Keywords dev-feedback removed
  • Resolution set to wontfix
  • Status changed from new to closed

I appreciate the feedback. It would be nice to protect sites from bad plugins/themes, but it's pretty obvious this is too much.

#7 in reply to: ↑ 6 @nacin
9 years ago

Replying to aaroncampbell:

It would be nice to protect sites from bad plugins/themes, but it's pretty obvious this is too much.

That's what "Deactivate" and "Delete" is for :-)

#8 @nacin
9 years ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.