Unscheduling cron jobs fails when original arguments were not an array.

[jrf]

The Cron API does not check whether the cron job arguments passed are an array when scheduling a cron job. This inadvertently allows for scheduling cron jobs with string, integer or other arguments.

However when unscheduling the cron job using wp_clear_scheduled_hook(), the arguments are always cast to an array which leads to cron jobs which *can* be scheduled, but can't be *un*scheduled using wp_clear_scheduled_hook().

The wp_clear_scheduled_hook() does throw a deprecated notices when non-array arguments are passed in, but this will most of the time go unnoticed as this function is most often used in a plugin deactivation routine.

The patch which I'm submitting makes sure that cron job arguments are always cast to an array.

The patch is backward compatible in that it:

• will not break the schedule_event filter for plugins (which are doing it wrong) which expect their original non-array argument to test against.
• will schedule all newly schedule events with array arguments independently of how the arguments were passed.
• will upgrade the cron array to ensure that all arguments are arrays.

The patch includes unit tests proving the existence of the bug and the fixing of it by this patch.

As far as I can see, this bug was introduced by the changes in https://core.trac.wordpress.org/changeset/12462 and has been in WP since 3.0.

The patching of this bug also brought to my attention *another* (ancient) bug where in the cron option upgrade routine _upgrade_cron_array() the array structure wasn't respected properly leading to Undefined index: args notices and the inadvertent removal of cron events which were scheduled on the same hook for the same timestamp with different arguments.

That bug has also been fixed in this patch.

[jrf]

Refreshed the patch to include the ticket number in the docblock of the unit tests.

[jrf]

Oh and FYI - this patch also takes care of the following PHP warnings:

PHP Warning:  array_slice() expects parameter 1 to be array, string given in /wp-includes/plugin.php on line 601

PHP Stack trace:
PHP   1. {main}() /wp-cron.php:0
PHP   2. do_action_ref_array($tag = 'testing', $args = 'STRING SINGLE Testing string arg') /wp-cron.php:117
PHP   3. array_slice('STRING SINGLE Testing string arg', 0, 1) /wp-includes/plugin.php:601


PHP Warning:  call_user_func_array() expects parameter 2 to be array, null given in /wp-includes/plugin.php on line 601

PHP Stack trace:
PHP   1. {main}() /wp-cron.php:0
PHP   2. do_action_ref_array($tag = 'testing', $args = 'STRING SINGLE Testing string arg') /wp-cron.php:117
PHP   3. call_user_func_array:{/wp-includes/plugin.php:601}('testing', NULL) /wp-includes/plugin.php:601

[jrf]

Updated for bug scrub feedback

[jrf]

Same as 2a + _doing_it_wrong()

[jrf]

Updated the patch based on feedback from the bug scrub Slack chat. /cc @chriscct7 @stevenkword

I think the patch should also throw a _doing_it_wrong() specifically where the plugin authors are doing it wrong

There is already a _deprecated_argument() in place in the wp_clear_scheduled_hook() function which was added when the function signature was changed.

I could add _doing_it_wrong() for all instances where the cron arguments are not passed in as an array. That would effectively mean adding it in six (!) places in the cron file.
See the alternative patch (2-b).

Also note: _doing_it_wrong() expects the WP version to be the version in which the message was added (4.5 (?)), however, the actual change in the function signature occurred in WP 2.1.0 as far as I can see, so IMHO it would make more sense to have 2.1.0 as the version number in the _doing_it_wrong() call.

also it'd be nice to get the related ticket ( *another* (ancient) bug), so that ticket can also be updated.

AFAIK this was never reported before and there is no trac ticket for this bug. I discovered it while working on this issue.

As the last time the version nr of the cron array was changed - and therefore the last time the _upgrade_cron_array() function was called -, was in 2006, I doubt you will find any reported issues.

The bug, however, becomes relevant for this patch as the cron array version nr will be upped and therefore _upgrade_cron_array() will now once again be called (once for each install).

Aside from the error notice, the net effect of this bug is cron events disappearing or no longer functioning correctly as the cron array would be mangled after being run through the upgrade function.

In more detail:
The cron array is a nested array with:
(array) timestamps -> (array) hooks -> (array) events -> (array) arguments, where events are saved with as a key the md5-ed serialization of the event arguments and as a value the actual event arguments.

The _upgrade_cron_array() function however treated the events array as if it were the arguments array. So if there were two events on a hook, there would only be one event after the upgrade routine and the arguments of that one event would be the previous two events.

and the new _cron_cast_to_array_helper needs a docbloc

Done - see updated patch.

[swissspidy]

The Cron API does not check whether the cron job arguments passed are an array when scheduling a cron job. This inadvertently allows for scheduling cron jobs with string, integer or other arguments.

How many plugins really do this? Is it worth papering over their faulty code and properly enabling non-array arguments? Couldn't we just call _doing_it_wrong() and return false?

See also #35491, where I suggested passing false to wp_next_scheduled() in order to get the next scheduled event for a hook regardless of the arguments. This patch here would prevent that from happening.

[jrf]

Responded to @swissspidy's comment: https://core.trac.wordpress.org/ticket/35491#comment:13

[jrf]

Refreshed against current master

[jrf]

Refreshed against current master

[jrf]

Please ignore the wp_next_schedules.3.patch file - my bad, uploaded the wrong patch and there is no way for me to delete it....

[jrf]

Just wondering if this patch can get some love ? I'd be happy to update it if someone would give the go-ahead for 4.9.

[peterwilsoncc]

The new function wp_get_scheduled_event() to be shipped in WordPress 5.0 also fails to include the check in wp_clear_scheduled_hook().

I'm still digging into this ticket but if it's to be fixed, the existing patch will need to be refreshed beforehand. I've marked it as such but I can figure out the goal from the existing patch so a refresh can wait until a milestone is added.

My current leaning is toward consistency within the API.

[peterwilsoncc]

34913.diff​ includes:

• refreshed against trunk
• Added deprecation check for new function wp_get_scheduled_event()
• Modified cron option upgrade routine to ensure $args are stored as an array
• Bumped database version as part of upgrade routine
• Switched tests to use a data provider

@jrf Are you able to take a look and let me know your thoughts?

I agree with your comment on ticket 35491 that a bug fix trumps an enhancement request but would be delighted if a lighter touch fix presented itself. I can't think of a lighter touch approach though.

TO DO: Database version may have changed by WP 5.1.0. Any commit will need to update these details.

[peterwilsoncc]

@jrf and I had an in person discussion about this last night.

• the cause of this ticket is that the cron array includes items in stored in an unexpected format
• unexpected data in the cron array was also causing a problem in PHP 8.1, fixed in [51695]
• each of the cron functions assumes _get_cron_array() returns a correctly formed cron array. This isn't entirely a safe assumption as a plugin may save it incorrectly or filter the cron option getter.

As PHP becomes stricter about type and introduces more fatal errors for incorrect type issues, we were considering whether it would be better to validate the cron array on the way out.