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

westonruter · 2020-07-08T00:18:13Z

Summary

This is a sub-PR as part #4984 and closely related to theme compatibility scanning (#4795).

Addresses this todo:

Being able to select a Reader theme is dependent on a site not customizing the AMP endpoint (query var). So if a site changes the query var from amp to something else like lite, then a warning needs to be displayed. Same goes for a theme registering a custom post type, etc.

It also makes this line mostly obsolete from the prototype PR #4644:

Being able to select a Reader theme is dependent on a site not customizing the AMP endpoint (query var). So if a site changes the query var from amp to something else like lite, then a warning is displayed instead of the theme list.

In other words, a site will be able to still customize their query var (as part of #2204) as long as they do it before the plugins_loaded action (at priority 8). If they don't, then they will see this warning at the moment in the settings screen:

And then, importantly, all non-legacy themes will no longer be available (with the explanation why provided), and the Next button is disabled if the previously-selected Reader theme is now no longer available:

In order to test the failure condition introduced in this PR, add a plugin that does the following:

add_action(
	'after_setup_theme',
	function () {
		define( 'AMP_QUERY_VAR', 'late' );
	},
	~PHP_INT_MAX
);

You could also just add a define( 'AMP_QUERY_VAR', 'late' ) to the active theme's functions.php.

By contrast, to customize the query var successfully with a Reader theme, use this plugin code (just defining the constant straight away in the plugin file):

define( 'AMP_QUERY_VAR', 'early' );

Todo

Disable the Next button when the selected theme is among those which are unavailable.
Add tests.

Checklist

My pull request is addressing an open issue (please create one otherwise).
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).

… themes

github-actions · 2020-07-08T00:21:37Z

Plugin builds for 7574646 are ready 🛎️!

Download development build
Download production build

johnwatkins0

This looks good and works in testing. I think we'd better merge this and the parent branch before my settings page branch is merged because this touches a couple of files that I've either gutted or moved in my branch. The React parts should work as is, though, and src/AmpSlugCustomizationWatcher.php looks right assuming @pierlon signs off as well.

pierlon · 2020-07-08T01:10:48Z

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

+/**
+ * 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.


External dependencies should be put before internal ones:

Fixed in f3f27ad

pierlon · 2020-07-08T01:31:18Z

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

+				? 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(


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

Select developer background (choice doesn't matter here I suppose)

Select Transitional template mode

Go back to previous page

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

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 😄.

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

Sounds good.

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

pierlon · 2020-07-08T02:08:00Z

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

@@ -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 )


Translator comment missing.

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:

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?

Addressed in 018d8d8

pierlon · 2020-07-08T02:14:12Z

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

+						<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 ) ) );


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.

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

pierlon · 2020-07-08T02:17:10Z

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


 					<?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>


Should we put warnings above informational notices?

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

pierlon · 2020-07-08T02:18:50Z

src/AmpSlugCustomizationWatcher.php

+	 *
+	 * @var string|null
+	 */
+	protected $customized_slug = null;


Redundant null default value.

Suggested change

protected $customized_slug = null;

protected $customized_slug;

Obsolete as of 52efa80

pierlon · 2020-07-08T04:01:39Z

Looks the the Travis check felt like to not run at all:

johnwatkins0 · 2020-07-08T04:04:58Z

Looks the the Travis check felt like to not run at all:

I have found this always to be the case on PRs against branches that are not develop.

westonruter · 2020-07-08T04:05:04Z

It doesn't run because the PR is not targeting one of these branches:

amp-wp/.travis.yml

Lines 28 to 31 in 06f6636

    
           branches: 
        
             only: 
        
               - develop 
        
               - /^\d+\.\d+$/

pierlon · 2020-07-08T04:07:12Z

Ah OK, noted.

westonruter · 2020-07-08T04:08:05Z

Maybe we should just remove that.

tests/php/src/Unit/AmpSlugCustomizationWatcherTest.php

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

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

d2c53e6

… themes

westonruter added this to the v1.6 milestone Jul 8, 2020

westonruter requested review from johnwatkins0 and pierlon July 8, 2020 00:18

googlebot added the cla: yes Signed the Google CLA label Jul 8, 2020

westonruter added 2 commits July 7, 2020 17:25

Disable Next button if selected Reader theme is unavailable

463bb86

Remove unneessary methods and member variables

52efa80

johnwatkins0 approved these changes Jul 8, 2020

View reviewed changes

pierlon suggested changes Jul 8, 2020

View reviewed changes

westonruter added 3 commits July 7, 2020 20:30

Add translators comment, add code tags to message

018d8d8

Fix order of dependencies

f3f27ad

Eliminate defining AMP_QUERY_VAR the first time amp_get_slug() is called

ee88672

westonruter requested a review from pierlon July 8, 2020 03:43

pierlon suggested changes Jul 8, 2020

View reviewed changes

tests/php/src/Unit/AmpSlugCustomizationWatcherTest.php Outdated Show resolved Hide resolved

Fix function casing issue

7574646

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

pierlon approved these changes Jul 8, 2020

View reviewed changes

westonruter merged commit 4c6c0c1 into add/reader-theme-loader Jul 8, 2020

westonruter deleted the add/reader-theme-late-query-var-checking branch July 8, 2020 04:19

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 18, 2020

This was referenced Jan 19, 2021

Wizard Step: Theme compatibility scanning #4795

Closed

Prevent non-legacy themes from being selected as Reader theme on Settings screen when AMP slug defined late #5789

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

westonruter commented Jul 8, 2020 •

edited

Loading

github-actions bot commented Jul 8, 2020 •

edited

Loading

johnwatkins0 left a comment

pierlon Jul 8, 2020

westonruter Jul 8, 2020

pierlon Jul 8, 2020

pierlon Jul 8, 2020

johnwatkins0 Jul 8, 2020

pierlon Jul 8, 2020

johnwatkins0 Jul 8, 2020

pierlon Jul 8, 2020

pierlon Jul 8, 2020 •

edited

Loading

westonruter Jul 8, 2020

westonruter Jul 8, 2020

pierlon Jul 8, 2020 •

edited

Loading

westonruter Jul 8, 2020

pierlon Jul 8, 2020 •

edited

Loading

westonruter Jul 8, 2020

pierlon Jul 8, 2020

westonruter Jul 8, 2020

pierlon commented Jul 8, 2020

johnwatkins0 commented Jul 8, 2020

westonruter commented Jul 8, 2020

pierlon commented Jul 8, 2020

westonruter commented Jul 8, 2020

	protected $customized_slug = null;
	protected $customized_slug;

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

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

Conversation

westonruter commented Jul 8, 2020 • edited Loading

Summary

Todo

Checklist

github-actions bot commented Jul 8, 2020 • edited Loading

johnwatkins0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon commented Jul 8, 2020

johnwatkins0 commented Jul 8, 2020

westonruter commented Jul 8, 2020

pierlon commented Jul 8, 2020

westonruter commented Jul 8, 2020

westonruter commented Jul 8, 2020 •

edited

Loading

github-actions bot commented Jul 8, 2020 •

edited

Loading

pierlon Jul 8, 2020 •

edited

Loading

pierlon Jul 8, 2020 •

edited

Loading

pierlon Jul 8, 2020 •

edited

Loading