Skip to content

Template activation: update migration #10418

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.

Sign up for GitHub

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

Open
ellatrix wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

Template activation: update migration #10418

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a962060
Template activation: add migration
ellatrix Oct 25, 2025
aac42d7
Update src/wp-admin/includes/upgrade.php
ellatrix Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Template activation: add migration
  • Loading branch information
@ellatrix
ellatrix committed Oct 25, 2025
commit a962060e730fbbf21313b4e9969cf910ae891446
44 changes: 44 additions & 0 deletions src/wp-admin/includes/upgrade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,10 @@ function upgrade_all() {
upgrade_682();
}

if ( $wp_current_db_version < 60718 ) {
upgrade_690();
}

maybe_disable_link_manager();

maybe_disable_automattic_widgets();
Expand Down Expand Up @@ -2481,6 +2485,46 @@ function ( $url ) {
}
}

/**
* Executes changes made in WordPress 6.9.0.
*
* @ignore
* @since 6.9.0
*
* @global int $wp_current_db_version The old (current) database version.
*/
function upgrade_690() {
global $wp_current_db_version;

if ( $wp_current_db_version < 60718 ) {
// Query all templates in the database that are linked to the current
// theme and activate them. See `get_block_templates`.
$template_query_args = array(
'post_status' => 'publish',
'post_type' => 'wp_template',
'posts_per_page' => -1,
Copy link
Member

@desrosj desrosj Oct 27, 2025

Choose a reason for hiding this comment

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

I know that this is a direct copy from get_block_templates(), but relying on an unbounded query during an upgrade makes me nervous. There's a lot that can go wrong and it could result in an inaccessible site. Especially If a site happens to have many millions of posts or terms.

I think that we should revisit get_block_templates() and figure out a better way to accomplish that than a posts_per_page value of -1.

I'm not sure what the most performant option is without testing, but here are a possible ideas:

  • A loop to grab the templates using WP_Query 100 at a time until all of them have been retrieved.
  • Use get_objects_in_term() to get the list of wp_template IDs, then a direct SQL query to retrieve just the post names.
  • A cron event that batches through the templates every 2 minutes until completed, similar to the approach used when shared terms were split.

Copy link
Member Author

@ellatrix ellatrix Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  • It makes me nervous to introduce more complex logic tbh.
  • Wouldn't the site be inaccessible right now when get_block_templates() is called?
  • We don't really expect people to have a ton of templates right now (especially not millions).

Copy link
Member Author

@ellatrix ellatrix Oct 27, 2025

Choose a reason for hiding this comment

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

Btw I don’t think we do cron because that risks there to be missing templates on the front end during this time.

Copy link
Member

@dd32 dd32 Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You can't assume that upgrade routines are run, in the case of Multisite they're often not run until months after the update (IMHO - because it requires a manual action in wp-admin/network from a super-admin) and in the case of automatic updates the routines should be run but sometimes are not until a user accesses wp-admin/ and goes through the upgrade database flow.

If WordPress requires this before get_block_templates() can work, then get_block_templates() needs to be fixed rather than this, basically you should assume that the active_templates option is not set.

Copy link
Member

@dd32 dd32 Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

BTW I'm not overly concerned about the unbounded query here, post_type=wp_template&post_status=publish should be relatively small number of posts, and even in a millions-of-posts site the table indexes should be appropriate to allow querying by this without concern.

Copy link
Member Author

@ellatrix ellatrix Nov 2, 2025

Choose a reason for hiding this comment

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

@desrosj @dd32 @mcsf Any strong opinion on not running the migration on init? I've added it on init temporarily in #10425 with some other changes. What would be the difference if we run it once on getting the active templates? In both cases it's just run once, so is there another benefit? The option is always required for template resolution, so it's not like it would be delayed to later when it's needed. Btw this is how it's been in the Gutenberg plugin so far. Alternatively, if we do it just-in-time on access, I'd prefer something like migrating on a default_option_{$option} filter, if that is fine.

Copy link

@mcsf mcsf Nov 3, 2025

Choose a reason for hiding this comment

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

No real preference here between hooking to init or running in get_active_templates; I don't think it really matters, it's a one-off execution.

Where I do have a preference is to avoid migrating on default_option{$option}.

But note that, regardless of choosing init or just-in-time, Dion's point still stands: concurrent requests could trigger multiple migrations at once. It's not a traditional race condition because a concurrency scenario here can't corrupt data or change behaviours, but it could flood the server. I don't know how much of a problem this really is, though: we're talking about querying a small collection, and doing it only once. Considering how much we'd have to change throughout the system to handle the scenario where get_active_templates returns array() because a separate thread is working on the migration, I'm not sure it's worth it. What do you think, @dd32?

Copy link
Member

@desrosj desrosj Nov 4, 2025

Choose a reason for hiding this comment

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

I also have a preference to avoid default_option{$option}.

I think I prefer @dd32's suggestion because of the added protection against a potential flood of requests. If a site is hit by 1,000s of requests before the first one can complete, each of those would perform an UPDATE query.

Copy link
Member

@dd32 dd32 Nov 4, 2025

Choose a reason for hiding this comment

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

The reason I don't like it on init is because it's just yet-another function hooked onto init that will forever stay there, without any ability to remove it (Other than inlining it).

It also relies upon the reader to see the get_option() call and understand where / when that was set.

In many respects, it seems that the option is being used here as a cache layer too? In other words; this shouldn't be an option at all, and rather should be a wp_cache_get() value.. with a fallback that pulls from the data-source..

Copy link
Member Author

@ellatrix ellatrix Nov 25, 2025

Choose a reason for hiding this comment

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

@dd32 No, get_active_templates only works until the user changes the active templates. Before that happens, the migration should already have been completed. The function is used for migration only, it has to be an option.

'no_found_rows' => true,
'lazy_load_term_meta' => false,
'tax_query' => array(
array(
'taxonomy' => 'wp_theme',
'field' => 'name',
'terms' => get_stylesheet(),
),
),
);

$template_query = new WP_Query( $template_query_args );
$active_templates = array();

foreach ( $template_query->posts as $post ) {
$active_templates[ $post->post_name ] = $post->ID;
}

update_option( 'active_templates', $active_templates );
}
}

/**
* Executes network-level upgrade routines.
*
Expand Down
2 changes: 1 addition & 1 deletion src/wp-includes/version.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
*
* @global int $wp_db_version
*/
$wp_db_version = 60717;
$wp_db_version = 60718;

/**
* Holds the TinyMCE version.
Expand Down
Loading