WordPress.org

Make WordPress Core

Opened 4 weeks ago

Last modified 4 weeks ago

#53940 new defect (bug)

Reconsider return types for `_get_cron_array()`.

Reported by: peterwilsoncc Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Cron API Keywords: needs-patch needs-unit-tests
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.

Change History (5)

#1 @jrf
4 weeks 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
4 weeks ago

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

#3 @jrf
4 weeks ago

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

Last edited 4 weeks ago by SergeyBiryukov (previous) (diff)

#4 in reply to: ↑ 2 ; follow-up: @peterwilsoncc
4 weeks 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
4 weeks 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 💗

Note: See TracTickets for help on using tickets.