Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade/Install: Delay automatic updates after installation. #6828

Conversation

costdev
Copy link
Contributor

@costdev costdev commented Jun 14, 2024

Note:
The changes in this PR cannot be tested in a wordpress-develop checkout because automatic updates do not run in version-controlled systems. Download 6.6 Beta X, then paste these changes in place before attempting to install the site.

Alternatively, if you have permissions for this repo, download the build artifact from the bottom of this page.


After installation, the user is directed to the Log In page. This triggers the wp_schedule_update_checks() function which is hooked to init. As a result, the user may be presented with a maintenance mode screen just after installing WordPress.

To improve the user experience, this delays Core updates by 1 hour, plugin updates by 1.5 hours, and theme updates by 2 hours.

Trac ticket: https://core.trac.wordpress.org/ticket/61457

@costdev costdev marked this pull request as ready for review June 14, 2024 18:03
Copy link

github-actions bot commented Jun 14, 2024

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props costdev, peterwilsoncc, hellofromtonya, dd32.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

Test using WordPress Playground

The 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

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@costdev costdev force-pushed the delay_automatic_updates_after_installation branch from ca61465 to d31f613 Compare June 15, 2024 04:14
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on WordPress-Develop by adding add_filter( 'automatic_updates_is_vcs_checkout', '__return_false' ); as an mu-plugin.

On trunk I was immediately presented with a maintenance mode screen, on this branch I was able to login immediately.

I think this change makes sense but it was my suggestion so of course I do. It would be good to get a few others to express an opinion too.

My one question is why the scheduling is being modified in wp_install() rather than in wp_schedule_update_checks()?

@costdev
Copy link
Contributor Author

costdev commented Jun 17, 2024

My one question is why the scheduling is being modified in wp_install() rather than in wp_schedule_update_checks()?

The main reason is context. The normal scheduling of wp_schedule_update_checks() isn't a problem for other calls to the function (extender calls included). The issue arises when it runs on the next do_action( 'init' ); call after installation. By having wp_install() set appropriate scheduling for post-installation, this ensures there is only a delay in that context.

An additional but related reason is that other calls to wp_schedule_update_checks() expect those actions to be scheduled for an instant run when no existing event exists. Changing the scheduling inside the function would break expectations when it's unnecessary to do so.

@hellofromtonya @desrosj @audrasjb @johnbillion Are any of you available to add your thoughts on this PR? I'd like to land this ahead of 6.6 Beta 3 tomorrow as the final planned Beta of the 6.6 cycle. Thanks!

@hellofromtonya
Copy link
Contributor

Test Report

Env

  • macOS
  • PHP 8.3
  • nginx
  • 6.6 Beta 2
  • Firefox
  • Single site

Test Steps

  1. I added a wp-content/mu-plugins.test.php file with this code in it:
<?php

add_filter( 'automatic_updates_is_vcs_checkout', '__return_false' );
  1. Then reset the database using WP-CLI:
wp db reset --yes
  1. Walked through the install screens.
  2. Clicked the Login link. Expect: Should be redirected to the Login screen.

Actual Results

Without this patch, able to reproduce the issue:
❌ got maintenance mode after clicking the Login link.

With this patch, ✅ was redirected to the Login screen.

This PR resolves the issue ✅

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in my test report, this patch resolves the issue.

I agree with @costdev to include the changes in wp_install() function (see his reasoning here).

This change LGTM for commit.

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@costdev @hellofromtonya Thanks, that works for me.

@dd32
Copy link
Member

dd32 commented Jun 18, 2024

My one question is why the scheduling is being modified in wp_install() rather than in wp_schedule_update_checks()?

An additional but related reason is that other calls to wp_schedule_update_checks() expect those actions to be scheduled for an instant run when no existing event exists. Changing the scheduling inside the function would break expectations when it's unnecessary to do so.

Personally, I don't think there's a realistic expectation of that, and scheduling it at time() is purely the default value that most choose to use as the starting point for cron tasks.

I don't feel strongly about this though, but I do feel it would be cleaner than the PR.

@costdev
Copy link
Contributor Author

costdev commented Jun 18, 2024

@dd32 I see your point, and though the code changes would be cleaner if only modifying wp_schedule_update_checks() directly, it's possible that this could have unintended consequences.

For example, code that clears the cron schedule for automatic updates, then runs wp_schedule_update_checks() for a one-line call to start automatic updates checks immediately. Should further functionality and/or behaviour expect that to be immediate, we don't know the potential impact a delay for all use cases could have.

Given that this is possible, it's a risk I don't feel is worth taking to resolve the issue in the ticket, which is specifically in a post-installation context.

@peterwilsoncc
Copy link
Contributor

I chatted to Colin a bit further about when to delay/offset the cron jobs.

Given that the release is in beta, we think it might be best to reevaluate whether the offset can be added in the original location post release. That will allow for further work looking in to the situation described above and a considered decision about any implications.

@costdev
Copy link
Contributor Author

costdev commented Jun 18, 2024

@dd32 Are you happy for us to commit this PR's changes for 6.6 Beta 3 and look at whether to change this to the wp_schedule_updates_checks() function in a later release?

@costdev costdev requested a review from hellofromtonya June 18, 2024 07:19
@hellofromtonya
Copy link
Contributor

Given that the release is in beta, we think it might be best to reevaluate whether the offset can be added in the original location post release. That will allow for further work looking in to the situation described above and a considered decision about any implications.

Good plan.

  • Get the fix as is committed in time for the last scheduled beta release (which is in a few hours - 16 UTC).
  • Then evaluate if there are "unintended consequences" for changing wp_schedule_update_checks() in the "post-installation context."

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM for 6.6 Beta 3 commit as it resolves the reported issue in the "post-installation context."

Plus, there's a plan to evaluate "unintended consequences" concerns of changing in wp_schedule_update_checks() in this context.

@costdev
Copy link
Contributor Author

costdev commented Jun 18, 2024

Committed in r58435.

@costdev costdev closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants