Template activation: update migration

[ellatrix]

Adds the migration logic needed for template activation.

See WordPress/gutenberg#67125. See WordPress/gutenberg#66950.

Any template is the database that is linked to the current theme should be activated. The existing query can be found at https://developer.wordpress.org/reference/functions/get_block_templates/, it's a 100% copy of that. Also note a few lines down that the status is set to publish when wp_id is not set.

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

Testing instructions: this can be tested by adding a new inactive template, then applying this patch. The admin should redirect to a database upgrade, after which the template you just added should be activated. This simulates templates edits pre-6.9.

---

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.

[github-actions]

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 ellatrix, westonruter, desrosj.

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

[github-actions]

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.

[desrosj]

Thanks @ellatrix!

I left some thoughts. Mainly, I am concerned about the unbounded query here even though it already exists in get_block_templates() today.

Could you link to some additional context about why this is necessary? What changed to require templates to be activated in 6.9?

[desrosj]

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.

[ellatrix]

• 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).

[ellatrix]

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

[dd32]

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.

[dd32]

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.

[mcsf]

(And then within that filter we could set the option in the database for the next time it's accessed.)

Is this safe? If so, that would work for me too. Although Dion's suggestion seems a bit more explicit.

[ellatrix]

@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.

[mcsf]

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?

[desrosj]

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.

[dd32]

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..

[ellatrix]

@desrosj Here is some context: WordPress/gutenberg#67125 and WordPress/gutenberg#66950.