Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
27 changes: 20 additions & 7 deletions assets/src/setup/pages/choose-reader-theme/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { __, sprintf } from '@wordpress/i18n';
import { useEffect, useContext, useMemo } from '@wordpress/element';

/**
Expand All @@ -13,6 +13,11 @@ import { Options } from '../../components/options-context-provider';
import { ReaderThemes } from '../../components/reader-themes-context-provider';
import { ThemeCard } from './theme-card';

/**
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External dependencies should be put before internal ones:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f3f27ad


/**
* Screen for choosing the Reader theme.
*/
Expand All @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An error occurred on this line while testing. To reproduce:

  1. Select developer background (choice doesn't matter here I suppose)
  2. Select Transitional template mode
  3. Go back to previous page

image


Looks like this a regression. Could we add a test to catch this, @johnwatkins0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for a suggested fix:

--- assets/src/setup/pages/choose-reader-theme/index.js	(revision 463bb866a789980a9437d8412767e9fd9f2e3096)
+++ assets/src/setup/pages/choose-reader-theme/index.js	(date 1594173727100)
@@ -51,18 +51,20 @@
 
 	// Separate available themes (both installed and installable) from those that need to be installed manually.
 	const { availableThemes, unavailableThemes } = useMemo(
-		() => themes.reduce(
-			( collections, theme ) => {
-				if ( ( AMP_QUERY_VAR_CUSTOMIZED_LATE && theme.slug !== LEGACY_THEME_SLUG ) || theme.availability === 'non-installable' ) {
-					collections.unavailableThemes.push( theme );
-				} else {
-					collections.availableThemes.push( theme );
-				}
+		() => themes
+			? themes.reduce(
+				( collections, theme ) => {
+					if ( ( AMP_QUERY_VAR_CUSTOMIZED_LATE && theme.slug !== LEGACY_THEME_SLUG ) || theme.availability === 'non-installable' ) {
+						collections.unavailableThemes.push( theme );
+					} else {
+						collections.availableThemes.push( theme );
+					}
 
-				return collections;
-			},
-			{ availableThemes: [], unavailableThemes: [] },
-		),
+					return collections;
+				},
+				{ availableThemes: [], unavailableThemes: [] },
+			)
+			: [],
 		[ themes ] );
 
 	if ( fetchingThemes ) {

@johnwatkins0 If you're able to identify a simpler fix that would be great 😄.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I caught this and fixed it on my branch. I would disregard it on this branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I do (themes || []).reduce, although I don't feel great about it.

( 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,10 @@ 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
? sprintf( __( '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 AMP_QUERY_VAR constant or adding an amp_query_var filter are done before the `plugins_loaded` action with priority 8.', 'amp' ), AMP_QUERY_VAR, DEFAULT_AMP_QUERY_VAR )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Translator comment missing.

Copy link
Contributor

@pierlon pierlon Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer surrounding technical jargon with the <code> tag (such as AMP_QUERY_VAR and plugins_loaded)?


The backticks look really weird in my browser:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I meant to remove the backticks because I saw the same weirdness.

I wasn't sure how best to add code in this case. I suppose it requires some dangerouslySetInnerHTML?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 018d8d8

: __( '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
7 changes: 5 additions & 2 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -535,9 +535,10 @@ function amp_redirect_old_slug_to_new_url( $link ) {
*
* @since 0.7
*
* @param bool $define_constant Whether store the query var in the AMP_QUERY_VAR constant.
* @return string Slug used for query var, endpoint, and post type support.
*/
function amp_get_slug() {
function amp_get_slug( $define_constant = true ) {
if ( defined( 'AMP_QUERY_VAR' ) ) {
return AMP_QUERY_VAR;
}
Expand All @@ -553,7 +554,9 @@ function amp_get_slug() {
*/
$query_var = apply_filters( 'amp_query_var', QueryVars::AMP );

define( 'AMP_QUERY_VAR', $query_var );
if ( $define_constant ) {
define( 'AMP_QUERY_VAR', $query_var );
}

return $query_var;
}
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 ) ) );
Copy link
Contributor

@pierlon pierlon Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should show the theme name here instead of its slug 🤔. I doubt the typical user would be familiar with the slugs for the installed themes, and in some cases the theme slug might not easily identify the theme in question.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This is just temporary since all of this will be eliminated with the work that @johnwatkins0 is doing.

?>
</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>
Copy link
Contributor

@pierlon pierlon Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we put warnings above informational notices?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This is just temporary since all of this will be eliminated with the work that @johnwatkins0 is doing.

</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_slug_customization_watcher->get_customized_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_slug_customization_watcher->get_customized_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
139 changes: 139 additions & 0 deletions src/AmpSlugCustomizationWatcher.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
<?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 {

/**
* The slug (query var) that was customized.
*
* @var string|null
*/
protected $customized_slug = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant null default value.

Suggested change
protected $customized_slug = null;
protected $customized_slug;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obsolete as of 52efa80


/**
* 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;
}

/**
* Whether the slug was customized.
*
* @return bool
*/
public function did_customize_slug() {
return null !== $this->customized_slug;
}

/**
* Get the customized slug.
*
* Returns null if it was not customized.
*
* @return string|null
*/
public function get_customized_slug() {
return $this->customized_slug;
}

/**
* Peek at the customized slug.
*
* This does not call amp_get_slug(true) because if it did then it would end up defining AMP_QUERY_VAR and destroy
* the ability for us to determine when exactly a theme or plugin is customizing it.
*
* @return string Query var.
*/
public function peek_customized_slug() {
return amp_get_slug( false );
}

/**
* 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() {
$slug = $this->peek_customized_slug();
if ( QueryVars::AMP !== $slug ) {
$this->customized_slug = $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() {
$slug = $this->peek_customized_slug();
if ( QueryVars::AMP !== $slug ) {
$this->customized_slug = $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
11 changes: 9 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,15 @@ public function get_reader_theme() {
* @see WP_Customize_Manager::start_previewing_theme() which provides for much of the inspiration here.
*/
public function override_theme() {
// The false parameter is passed to prevent persisting the query var as the AMP_QUERY_VAR constant. This
// prevents interference with AmpSlugCustomizationWatcher.
$query_var = amp_get_slug( false );

// Short-circuit if the request does not include the AMP query var.
if ( ! isset( $_GET[ $query_var ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
return;
}

$theme = $this->get_reader_theme();
if ( ! $theme instanceof WP_Theme ) {
return;
Expand Down