WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 6 months ago

#35491 reopened enhancement

Add a function to check whether a hook is scheduled

Reported by: dlh Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Cron API Keywords: has-patch has-unit-tests
Focuses: Cc:
PR Number:

Description

I'm working with a plugin that calls wp_schedule_event() with an unpredictable value for $args.

Sometimes it would be helpful to know whether this plugin's event is scheduled, but because I can only guess at $args, I can't always detect it with wp_next_scheduled().

The attached patch attempts to address this scenario with a function that checks only whether a hook is scheduled, regardless of its $args.

Attachments (6)

35491.patch (1.9 KB) - added by dlh 4 years ago.
35491.2.patch (2.0 KB) - added by dlh 4 years ago.
wp_next_scheduled.diff (2.4 KB) - added by swissspidy 4 years ago.
wp_next_scheduled.2.diff (2.3 KB) - added by dlh 4 years ago.
wp_next_scheduled.3.patch (2.9 KB) - added by jrf 4 years ago.
Updated with suggestion as per comment #35491:13
35491-wp_next_scheduled.4.patch (3.1 KB) - added by jrf 4 years ago.
Rebased against current master & upped version nr of change

Download all attachments as: .zip

Change History (21)

@dlh
4 years ago

#1 @johnbillion
4 years ago

  • Keywords has-patch has-unit-tests added
  • Milestone changed from Awaiting Review to 4.5

@dlh
4 years ago

#2 @dlh
4 years ago

35491.2.patch adds one more assertion.

#3 @swissspidy
4 years ago

It doesn't hurt if wp_is_scheduled_hook returns the timestamp instead of true, right?

In that case, the function would be quite similar to wp_next_scheduled() in that it's probably just easier to extend it. For example, by just allowing to pass $args = false to wp_next_scheduled() in order to get this information.

wp_next_scheduled.diff implements this in a backwards-compatible manner.

#4 follow-up: @dlh
4 years ago

Thanks @swissspidy. With wp_next_scheduled.diff: If I scheduled 'my_hook' twice, one with 'foo' for $args and one with false, and the 'foo' event was scheduled to occur next, wouldn't wp_next_scheduled( 'my_hook', false ) return the wrong timestamp?

That aside, to me, passing false to wp_next_scheduled() adds ambiguity. How can I tell whether the author wants to know whether the hook is scheduled at all or whether it was scheduled with $args = false?

I could see how returning the timestamp would help in some cases. As you noted, though, that makes it tough to distinguish between wp_next_scheduled(). I could also see a developer thinking the function returned all the hook's timestamps, not just the next one. So I still lean towards returning bool, but I'm open to other ideas.

#5 in reply to: ↑ 4 @swissspidy
4 years ago

If I scheduled 'my_hook' twice, one with 'foo' for $args and one with false, and the 'foo' event was scheduled to occur next, wouldn't wp_next_scheduled( 'my_hook', false ) return the wrong timestamp?

$args is always an array containing the parameters passed to the callback (using call_user_func_array('wp_reschedule_event', $new_args);). Anything else isn't supported and leads to errors. That's why wp_next_scheduled( 'my_hook', false ) would work, without breaking BC.

That aside, to me, passing false to wp_next_scheduled() adds ambiguity. How can I tell whether the author wants to know whether the hook is scheduled at all or whether it was scheduled with $args = false?

My idea was to DRY things up because both functions are so similar. I agree that it may be confusing, but that can be prevented with documentation.

I could see how returning the timestamp would help in some cases. As you noted, though, that makes it tough to distinguish between wp_next_scheduled(). I could also see a developer thinking the function returned all the hook's timestamps, not just the next one. So I still lean towards returning bool, but I'm open to other ideas.

wp_next_scheduled() returns the next scheduled timestamp, that's what the function name implies.

With my patch, wp_is_scheduled_hook( $hook ) could basically be a wrapper for wp_next_scheduled( $hook, false ), if that helps prevent confusion.

#6 @dlh
4 years ago

Anything else isn't supported and leads to errors.

My mistake.

wp_next_scheduled() returns the next scheduled timestamp, that's what the function name implies.

Sorry, I meant that any new function that returned the timestamp could introduce the confusion. But it might not. In any case, I'm happy to see this go in either direction.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


4 years ago

#8 @swissspidy
4 years ago

  • Keywords commit added

We're getting closer to the enhancement and feature request deadline, so I guess it's decision time.

I still think that extending wp_next_scheduled_event() is the more straightforward and DRY solution. Patch still applies cleanly.

#9 @dlh
4 years ago

wp_next_scheduled.2.diff fixes some incorrect uses of wp_schedule_event() that I had included in the test.

There also seems to be a potential collision between this ticket and the latest patches for #34913 that might need to be sorted out, if I understand them correctly.

#10 @swissspidy
4 years ago

  • Keywords commit removed

Thanks for pointing out #34913. Definitely need to take a closer look at it.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


4 years ago

#12 @jorbin
4 years ago

  • Milestone changed from 4.5 to Future Release

#13 @jrf
4 years ago

@swissspidy @dlh I've been thinking about the conflict with ticket #34913. I believe the conflict can quite easily be avoided by adding a new parameter to the wp_next_scheduled() function to indicate whether to check the arguments or not, like so:

function wp_next_scheduled( $hook, $args = array(), $ignore_args = false )

The reasons why I'm suggesting this are as follows:

  1. IMHO a clear bug trumps an enhancement.
  2. As the bug from #34913 has been in WP since v3.0 without any doing it wrong notices, there is no telling how often devs have been doing it wrong without realizing. (and yes, I ran into it as cron jobs of deactivated plugins weren't being unscheduled.)
  3. If a doing it wrong had been added in WP 3.0 and the scheduling had been prevented from that point in time with a return false, then @swissspidy suggestion in #34913:8 would have been fine. However, 14 WP versions have been released since. Suddenly starting to return false now would break backward compatibility for all plugins which have been doing it wrong in the mean time and AFAIK breaking backward compatibility is a big no-no...
  4. The current patch in #34913 solves this cleanly without breaking backward compatibility and with my above suggestion for this patch, both could be merged without conflict.

The code for checking next scheduled events would become something like this and would maintain backward compatibility for both patches:

// Event with specific arguments:
`if ( false === wp_next_scheduled( $hook, $args ) ) {}`

// Event without arguments:
`if ( false === wp_next_scheduled( $hook ) ) {}`

// Any event independently of arguments:
`if ( false === wp_next_scheduled( $hook, null, true ) ) {}`

@jrf
4 years ago

Updated with suggestion as per comment #35491:13

#14 @swissspidy
4 years ago

Somehow I ended up as a component maintainer for the Cron API component, so I'll have another look at this :-)

@jrf
4 years ago

Rebased against current master & upped version nr of change

#16 @peterwilsoncc
11 months ago

  • Milestone set to Future Release
  • Status changed from new to reopened

This is worth some investigation so I'm reopening this and putting it down for a future release. The Cron component has a bit of a backlog, so I won't give a timeframe for now as it would set a false expectation.

Note: See TracTickets for help on using tickets.