#15296 closed defect (bug) (wontfix)
Filters don't check that the function is callable
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 3.0 |
Component: | Performance | Keywords: | |
Focuses: | Cc: |
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)
#2
@
14 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
@
14 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
@
14 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
@
14 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:
↓ 7
@
14 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
@
14 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 :-)
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?