WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 3 months ago

#40161 reopened enhancement

Wrong documented or coded 'schedule_event' filter

Reported by: esemlabel Owned by:
Milestone: 5.0 Priority: low
Severity: normal Version:
Component: Cron API Keywords: needs-patch
Focuses: docs 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 16 months ago.
patch to fix the issue

Download all attachments as: .zip

Change History (9)

#1 @esemlabel
16 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
16 months ago

patch to fix the issue

#2 @abhishekfdd
16 months ago

  • Keywords has-patch added

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

#6 @peterwilsoncc
3 months 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
3 months 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
3 months ago

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

Note: See TracTickets for help on using tickets.