#57271 Return true when the existing stored cron array value already matches the new value#12541
#57271 Return true when the existing stored cron array value already matches the new value#12541johnbillion wants to merge 1 commit into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @liedekef. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts WordPress core’s private _set_cron_array() behavior so that when persisting the cron option reports failure (i.e., update_option( 'cron', ... ) returns false) but the stored value already matches the intended cron array, the function returns true instead of signaling failure (or returning WP_Error when $wp_error is truthy). This targets cases that currently surface “The cron event list could not be saved.” even though the desired cron value is already in place.
Changes:
- Update
_set_cron_array()to treat “no change because value already matches” as success by comparing the stored cron option to the intended value. - Refactor/add PHPUnit coverage around return values for “already stored”, “not updated”, and “WP_Error on failure” cases.
- Add a test helper to simulate
update_option( 'cron' )reporting “no change”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/wp-includes/cron.php |
Adds a post-update_option() fallback check to return true when the stored cron option already matches the intended cron array. |
tests/phpunit/tests/cron/setCronArray.php |
Updates and expands unit tests for _set_cron_array() return-value behavior, including a helper to simulate option update failure/no-change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -116,6 +122,22 @@ public function data_set_cron_array_returns_false_when_not_updated() { | |||
| ); | |||
| /** | ||
| * Tests that `_set_cron_array()` returns false when the cron option was not updated and `$wp_error` is truthy. | ||
| * | ||
| * @ticket 38903 | ||
| * @ticket 57271 | ||
| * | ||
| * @dataProvider data_set_cron_array_returns_WP_Error_when_not_updated | ||
| * | ||
| * @param array $input Cron array. | ||
| */ | ||
| public function test_set_cron_array_returns_false_when_not_updated( $input ) { | ||
| $this->force_update_option_cron_failure(); | ||
|
|
||
| $this->assertFalse( _set_cron_array( $input ) ); | ||
| } |
| // Force update_option() to report no change by keeping the existing | ||
| // value, simulating a failure to persist a genuinely new value. | ||
| add_filter( | ||
| 'pre_update_option_cron', | ||
| static fn ( $value, $old_value ) => $old_value, | ||
| 10, | ||
| 2 | ||
| ); |
| if ( ! $result ) { | ||
| // If the cron array already matches the new value, then treat it as a success. | ||
| $stored = get_option( 'cron' ); | ||
|
|
||
| if ( maybe_serialize( $stored ) === maybe_serialize( $cron ) ) { | ||
| $result = true; | ||
| } | ||
| } |
|
The AI suggestions are correct: get_option might still cache the old value. My table-cron replacement plugin shouldn't suffer from this. |
|
If I'm reading this right and the aim here is to silence the errors when cron fails to save but the existing option already contains the correct data then I don't think there's an issue with doing this. That being said, it would be good to get to the bottom of why this is happening in the first place. I also agree with @liedekef in that the AI's concern that cached data could be compared is valid and that the cache should be bypassed in this instance or both compared (if an object cache is active) to make sure everything matches! |
This PR proposes changing the return value of
_set_cron_array()to true when updating the cron array option fails but its value already matches the desired value.This is a speculative change. I don't know whether many/any of the reported instances of the "The cron event list could not be saved" error in the ticket are actually caused by the cron array having already been updated to the expected value (for example, via a concurrent process).
Is this a good idea? I think so. Does it paper over an underlying issue? I don't think it does. Feedback welcome.
Trac ticket: https://core.trac.wordpress.org/ticket/57271
Use of AI Tools
AI assistance: No
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.