Wizard Step: Theme compatibility scanning

[westonruter]

Feature description

Alongside plugin compatibility scanning (#4719), scanning is also needed for the active theme. In particular, if the theme is not generating validation errors, then Standard or Transitional modes may be good choices. If the theme is generating validation errors, however, then Reader mode may be the best bet especially if the user does not have technical ability to fix the issues or evaluate whether the invalid markup can be removed.

There's one big caveat here for recommending Reader mode, and specifically a Reader theme. In particular, it may not be advisable if the active theme is violating a best practice of registering post types, taxonomies, post types, blocks, and widgets in plugins rather than in theme. If the active theme is registering a post type, for example, and a Reader theme is selected, then this post type will be absent entirely.

What we need to do is get a list of all registered blocks, widgets, post types, and taxonomies when the active theme is running. Then we need need to do it again with the active theme switched to an empty one. If there is a difference, then the missing entities were registered in the theme and will not be available in the new Reader mode. In such a mode, only Classic theme would be recommended. And we will need to list out what will not be available if a Reader theme is selected.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

The user is guided to choose the best template mode based on their active theme/plugins.

Implementation brief

1. Add a new Onboarding Wizard step for Site Scanning as well as an area on the Settings Screen to initiate a scan.
2. Obtain validation results for URLs representative of the site, using key WP templates (homepage, single post, search results, category archive, etc. as returned by \AmpProject\AmpWP\Validation\ScannableURLProvider::get_urls()). Important: The list of URLs to validate should be returned and then each URL should be scanned. This must be done instead of scanning all URLs with loopback requests in one request to avoid timeouts.
3. Gather list of theme-registered entities (post types, shortcodes, blocks, taxonomies, etc.) which are (erroneously) registered in the theme. See Add REST endpoint providing post types, taxonomies, blocks, and widgets registered by the theme #5751.
4. Based on the above results, in the Onboarding Wizard and Settings Screen:
   1. If there are no validation errors reported at all (neither from the theme nor plugins), recommend Standard mode.
   2. If there are validation errors coming exclusively from plugins, recommend Transitional mode.
   3. If there are validation errors coming from the theme, recommend Reader mode.
5. If there are theme-registered entities (step 2 above), inform them in the Reader theme selection that AMP Legacy is the only theme available. Otherwise, any Reader theme can be chosen.
6. Make sure template mode warnings are displayed on mobile. See Wizard Step: Theme compatibility scanning #4795 (comment).

QA testing instructions

Demo

Changelog entry

[jwold]

From John:

So we’ll need something that says “Here are the features of your site that will be missing in Reader mode.” Some of this could end up being opaque, e.g., if post types, taxonomies, or blocks are registered with nonexistent or untranslatable labels. We might have to keep the language vague (e.g., “Four registered post types will be missing in Reader mode.“). Also, blocks can be registered in JS only, so that part could be challenging.

Although I guess JS-only blocks would end up being rendered and saved in the database, so would likely still work. The dynamic blocks rendered in PHP, which would be lost, can be checked in the scanner.

From Weston:

Correct. Only dynamic blocks would be a problem.

Well, other than the CSS that a theme may enqueue for JS-only blocks.

[jwold]

Is this an issue that needs copy to display what's happening to the user, or would this be a background task?

[westonruter]

The site compatibility scan involves checking URLs representative of the various templates and post types. So let's say that there are a dozen such URLs. There should be a progress bar that shows the status of checking each. The overall screen would say: “Checking Site Compatibility” with perhaps a description:

Now checking various pages of your site to see if there are any AMP compatibility issues with your active theme and plugins. This will be used to provide you with a recommendation for how to use the AMP plugin with your site.

The site compatibility scan step of the wizard should have a progress bar that shows which template is currently being checked.

[westonruter]

Here's the logic that WP-CLI uses to prevent a theme from being loaded when --skip-themes is supplied:

https://github.com/wp-cli/wp-cli/blob/cb7a8af075edf8a83d3a273e03ceb5716f15eb34/php/WP_CLI/Runner.php#L1339-L1341

https://github.com/wp-cli/wp-cli/blob/cb7a8af075edf8a83d3a273e03ceb5716f15eb34/php/WP_CLI/Runner.php#L1633-L1677

[westonruter]

Note: In #5192 (comment) I hid the template mode warnings on mobile. This should be revisited when theme scanning is implemented, figuring out a way to make warnings show on small viewports. cc @jwold @johnwatkins0

[westonruter]

@johnwatkins0 One key aspect of this issue that is distinct from #4719 is the need to scan the theme for this bit:

There's one big caveat here for recommending Reader mode, and specifically a Reader theme. In particular, it may not be advisable if the active theme is violating a best practice of registering post types, taxonomies, post types, blocks, and widgets in plugins rather than in theme. If the active theme is registering a post type, for example, and a Reader theme is selected, then this post type will be absent entirely.

What we need to do is get a list of all registered blocks, widgets, post types, and taxonomies when the active theme is running. Then we need need to do it again with the active theme switched to an empty one. If there is a difference, then the missing entities were registered in the theme and will not be available in the new Reader mode. In such a mode, only Classic theme would be recommended. And we will need to list out what will not be available if a Reader theme is selected.

There needs to be a REST API endpoint that lists out these entities (post types, shortcodes, blocks, etc), along with a parameter for whether the active theme should be short-circuited. When that same request is made without the active theme short-circuited, then if there is a difference in the registered entities, then we know that the theme is responsible for registering something and we need to warn that those entities will not be available when using a Reader theme. Users will need to move the entity registration logic into a custom plugin, which is a best practice anyway.

[johnwatkins0]

Thanks for calling that out @westonruter. Yes, I think that's the main sizable new piece of the site scan work, and I'll make that the focus of this issue.

[westonruter]

There would be 4 conditions which would prevent a non-legacy Reader theme from being available to select:

1. Themes cannot be installed on the environment (either due to filesystem permissions or user capabilities).
2. The AMP query var was defined too late (after plugins_loaded action with priority 8, per Add checking for whether AMP query var is defined too late for Reader themes #4999 and Prevent non-legacy themes from being selected as Reader theme on Settings screen when AMP slug defined late #5789.)
3. A fatal error occurs when attempting to load an AMP page with a Reader theme selected (what we're discussing now).
4. The theme is registering custom post types, shortcodes, taxonomies, etc which would not be available when the Reader theme is loaded.

In all 4 3 cases, the same yellow warning box should be displayed.

Also, in the 3rd case we should try to detect the plugin that is causing the fatal error so that we can then mention to the user in the notice which plugin specifically is causing the fatal error. With that information in hand, we can advise that they have the option to suppress that plugin, which may allow them to then select the desired Reader theme.

We have the LikelyCulpritDetector service which can be used to analyze the error backtrace to determine the theme/plugin (which we use when amp_is_available() is called too early per #5289). We should be using this as well for the error screen shown when an exception happens during post-processing (#5320).

Communicating the error back to the Settings screen and/or Onboarding Wizard will be a bit tricky. We'll have to actually load the site with a Reader theme active (e.g. with some query var that forces a Reader theme to be loaded prior to updating the settings) and then pass back the error details. For the latter part, perhaps the error screen added in #5320 could be modified to return a JSON object when wp_is_json_request().

[jwold]

Notes from a recent team call:

At the end of the site scanning, we should have a message: "Please verify that everything is correct."

Site Scanning could then report findings in a user friendly way, and essentially do the Site Review for the user (deprecating #6071 when complete), and have a message, "Some parts of your site may or may not be available, please verify."

So for 2.2 we'll launch the Review Section, and for 2.3 (or later) we'll hook into site scanning to help users with the review. This could be done async, so users get a notification later, when the site scanning site review is ready.

[delawski]

@westonruter Do you think we can close it via #6610 just like the Plugin compatibility scanning issue (#4719)?

[westonruter]

@delawski Almost. I think the outstanding piece here is to incorporate #6516. Namely, we need to determine if the theme is registering any custom post types, taxonomies, shortcodes, blocks, etc. This is captured in this point above:

3. Gather list of theme-registered entities (post types, shortcodes, blocks, taxonomies, etc.) which are (erroneously) registered in the theme.

If so, we cannot rightly recommend recommend a Reader theme since doing so would cause the user to lose their custom functionality on AMP pages.