Add checking for whether AMP query var is defined too late for Reader themes

assets/src/setup/pages/choose-reader-theme/index.js

@@ -1,9 +1,14 @@
 /**
  * WordPress dependencies
  */
-import { __ } from '@wordpress/i18n';
+import { __, sprintf } from '@wordpress/i18n';
 import { useEffect, useContext, useMemo } from '@wordpress/element';
 
+/**
+ * External dependencies
+ */
+import { AMP_QUERY_VAR, DEFAULT_AMP_QUERY_VAR, LEGACY_THEME_SLUG, AMP_QUERY_VAR_CUSTOMIZED_LATE } from 'amp-setup'; // From WP inline script.
+
 /**
  * Internal dependencies
  */
@@ -32,18 +37,23 @@ export function ChooseReaderTheme() {
 			return;
 		}
 
-		if ( themes && readerTheme && canGoForward === false ) {
-			if ( themes.map( ( { slug } ) => slug ).includes( readerTheme ) ) {
-				setCanGoForward( true );
-			}
+		if (
+			themes &&
+			readerTheme &&
+			canGoForward === false &&
+			! AMP_QUERY_VAR_CUSTOMIZED_LATE
+				? themes.map( ( { slug } ) => slug ).includes( readerTheme )
+				: readerTheme === LEGACY_THEME_SLUG
+		) {
+			setCanGoForward( true );
 		}
 	}, [ canGoForward, setCanGoForward, readerTheme, themes, themeSupport ] );
 
 	// Separate available themes (both installed and installable) from those that need to be installed manually.
 	const { availableThemes, unavailableThemes } = useMemo(
 		() => themes.reduce(
 			( collections, theme ) => {
-				if ( theme.availability === 'non-installable' ) {
+				if ( ( AMP_QUERY_VAR_CUSTOMIZED_LATE && theme.slug !== LEGACY_THEME_SLUG ) || theme.availability === 'non-installable' ) {
 					collections.unavailableThemes.push( theme );
 				} else {
 					collections.availableThemes.push( theme );
@@ -96,7 +106,23 @@ export function ChooseReaderTheme() {
 							{ __( 'Unavailable themes', 'amp' ) }
 						</h3>
 						<p>
-							{ __( 'The following themes are compatible but cannot be installed automatically. Please install them manually, or contact your host if you are not able to do so.', 'amp' ) }
+							{ AMP_QUERY_VAR_CUSTOMIZED_LATE
+								/* dangerouslySetInnerHTML reason: Injection of code tags. */
+								? <span
+									dangerouslySetInnerHTML={ {
+										__html: sprintf(
+											/* translators: 1: customized AMP query var, 2: default query var, 3: the AMP_QUERY_VAR constant name, 4: the amp_query_var filter, 5: the plugins_loaded action */
+											__( 'The following themes are not available because your site (probably the active theme) has customized the AMP query var too late (it is set to %1$s as opposed to the default of %2$s). Please make sure that any customizations done by defining the %3$s constant or adding an %4$s filter are done before the %5$s action with priority 8.', 'amp' ),
+											`<code>${ AMP_QUERY_VAR }</code>`,
+											`<code>${ DEFAULT_AMP_QUERY_VAR }</code>`,
+											'<code>AMP_QUERY_VAR</code>',
+											'<code>amp_query_var</code>',
+											'<code>plugins_loaded</code>',
+										),
+									} }
+								/>
+								: __( 'The following themes are compatible but cannot be installed automatically. Please install them manually, or contact your host if you are not able to do so.', 'amp' )
+							}
 						</p>
 						<ul className="choose-reader-theme__grid">
 							{ unavailableThemes.map( ( theme ) => (

includes/amp-helper-functions.php

@@ -538,10 +538,6 @@ function amp_redirect_old_slug_to_new_url( $link ) {
  * @return string Slug used for query var, endpoint, and post type support.
  */
 function amp_get_slug() {
-	if ( defined( 'AMP_QUERY_VAR' ) ) {
-		return AMP_QUERY_VAR;
-	}
-
 	/**
 	 * Filter the AMP query variable.
 	 *
@@ -551,11 +547,7 @@ function amp_get_slug() {
 	 *
 	 * @param string $query_var The AMP query variable.
 	 */
-	$query_var = apply_filters( 'amp_query_var', QueryVars::AMP );
-
-	define( 'AMP_QUERY_VAR', $query_var );
-
-	return $query_var;
+	return apply_filters( 'amp_query_var', defined( 'AMP_QUERY_VAR' ) ? AMP_QUERY_VAR : QueryVars::AMP );
 }
 
 /**

includes/options/class-amp-options-menu.php

@@ -5,7 +5,10 @@
  * @package AMP
  */
 
+use AmpProject\AmpWP\AmpSlugCustomizationWatcher;
 use AmpProject\AmpWP\Option;
+use AmpProject\AmpWP\QueryVars;
+use AmpProject\AmpWP\Services;
 
 /**
  * AMP_Options_Menu class.
@@ -134,6 +137,9 @@ public function add_menu_items() {
 	public function render_theme_support() {
 		$theme_support = AMP_Options_Manager::get_option( Option::THEME_SUPPORT );
 
+		/** @var AmpSlugCustomizationWatcher $amp_slug_customization_watcher */
+		$amp_slug_customization_watcher = Services::get( 'amp_slug_customization_watcher' );
+
 		/* translators: %s: URL to the documentation. */
 		$standard_description = sprintf( __( 'The active theme integrates AMP as the framework for your site by using its templates and styles to render webpages. This means your site is <b>AMP-first</b> and your canonical URLs are AMP! Depending on your theme/plugins, a varying level of <a href="%s">development work</a> may be required.', 'amp' ), esc_url( 'https://amp-wp.org/documentation/developing-wordpress-amp-sites/' ) );
 		/* translators: %s: URL to the documentation. */
@@ -205,15 +211,51 @@ public function render_theme_support() {
 				<dd>
 					<p><?php echo wp_kses_post( $reader_description ); ?></p>
 
-					<p>
-						<?php
-						/* translators: %s is the reader theme */
-						echo esc_html( sprintf( __( 'The "%s" theme is currently selected in the wizard. The ability to change it from the admin screen will come soon.', 'amp' ), AMP_Options_Manager::get_option( Option::READER_THEME ) ) );
-						?>
-					</p>
+					<div class="notice notice-info notice-alt inline">
+						<p>
+							<?php
+							/* translators: %s is the reader theme */
+							echo esc_html( sprintf( __( 'The "%s" theme is currently selected in the wizard. The ability to change it from the admin screen will come soon.', 'amp' ), AMP_Options_Manager::get_option( Option::READER_THEME ) ) );
+							?>
+						</p>
+					</div>
 
 					<?php if ( AMP_Options_Manager::get_option( Option::READER_THEME ) === get_template() ) : ?>
-						<p><?php esc_html_e( 'The currently-selected Reader theme is the same as the active theme. This means that Reader mode is disabled since it is no different from Transitional mode at this point.', 'amp' ); ?></p>
+						<div class="notice notice-info notice-alt inline">
+							<p><?php esc_html_e( 'The currently-selected Reader theme is the same as the active theme. This means that Reader mode is disabled since it is no different from Transitional mode at this point.', 'amp' ); ?></p>
+						</div>
+					<?php endif; ?>
+
+					<?php if ( $amp_slug_customization_watcher->did_customize_late() ) : ?>
+						<div class="notice notice-warning notice-alt inline">
+							<p>
+								<?php
+								echo wp_kses_post(
+									sprintf(
+										/* translators: 1: customized AMP query var, 2: default AMP query var */
+										esc_html__( 'Warning: You customized your AMP query var too late. This means that Reader themes will not be available to you. You customized it to be %1$s instead of the default %2$s.', 'amp' ),
+										sprintf( '&#8220;<code>%s</code>&#8221;', esc_html( amp_get_slug() ) ),
+										sprintf( '&#8220;<code>%s</code>&#8221;', QueryVars::AMP )
+									)
+								);
+								?>
+							</p>
+						</div>
+					<?php elseif ( $amp_slug_customization_watcher->did_customize_early() ) : ?>
+						<div class="notice notice-info notice-alt inline">
+							<p>
+								<?php
+								echo wp_kses_post(
+									sprintf(
+										/* translators: 1: customized AMP query var, 2: default AMP query var */
+										esc_html__( 'You customized your query var to be %1$s instead of the default %2$s.', 'amp' ),
+										sprintf( '&#8220;<code>%s</code>&#8221;', esc_html( amp_get_slug() ) ),
+										sprintf( '&#8220;<code>%s</code>&#8221;', QueryVars::AMP )
+									)
+								);
+								?>
+							</p>
+						</div>
 					<?php endif; ?>
 				</dd>
 			</dl>

includes/options/views/class-amp-setup-wizard-submenu-page.php

@@ -7,6 +7,10 @@
  */
 
 use AmpProject\AmpWP\Admin\DevToolsUserAccess;
+use AmpProject\AmpWP\AmpSlugCustomizationWatcher;
+use AmpProject\AmpWP\Option;
+use AmpProject\AmpWP\QueryVars;
+use AmpProject\AmpWP\Services;
 
 /**
  * AMP setup wizard submenu page class.
@@ -104,6 +108,9 @@ public function enqueue_assets( $hook_suffix ) {
 			return;
 		}
 
+		/** @var AmpSlugCustomizationWatcher $amp_slug_customization_watcher */
+		$amp_slug_customization_watcher = Services::get( 'amp_slug_customization_watcher' );
+
 		$asset_file   = AMP__DIR__ . '/assets/js/' . self::ASSET_HANDLE . '.asset.php';
 		$asset        = require $asset_file;
 		$dependencies = $asset['dependencies'];
@@ -145,6 +152,9 @@ public function enqueue_assets( $hook_suffix ) {
 		$setup_wizard_data = [
 			'AMP_OPTIONS_KEY'                    => AMP_Options_Manager::OPTION_NAME,
 			'AMP_QUERY_VAR'                      => amp_get_slug(),
+			'DEFAULT_AMP_QUERY_VAR'              => QueryVars::AMP,
+			'AMP_QUERY_VAR_CUSTOMIZED_LATE'      => $amp_slug_customization_watcher->did_customize_late(),
+			'LEGACY_THEME_SLUG'                  => AMP_Reader_Themes::DEFAULT_READER_THEME,
 			'APP_ROOT_ID'                        => self::APP_ROOT_ID,
 			'CUSTOMIZER_LINK'                    => add_query_arg(
 				[

src/AmpSlugCustomizationWatcher.php

@@ -0,0 +1,96 @@
+<?php
+/**
+ * Class AmpSlugCustomizationWatcher.
+ *
+ * @package AmpProject\AmpWP
+ */
+
+namespace AmpProject\AmpWP;
+
+use AmpProject\AmpWP\Infrastructure\Registerable;
+use AmpProject\AmpWP\Infrastructure\Service;
+
+/**
+ * Service for redirecting mobile users to the AMP version of a page.
+ *
+ * @package AmpProject\AmpWP
+ */
+final class AmpSlugCustomizationWatcher implements Service, Registerable {
+
+	/**
+	 * Whether the slug was customized early (at plugins_loaded action, priority 8).
+	 *
+	 * @var bool
+	 */
+	protected $is_customized_early = false;
+
+	/**
+	 * Whether the slug was customized early (at after_setup_theme action, priority 4).
+	 *
+	 * @var bool
+	 */
+	protected $is_customized_late = false;
+
+	/**
+	 * Register.
+	 */
+	public function register() {
+		// This is at priority 8 because ReaderThemeLoader::override_theme runs at priority 9, which in turn runs right
+		// before _wp_customize_include at priority 10. A slug is customized early if it is customized at priority 8.
+		add_action( 'plugins_loaded', [ $this, 'determine_early_customization' ], 8 );
+	}
+
+	/**
+	 * Whether the slug was customized early (at plugins_loaded action, priority 8).
+	 *
+	 * @return bool
+	 */
+	public function did_customize_early() {
+		return $this->is_customized_early;
+	}
+
+	/**
+	 * Whether the slug was customized early (at after_setup_theme action, priority 4).
+	 *
+	 * @return bool
+	 */
+	public function did_customize_late() {
+		return $this->is_customized_late;
+	}
+
+	/**
+	 * Determine if the slug was customized early.
+	 *
+	 * Early customization happens by plugins_loaded action at priority 8; this is required in order for the slug to be
+	 * used by `ReaderThemeLoader::override_theme()` which runs at priority 9; this method in turn must run before
+	 * before `_wp_customize_include()` which runs at plugins_loaded priority 10. At that point the current theme gets
+	 * determined, so for Reader themes to apply the logic in `ReaderThemeLoader` must run beforehand.
+	 */
+	public function determine_early_customization() {
+		if ( QueryVars::AMP !== amp_get_slug() ) {
+			$this->is_customized_early = true;
+		} else {
+			add_action( 'after_setup_theme', [ $this, 'determine_late_customization' ], 4 );
+		}
+	}
+
+	/**
+	 * Determine if the slug was defined late.
+	 *
+	 * Late slug customization often happens when a theme itself defines `AMP_QUERY_VAR`. This is too late for the plugin
+	 * to be able to offer Reader themes which must have `AMP_QUERY_VAR` defined by plugins_loaded priority 9. Also,
+	 * defining `AMP_QUERY_VAR` is fundamentally incompatible since loading a Reader theme means preventing the original
+	 * theme from ever being loaded, and thus the theme's customized `AMP_QUERY_VAR` will never be read.
+	 *
+	 * This method must run before `amp_after_setup_theme()` which runs at the after_setup_theme action priority 5. In
+	 * this function, the `amp_get_slug()` function is called which will then set the query var for the remainder of the
+	 * request.
+	 *
+	 * @see amp_after_setup_theme()
+	 */
+	public function determine_late_customization() {
+		if ( QueryVars::AMP !== amp_get_slug() ) {
+			$this->is_customized_late = true;
+		}
+	}
+}

src/AmpWpPlugin.php

@@ -63,6 +63,7 @@ protected function get_service_classes() {
 			'plugin_suppression'               => PluginSuppression::class,
 			'mobile_redirection'               => MobileRedirection::class,
 			'reader_theme_loader'              => ReaderThemeLoader::class,
+			'amp_slug_customization_watcher'   => AmpSlugCustomizationWatcher::class,
 		];
 	}
 

src/ReaderThemeLoader.php

@@ -51,8 +51,6 @@ public static function is_needed() {
 			AMP_Theme_Support::READER_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT )
 			&&
 			AMP_Reader_Themes::DEFAULT_READER_THEME !== AMP_Options_Manager::get_option( Option::READER_THEME )
-			&&
-			isset( $_GET[ amp_get_slug() ] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended
 		);
 	}
 
@@ -85,6 +83,11 @@ public function get_reader_theme() {
 	 * @see WP_Customize_Manager::start_previewing_theme() which provides for much of the inspiration here.
 	 */
 	public function override_theme() {
+		// Short-circuit if the request does not include the AMP query var.
+		if ( ! isset( $_GET[ amp_get_slug() ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
+			return;
+		}
+
 		$theme = $this->get_reader_theme();
 		if ( ! $theme instanceof WP_Theme ) {
 			return;