Support late-defined slugs for paired URL structures and Reader themes

[westonruter]

Summary

I just found a problem related to defining the AMP query var late. While at first this was only a problem for Reader themes, I realize it also (naturally) applies to when you're using paired URL structures, especially the legacy Reader or path suffix which involve /amp/. So if you have a plugin that does this:

add_action( 'setup_theme', function () {
	define( 'AMP_QUERY_VAR', 'lite' );
} );

And you have the paired URL structure set to "Path suffix". Going to /2020/01/07/hello-world/lite/ results in a 404. This is because the routing logic in the PairedRouting service gets run way earlier at plugins_loaded, so as far as it knows the the path suffix is /amp/ at that point since the logic in setup_theme hasn't run yet.

This circles back to the discussion we had 5 days ago, captured in Joshua's notes: #5859 (comment)

The way to fix this and also to allow any Reader theme even when late-defined slug is used, is to store the late-defined slug in an option. This would cache it to allow it to be used at any point in the WP page lifecycle.

This PR implements this idea which was first explored in #6042 for caching the primary theme's support features in options so that they are available when a Reader theme is selected.

• Remove now-unnecessary late-defined slug checks for selecting a Reader theme (e.g. Disable non-legacy themes selection on Settings screen when AMP slug is defined late #5859).
• Add tests.

Site Health Check

When the AMP slug (query var) is customized, a new Site Health check is added which tells the user if it was defined early (good) or late (bad). If late, it is recommended that they change the definition to be earlier by putting in a plugin:

Defined Early	Defined Late

If the user has not customized the AMP slug or if the site is in Standard mode (where the AMP slug is irrelevant), then the test is omitted entirely.

This information is also now included in Site Health Info:

Rewrite Endpoint Removed

In 3420914 an add_rewrite_endpoint() call was removed which should have been removed as part of #5558. Paired URL Structures do not use rewrite endpoints now, so there is no need to add a rewrite endpoint nor to flush them when settings change.

Checklist

• My code is tested and passes existing tests.
• My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[codecov]

Codecov Report

Merging #6125 (92830a0) into develop (0627a15) will increase coverage by 0.02%.
The diff coverage is 65.38%.

❗ Current head 92830a0 differs from pull request most recent head 61c7c29. Consider uploading reports for the commit 61c7c29 to get more accurate results

@@              Coverage Diff              @@
##             develop    #6125      +/-   ##
=============================================
+ Coverage      75.08%   75.11%   +0.02%     
- Complexity      5846     5847       +1     
=============================================
  Files            234      234              
  Lines          17663    17667       +4     
=============================================
+ Hits           13263    13271       +8     
+ Misses          4400     4396       -4     

Flag	Coverage Δ	Complexity Δ
javascript	79.84% <ø> (ø)	0.00 <ø> (ø)
php	74.90% <65.38%> (+0.02%)	5847.00 <1.00> (+1.00)
unit	74.90% <65.38%> (+0.02%)	5847.00 <1.00> (+1.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
src/Admin/OnboardingWizardSubmenuPage.php	82.10% <ø> (-0.37%)	21.00 <0.00> (ø)
src/Admin/OptionsMenu.php	31.31% <0.00%> (+0.62%)	18.00 <0.00> (ø)
includes/amp-helper-functions.php	85.04% <100.00%> (+0.11%)	0.00 <0.00> (ø)
src/AmpSlugCustomizationWatcher.php	84.61% <100.00%> (+15.38%)	7.00 <0.00> (ø)
src/AmpWpPlugin.php	100.00% <100.00%> (ø)	9.00 <0.00> (ø)
src/PairedRouting.php	96.35% <100.00%> (+0.04%)	118.00 <1.00> (+1.00)

[github-actions]

Plugin builds for 3549c0a are ready 🛎️!

• Download development build
• Download production build

[westonruter]

To test this, I first installed 2.0.11 and emptied out the amp-options to start from scratch.

I activated a plugin which did this:

add_action( 'setup_theme', function () {
	define( 'AMP_QUERY_VAR', 'lite' );
} );

And I flushed my rewrite rules. Then I went through the onboarding wizard and saw the notice I could only select the legacy theme:

The /lite/ endpoint was successfully accessible to me in the legacy AMP Reader mode: https://wordpress-stable.lndo.site/2021/04/27/cover-image/lite/

Then I switched to the build for this PR via the QA Tester plugin. Upon doing so I was able to confirm:

1. The /lite/ URL should continue to work.
2. The amp-options should now include a late_defined_slug option which is set to lite.

16c16
<   'version' => '2.0.11',
---
>   'version' => '2.1.0-beta2-20210427T201055Z-8d1cbcc',
17a18
>   'paired_url_structure' => 'legacy_reader',
22a24,25
>   'primary_theme_support' => NULL,
>   'late_defined_slug' => 'lite',

3. No longer does that Onboarding Wizard screen for Reader themes restrict me to Legacy.

4. The default Paired URL Structure structure is “Legacy Reader”:

5. I can switch it to “Path suffix” and the URLs work as expected (e.g. /lite/ works on a WP page).

[westonruter]

There are two cases that are not currently accounted for:

1. If you upgrade to 2.1 via WP-CLI (or via uploading with FTP), then the upgrade routing will not be triggered until the user next accesses the admin. (This could also be an issue during automatic updates.) This can mean that the AMP URLs will stop working until they do so. Nevertheless, since 2.0.11 adds a rewrite endpoint for the AMP slug, then a 404 won't actually happen unless the rewrite rules are also flushed after upgrading, until the user access the admin again.
2. If you deactivate the plugin (or remove the theme code) that defines the AMP slug late (e.g. lite), then the cached late-defined slug won't be removed until the options are re-saved. This means that the /lite/ AMP URLs will continue to work, but the canonical pages will start using /amp/ in the amphtml links, which will start to 404. However, it is unlikely that a site will be changing their customized AMP slug after they had set it, so this doesn't seem like something we should worry too much about.

[westonruter]

There are two cases that are not currently accounted for:

I've added 61c7c29 as a prototype for how to address these cases. It checks to see if the option storing the late-defined slug is different than the current late-defined slug. If so, it schedules an immediate event to update the option. It's scheduling an event rather than updating the option right then and there in order to frontend writes which we worked to avoid in #3284.

However, it's true that scheduling a cron event does do a frontend write. So maybe this is no better.

[schlessera]

Minor nitpick.

[schlessera]

However, it's true that scheduling a cron event does do a frontend write. So maybe this is no better.

That is only true for sites which use out-of-the-box WP scheduling. Most high traffic sites will use a real cron process instead, which will run the DB update in a proper, separate background process.

[pierlon]

Apart from the suggestion @schlessera raised, this looks good 👍.

[westonruter]

That is only true for sites which use out-of-the-box WP scheduling. Most high traffic sites will use a real cron process instead, which will run the DB update in a proper, separate background process.

@schlessera But what about actually scheduling the task? Calling wp_schedule_single_event() will ultimately call _set_cron_array(), and that does update_option( 'cron', $cron ).

[westonruter]

@schlessera so this means there will be a frontend write to the cron option. Do we feel ok with that?

[schlessera]

If I understand correctly, this would only happen the first time after a slug change AND the slug change is done with bad timing, correct?

[westonruter]

It would happen if you, for example, had a theme that defined the AMP_QUERY_VAR constant. If you upgrade the AMP plugin and aren't logged in to the admin (as in the case of automatic updates or deploys via Git or FTP), then the next frontend request from an unauthenticated user would trigger this condition to schedule the event to update the option.

[westonruter]

I have a compromise. It seems we need to do a frontend DB write, either by scheduling a task to update the option or updating the AMP options directly. Nevertheless, having a late-defined AMP slug is not the ideal. So what we can do is add a notice that encourages the site owner to move slug to be defined in a plugin at top-level (before plugins_loaded). This can be communicated in a Site Health test and/or a notice that shows up on the AMP Settings screen.

[pierlon]

That sounds like a good compromise.

[westonruter]

OK, here we go:

Not Customized	Defined Early	Defined Late
(No test)

[pierlon]

Looks good, LGTM .

[pierlon]

QA Passed

Given a plugin with the following filter:

add_action( 'setup_theme', function () {
	define( 'AMP_QUERY_VAR', 'lite' );
} );

• I can select from the list of Reader themes in the Onboarding Wizard:

• I can select from the list of Reader themes on the Settings page:

• The slug is saved under the late_defined_slug key in the amp_options option:

wp option get amp-options --format=yaml

--
theme_support: reader
supported_post_types:
  - post
  - page
  - attachment
analytics:
  b231392c-3c3d-4f57-ab6e-6dc02687f992:
    type: ""
    config: '{}'
all_templates_supported: true
supported_templates:
  - is_singular
version: 2.1.0-RC1-20210428T232407Z-bc2fccc
reader_theme: legacy
paired_url_structure: path_suffix
plugin_configured: true
mobile_redirect: false
suppressed_plugins: [ ]
late_defined_slug: lite
primary_theme_support: null

• With the paired URL structure set to "Path suffix", accessing AMP pages does not result in a 404:

• The Site Health check warns about the slug being defined late:

• When the slug is defined before the plugins_loaded action occurs (at priority 4), a Site Health check is shown informing the user all is well:

[schlessera]

So what we can do is add a notice that encourages the site owner to move slug to be defined in a plugin at top-level (before plugins_loaded).

In that case, shouldn't they just be pointed to our settings screen and be able to set the slug there?

[westonruter]

In that case, shouldn't they just be pointed to our settings screen and be able to set the slug there?

We don't provide a UI to change the slug, at least not yet. This is not something that 80% of users require so I don't think a UI is warranted. I can only recall a few sites that have customized the slug.