Make WordPress Core

Opened 3 years ago

Closed 19 months ago

Last modified 18 months ago

#53940 closed enhancement (fixed)

Reconsider return types for `_get_cron_array()`.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Cron API Keywords: good-first-bug has-patch has-unit-tests add-to-field-guide
Focuses: Cc:

Description

`_get_cron_array()` currently has multiple return types:

  • false: The cron option is not set/is set incorrectly
  • array: The cron option is set correctly and either does or does not contain any cron jobs.

As either type can be returned when there are no cron jobs, I think it's worth discussing whether the return types are correct, specifically:

  • should the function always return an array
  • should the function always return false when there are zero cron jobs

Either change would improve the consistency of the return type but be a technical break of backward compatibility.

My inclination is to always return an array as it's fair to expect delete_option( '_cron' ) zeros out the cron jobs in the same way that _set_cron_array( [] ) does.

I also think always returning an array when there is no option/no cron jobs is more consistent than returning a different type when there are zero registered jobs depending on how they were deleted.

Follow up to #3058, #53635.

Attachments (1)

53940.patch (645 bytes) - added by thakkarhardik 19 months ago.

Download all attachments as: .zip

Change History (16)

#1 @jrf
3 years ago

I also think always returning an array when there is no option/no cron jobs is more consistent than returning a different type when there are zero registered jobs depending on how they were deleted.

@peterwilsoncc I concur, though changing the return type is a BC-break and may break code relying on the false return type.

FYI: we discussed this function and the return type in last weeks livestream about PHP 8.1.

#2 follow-up: @jrf
3 years ago

Note: the BC break may be acceptable in this case as the function is marked as internal only.

#3 @jrf
3 years ago

For visibility - there is a ticket open to add tests to this function: #53927

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#4 in reply to: ↑ 2 ; follow-up: @peterwilsoncc
3 years ago

Replying to jrf:

Note: the BC break may be acceptable in this case as the function is marked as internal only.

That's what I was thinking but hoping for thoughts from other committers such as yourself.

There are a few plugins in the repository using it directly, but a check of those with over 100,000 installs shows that many assume it's always an array and those that don't would be unaffected by such a change.

FYI: we discussed this function and the return type in last weeks livestream about PHP 8.1.

Yeah, I saw that while cleaning the house on the weekend. It's what prompted me to start thinking whether it should always return an array.

#5 in reply to: ↑ 4 @jrf
3 years ago

Replying to peterwilsoncc:

There are a few plugins in the repository using it directly, but a check of those with over 100,000 installs shows that many assume it's always an array and those that don't would be unaffected by such a change.

Verified and I concur, though it is painful to see that some of those huge plugins are doing it wrong.

Yeah, I saw that while cleaning the house on the weekend. It's what prompted me to start thinking whether it should always return an array.

That's wonderful to hear! Hoping that we'll be able to inspire many more improvements via those live streams in the future 💗

#6 @peterwilsoncc
2 years ago

In 52656:

Upgrade: Prevent warnings upgrading cron array.

An unvisited site may have an undefined cron array, resulting in _get_cron_array() returning the value false. Previously this would trigger warning in upgrade_590() as the function assumed _get_cron_array() would alway return an array.

No database version change is required as the upgrade routine was successful on sites with a cron array during 5.9.0. On sites without a cron array, the error has already been thrown if they are running db version 51917. This fix is only required for new sites or those upgrading that have skipped 5.9.0.

Follow up to [51917].

Props chrisvanpatten, kapilpaul, SergeyBiryukov.
Fixes #54906.
See #53940.

#7 @peterwilsoncc
2 years ago

In 52663:

Upgrade: Prevent warnings upgrading cron array.

An unvisited site may have an undefined cron array, resulting in _get_cron_array() returning the value false. Previously this would trigger warning in upgrade_590() as the function assumed _get_cron_array() would alway return an array.

No database version change is required as the upgrade routine was successful on sites with a cron array during 5.9.0. On sites without a cron array, the error has already been thrown if they are running db version 51917. This fix is only required for new sites or those upgrading that have skipped 5.9.0.

Follow up to [51917].

Props chrisvanpatten, kapilpaul, SergeyBiryukov, audrasjb.
Merges [52656] and [52662] to the 5.9 branch.
Fixes #54906.
See #53940.

#8 @peterwilsoncc
20 months ago

  • Milestone changed from Awaiting Review to 6.1
  • Type changed from defect (bug) to enhancement

I think this can be done and _get_cron_array() always return an array.

It's a BC change but as zero scheduled tasks can currently either return false or an empty array I think it's an improvement.

#9 @johnbillion
20 months ago

  • Keywords good-first-bug added

#10 @thakkarhardik
19 months ago

  • Keywords has-patch added; needs-patch removed

Hi @peterwilsoncc, I've added a patch which will always return array instead of false on failure from _get_cron_array().

This ticket was mentioned in PR #3038 on WordPress/wordpress-develop by peterwilsoncc.


19 months ago
#11

  • Keywords has-unit-tests added; needs-unit-tests removed

#12 @peterwilsoncc
19 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 53791:

Cron API: Modify _get_cron_array() to always return an array.

Change the return type of _get_cron_array() to an empty array if the cron option is either missing or of an
unexpected type.

This change ensures the return value for no registered events is consistently an empty array. Previously the return
value could be either an empty array or false.

Props thakkarhardik, jrf, costdev.
Fixes #53940.

#14 @peterwilsoncc
19 months ago

Thanks @thakkarhardik, I've committed your patch.

I ended up including a bunch of test changes alongside your fix to make sure the function was working as intended.

#15 @milana_cap
18 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.