WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 7 months ago

#40161 new enhancement

Wrong documented or coded 'schedule_event' filter

Reported by: esemlabel Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Cron API Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/cron.php#L41
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

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

Attachments (1)

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

Download all attachments as: .zip

Change History (6)

#1 @esemlabel
7 months ago

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

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

@abhishekfdd
7 months ago

patch to fix the issue

#2 @abhishekfdd
7 months ago

  • Keywords has-patch added

#3 @swissspidy
7 months 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 months 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 months 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.

Note: See TracTickets for help on using tickets.