Make WordPress Core

Opened 7 years ago

Last modified 5 years ago

#40161 reopened enhancement

Wrong documented or coded 'schedule_event' filter

Reported by: esemlabel's profile esemlabel Owned by:
Milestone: Future Release Priority: low
Severity: normal Version:
Component: Cron API Keywords: needs-patch
Focuses: docs Cc:

says that $event parameter should always be an object.

But the following code allows to terminate script only when passing "false" values to filter ("", array(), null, 0 or false), whereas checking false as object will produce error.

The documentation should be changed to force check isset( $event->hook ) when using this filter, otherwise the filter should be changed to something like this

$event = apply_filters( 'schedule_event', $event->hook, $event );
if ( ! $event->hook )
    return false;

Attachments (1)

40161.patch (386 bytes) - added by abhishekfdd 7 years ago.
patch to fix the issue

Download all attachments as: .zip

Change History (11)

#1 @esemlabel
7 years ago

To not break existing plugins, we can change condition, which allows to "false" only the hook name value

if ( ! $event->hook )
    return false;

7 years ago

patch to fix the issue

#2 @abhishekfdd
7 years ago

  • Keywords has-patch added

#3 @swissspidy
7 years ago

  • Keywords reporter-feedback added
  • Type changed from defect (bug) to enhancement
  • Version trunk deleted

I cannot seem to understand the issue you're facing with this.

Developers would do something like this:

function my_filter_event( $event ) {
    if ( $event  && 'foo' === $event->hook ) {
        // Prevent this specific event from running.
        return false;

    return $event;

add_filter( 'schedule_event', 'my_filter_event' );

Of course $event should be an object, but you can return false to short-circuit this. If that's confusing, we can look at updating the inline documentation.

#4 @esemlabel
7 years ago

Yes, someone can return false to short-circuit this. But many of other developers will produce error when hook in to their even with filter

if ( $event->hook === 'my_plugin_event' ) then perform something inside my plugin

, because they can't predict if anyone else will use such way to prevent event from running.
The one way without modifying core is always use low priority for every filter hook, that return false to 'schedule_event' filter. Otherwise, there is huge probability to catch debug error from plugins and themes.

#5 @swissspidy
7 years ago

  • Keywords 2nd-opinion added; reporter-feedback removed

But many of other developers will produce error when hook in to their even with filter […] , because they can't predict if anyone else will use such way to prevent event from running.

But that's the same with every filter out there, isn't it? That's why I added a more strict check in my previous example.

If you understand you correctly, with 40161.patch you want to say "don't just return false, but set the hook property to a falsey value, so others don't run into problems". If so, then I think 40161.patch is not enough and this would need to be adjusted in the documentation as well, to educate developers.

However, returning false in a filter to short-circuit something is a common pattern in WordPress and in this particular scenario I don't see much value in changing this. I mean, all the plugins out there using this filter won't suddenly change their code. You'd still run into the same problems.

#6 @peterwilsoncc
6 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I'm closing this as wontfix, as mentioned above it's the nature of WordPress filters that plugins need to be aware a modified value may be passed to their function.

#7 @johnbillion
6 years ago

  • Focuses docs added
  • Keywords needs-patch added; has-patch 2nd-opinion removed
  • Milestone set to 5.0
  • Priority changed from normal to low
  • Resolution wontfix deleted
  • Status changed from closed to reopened

The documentation for the $event parameter can still be improved to indicate that allowed values are either an object or boolean false.

#8 @johnbillion
6 years ago

To clarify: The return value from filters can be anything, but the documentation should show the allowed or expected types.

#9 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

#10 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release
Note: See TracTickets for help on using tickets.