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

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

Merged
merged 12 commits into from
Apr 28, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 1 addition & 24 deletions assets/src/components/reader-theme-carousel/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { AMP_QUERY_VAR, DEFAULT_AMP_QUERY_VAR, AMP_QUERY_VAR_CUSTOMIZED_LATE } from 'amp-settings'; // From WP inline script.

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -155,25 +150,7 @@ export function ReaderThemeCarousel() {
hasUnavailableThemes && (
<AMPNotice type={ NOTICE_TYPE_INFO }>
<p>
{ 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 */
__( 'Some of the themes below 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>',
),
} }
/>
)
: __( 'Some supported themes cannot be installed automatically on this site. To use, please install them manually or contact your hosting provider.', 'amp' )
}
{ __( 'Some supported themes cannot be installed automatically on this site. To use, please install them manually or contact your hosting provider.', 'amp' ) }
</p>
<AMPSettingToggle
text={ __( 'Show unavailable themes', 'amp' ) }
Expand Down
23 changes: 2 additions & 21 deletions assets/src/components/reader-theme-selection/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@
* External dependencies
*/
import PropTypes from 'prop-types';
import { AMP_QUERY_VAR, DEFAULT_AMP_QUERY_VAR, AMP_QUERY_VAR_CUSTOMIZED_LATE } from 'amp-settings'; // From WP inline script.

/**
* WordPress dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { __ } from '@wordpress/i18n';
import { useContext } from '@wordpress/element';

/**
Expand Down Expand Up @@ -56,25 +55,7 @@ export function ReaderThemeSelection() {
{ __( 'Unavailable themes', 'amp' ) }
</h3>
<p>
{ 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' )
}
{ __( '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
4 changes: 2 additions & 2 deletions assets/src/components/reader-themes-context-provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { __ } from '@wordpress/i18n';
* External dependencies
*/
import PropTypes from 'prop-types';
import { AMP_QUERY_VAR_CUSTOMIZED_LATE, USING_FALLBACK_READER_THEME, LEGACY_THEME_SLUG } from 'amp-settings';
import { USING_FALLBACK_READER_THEME, LEGACY_THEME_SLUG } from 'amp-settings';

