Skip to content

Commit

Permalink
Merge pull request #4999 from ampproject/add/reader-theme-late-query-…
Browse files Browse the repository at this point in the history
…var-checking

Add checking for whether AMP query var is defined too late for Reader themes
  • Loading branch information
westonruter authored Jul 8, 2020
2 parents fa73af5 + 7574646 commit 4c6c0c1
Show file tree
Hide file tree
Showing 8 changed files with 282 additions and 25 deletions.
40 changes: 33 additions & 7 deletions assets/src/setup/pages/choose-reader-theme/index.js
Original file line number Diff line number Diff line change
@@ -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
*/
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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 ) => (
Expand Down
10 changes: 1 addition & 9 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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 );
}

/**
Expand Down
56 changes: 49 additions & 7 deletions includes/options/class-amp-options-menu.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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>
Expand Down
10 changes: 10 additions & 0 deletions includes/options/views/class-amp-setup-wizard-submenu-page.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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'];
Expand Down Expand Up @@ -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(
[
Expand Down
96 changes: 96 additions & 0 deletions src/AmpSlugCustomizationWatcher.php
Original file line number Diff line number Diff line change
@@ -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;
}
}
}
1 change: 1 addition & 0 deletions src/AmpWpPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
];
}

Expand Down
7 changes: 5 additions & 2 deletions src/ReaderThemeLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
}

Expand Down Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 4c6c0c1

Please sign in to comment.