/**
* Internal dependencies
Expand Down Expand Up @@ -282,7 +282,7 @@ export function ReaderThemesContextProvider( { wpAjaxUrl, children, currentTheme
const { availableThemes, unavailableThemes } = useMemo(
() => ( filteredThemes || [] ).reduce(
( collections, theme ) => {
if ( ( AMP_QUERY_VAR_CUSTOMIZED_LATE && theme.slug !== LEGACY_THEME_SLUG ) || theme.availability === 'non-installable' ) {
if ( theme.availability === 'non-installable' ) {
collections.unavailableThemes.push( theme );
} else {
collections.availableThemes.push( theme );
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { LEGACY_THEME_SLUG, AMP_QUERY_VAR_CUSTOMIZED_LATE } from 'amp-settings'; // From WP inline script.

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -35,9 +30,7 @@ export function ChooseReaderTheme() {
themes &&
readerTheme &&
canGoForward === false &&
! AMP_QUERY_VAR_CUSTOMIZED_LATE
? themes.map( ( { slug } ) => slug ).includes( readerTheme )
: readerTheme === LEGACY_THEME_SLUG
themes.map( ( { slug } ) => slug ).includes( readerTheme )
) {
setCanGoForward( true );
}
Expand Down
15 changes: 14 additions & 1 deletion includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

use AmpProject\AmpWP\Admin\ReaderThemes;
use AmpProject\AmpWP\AmpSlugCustomizationWatcher;
use AmpProject\AmpWP\AmpWpPluginFactory;
use AmpProject\AmpWP\Exception\InvalidService;
use AmpProject\AmpWP\Icon;
Expand Down Expand Up @@ -532,9 +533,21 @@ function _amp_bootstrap_customizer() {
*
* @since 0.7
*
* @param bool $ignore_late_defined_slug Whether to ignore the late defined slug.
westonruter marked this conversation as resolved.
Show resolved Hide resolved
* @return string Slug used for query var, endpoint, and post type support.
*/
function amp_get_slug() {
function amp_get_slug( $ignore_late_defined_slug = false ) {

// When a slug was defined late according to AmpSlugCustomizationWatcher, the slug will be stored in the
// LATE_DEFINED_SLUG option by the PairedRouting service so that it can be used early. This is only needed until
// the after_setup_theme action fires, because at that time the late-defined slug will have been established.
if ( ! $ignore_late_defined_slug && ! did_action( AmpSlugCustomizationWatcher::LATE_DETERMINATION_ACTION ) ) {
$slug = AMP_Options_Manager::get_option( Option::LATE_DEFINED_SLUG );
if ( ! empty( $slug ) && is_string( $slug ) ) {
return $slug;
}
}

/**
* Filter the AMP query variable.
*
Expand Down
2 changes: 0 additions & 2 deletions src/Admin/OnboardingWizardSubmenuPage.php
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,6 @@ 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' => QueryVar::AMP,
'AMP_QUERY_VAR_CUSTOMIZED_LATE' => $amp_slug_customization_watcher->did_customize_late(),
'LEGACY_THEME_SLUG' => ReaderThemes::DEFAULT_READER_THEME,
'APP_ROOT_ID' => self::APP_ROOT_ID,
'CUSTOMIZER_LINK' => add_query_arg(
Expand Down
22 changes: 10 additions & 12 deletions src/Admin/OptionsMenu.php
Original file line number Diff line number Diff line change
Expand Up @@ -214,28 +214,26 @@ public function enqueue_assets( $hook_suffix ) {
$is_reader_theme = $this->reader_themes->theme_data_exists( get_stylesheet() );

$js_data = [
'AMP_QUERY_VAR' => amp_get_slug(),
'DEFAULT_AMP_QUERY_VAR' => QueryVar::AMP,
'AMP_QUERY_VAR_CUSTOMIZED_LATE' => $amp_slug_customization_watcher->did_customize_late(),
'CURRENT_THEME' => [
'AMP_QUERY_VAR' => amp_get_slug(),
'CURRENT_THEME' => [
'name' => $theme->get( 'Name' ),
'description' => $theme->get( 'Description' ),
'is_reader_theme' => $is_reader_theme,
'screenshot' => $theme->get_screenshot() ?: null,
'url' => $theme->get( 'ThemeURI' ),
],
'OPTIONS_REST_PATH' => '/amp/v1/options',
'READER_THEMES_REST_PATH' => '/amp/v1/reader-themes',
'IS_CORE_THEME' => in_array(
'OPTIONS_REST_PATH' => '/amp/v1/options',
'READER_THEMES_REST_PATH' => '/amp/v1/reader-themes',
'IS_CORE_THEME' => in_array(
get_stylesheet(),
AMP_Core_Theme_Sanitizer::get_supported_themes(),
true
),
'LEGACY_THEME_SLUG' => ReaderThemes::DEFAULT_READER_THEME,
'USING_FALLBACK_READER_THEME' => $this->reader_themes->using_fallback_theme(),
'THEME_SUPPORT_ARGS' => AMP_Theme_Support::get_theme_support_args(),
'THEME_SUPPORTS_READER_MODE' => AMP_Theme_Support::supports_reader_mode(),
'UPDATES_NONCE' => wp_create_nonce( 'updates' ),
'LEGACY_THEME_SLUG' => ReaderThemes::DEFAULT_READER_THEME,
'USING_FALLBACK_READER_THEME' => $this->reader_themes->using_fallback_theme(),
'THEME_SUPPORT_ARGS' => AMP_Theme_Support::get_theme_support_args(),
'THEME_SUPPORTS_READER_MODE' => AMP_Theme_Support::supports_reader_mode(),
'UPDATES_NONCE' => wp_create_nonce( 'updates' ),
];

wp_add_inline_script(
Expand Down
13 changes: 10 additions & 3 deletions src/AmpSlugCustomizationWatcher.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@
*/
final class AmpSlugCustomizationWatcher implements Service, Registerable {

/**
* Action at which a slug can be considered late-defined, which is the ideal case.
*
* @var string
*/
const LATE_DETERMINATION_ACTION = 'after_setup_theme';

/**
* Whether the slug was customized early (at plugins_loaded action, priority 8).
*
Expand Down Expand Up @@ -68,10 +75,10 @@ public function did_customize_late() {
* determined, so for Reader themes to apply the logic in `ReaderThemeLoader` must run beforehand.
*/
public function determine_early_customization() {
if ( QueryVar::AMP !== amp_get_slug() ) {
if ( QueryVar::AMP !== amp_get_slug( true ) ) {
$this->is_customized_early = true;
} else {
add_action( 'after_setup_theme', [ $this, 'determine_late_customization' ], 4 );
add_action( self::LATE_DETERMINATION_ACTION, [ $this, 'determine_late_customization' ], 4 );
}
}

Expand All @@ -90,7 +97,7 @@ public function determine_early_customization() {
* @see amp_after_setup_theme()
*/
public function determine_late_customization() {
if ( QueryVar::AMP !== amp_get_slug() ) {
if ( QueryVar::AMP !== amp_get_slug( true ) ) {
$this->is_customized_late = true;
}
}
Expand Down
1 change: 1 addition & 0 deletions src/AmpWpPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ protected function get_arguments() {
*/
protected function get_shared_instances() {
return [
AmpSlugCustomizationWatcher::class,
PluginRegistry::class,
Instrumentation\StopWatch::class,
DevTools\CallbackReflection::class,
Expand Down
9 changes: 8 additions & 1 deletion src/Option.php
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,17 @@ interface Option {
/**
* The key of the option storing whether the setup wizard has been completed.
*
* @var boolean
* @var string
*/
const PLUGIN_CONFIGURED = 'plugin_configured';

/**
* Cached slug when it is defined late.
*
* @var string
*/
const LATE_DEFINED_SLUG = 'late_defined_slug';

/**
* Suppressed plugins
*
Expand Down
63 changes: 54 additions & 9 deletions src/PairedRouting.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ final class PairedRouting implements Service, Registerable {
*/
const CUSTOM_PAIRED_ENDPOINT_SOURCES = 'custom_paired_endpoint_sources';

/**
* Action which is triggered when the late-defined slug needs to be updated in options.
*
* @var string
*/
const ACTION_UPDATE_LATE_DEFINED_SLUG_OPTION = 'amp_update_late_defined_slug_option';

/**
* Paired URL service.
*
Expand All @@ -117,6 +124,13 @@ final class PairedRouting implements Service, Registerable {
*/
private $callback_reflection;

/**
* AMP slug customization watcher.
*
* @var AmpSlugCustomizationWatcher
*/
private $amp_slug_customization_watcher;

/**
* Plugin registry.
*
Expand Down Expand Up @@ -150,16 +164,18 @@ final class PairedRouting implements Service, Registerable {
/**
* PairedRouting constructor.
*
* @param Injector $injector Injector.
* @param CallbackReflection $callback_reflection Callback reflection.
* @param PluginRegistry $plugin_registry Plugin registry.
* @param PairedUrl $paired_url Paired URL service.
* @param Injector $injector Injector.
* @param CallbackReflection $callback_reflection Callback reflection.
* @param PluginRegistry $plugin_registry Plugin registry.
* @param PairedUrl $paired_url Paired URL service.
* @param AmpSlugCustomizationWatcher $amp_slug_customization_watcher AMP slug customization watcher.
*/
public function __construct( Injector $injector, CallbackReflection $callback_reflection, PluginRegistry $plugin_registry, PairedUrl $paired_url ) {
$this->injector = $injector;
$this->callback_reflection = $callback_reflection;
$this->plugin_registry = $plugin_registry;
$this->paired_url = $paired_url;
public function __construct( Injector $injector, CallbackReflection $callback_reflection, PluginRegistry $plugin_registry, PairedUrl $paired_url, AmpSlugCustomizationWatcher $amp_slug_customization_watcher ) {
$this->injector = $injector;
$this->callback_reflection = $callback_reflection;
$this->plugin_registry = $plugin_registry;
$this->paired_url = $paired_url;
$this->amp_slug_customization_watcher = $amp_slug_customization_watcher;
}

/**
Expand All @@ -172,6 +188,23 @@ public function register() {
add_filter( 'amp_default_options', [ $this, 'filter_default_options' ], 10, 2 );
add_filter( 'amp_options_updating', [ $this, 'sanitize_options' ], 10, 2 );

add_action(
self::ACTION_UPDATE_LATE_DEFINED_SLUG_OPTION,
function () {
AMP_Options_Manager::update_option( Option::LATE_DEFINED_SLUG, $this->get_late_defined_slug() );
}
);

add_action(
AmpSlugCustomizationWatcher::LATE_DETERMINATION_ACTION,
function () {
$late_defined_slug = $this->get_late_defined_slug();
if ( AMP_Options_Manager::get_option( Option::LATE_DEFINED_SLUG ) !== $late_defined_slug ) {
wp_schedule_single_event( time(), self::ACTION_UPDATE_LATE_DEFINED_SLUG_OPTION );
}
}
);

add_action( 'template_redirect', [ $this, 'redirect_extraneous_paired_endpoint' ], 9 );

// Priority 7 needed to run before PluginSuppression::initialize() at priority 8.
Expand Down Expand Up @@ -655,6 +688,15 @@ public function filter_default_options( $defaults, $options ) {
return $defaults;
}

/**
* Get the late defined slug, or null if it was not defined late.
*
* @return string|null Slug or null.
*/
public function get_late_defined_slug() {
return $this->amp_slug_customization_watcher->did_customize_late() ? amp_get_slug() : null;
}

/**
* Sanitize options.
*
Expand All @@ -665,6 +707,9 @@ public function filter_default_options( $defaults, $options ) {
* @return array Sanitized options.
*/
public function sanitize_options( $options, $new_options ) {
// Cache the AMP slug in options when it is defined late so that it can be referred to early at plugins_loaded.
$options[ Option::LATE_DEFINED_SLUG ] = $this->get_late_defined_slug();

if (
isset( $new_options[ Option::PAIRED_URL_STRUCTURE ] )
&&
Expand Down
Loading