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

Prevent infinite recursion error when active theme is set to be same as reader theme #5899

Merged
merged 30 commits into from
Mar 10, 2021

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Feb 19, 2021

Summary

In #4984 via 093b40c & 0064cbc the AMP plugin would dynamically set the template mode from reader to transitional when the active theme has been changed to be the same as the selected reader theme. This switch is problematic because it involves a check for whether the request has an AMP endpoint, and as noted in a4f2ed4:

// @todo Beware infinite recursion, particularly when deciding among custom endpoints!

Well, I just ran across this issue. I had set my Reader theme to be Twenty Twenty-One and then I switched my active theme to also be Twenty Twenty-One via wp theme activate twentytwentyone. Upon doing so, WP would error out with:

PHP Fatal error:  Uncaught AmpProject\AmpWP\Exception\InvalidService: The service ID "paired_routing" is not recognized and cannot be retrieved. in /app/public/content/plugins/amp/src/Exception/InvalidService.php:57
Stack trace:
#0 /app/public/content/plugins/amp/src/Infrastructure/ServiceContainer/SimpleServiceContainer.php(39): AmpProject\AmpWP\Exception\InvalidService::from_service_id('paired_routing')
#1 /app/public/content/plugins/amp/src/Services.php(55): AmpProject\AmpWP\Infrastructure\ServiceContainer\SimpleServiceContainer->get('paired_routing')
#2 /app/public/content/plugins/amp/includes/amp-helper-functions.php(1849): AmpProject\AmpWP\Services::get('paired_routing')
#3 /app/public/content/plugins/amp/includes/options/class-amp-options-manager.php(176): amp_has_paired_endpoint()
#4 /app/public/content/plugins/amp/includes/options/class-amp-options-manager.php(299): AMP_Options_Manager::get_options()
#5 /app/public/content/plugins/amp/src/Admin/PairedBrowsing.php(60): AMP_Options_Manager::get_option('theme_support')
# in /app/public/content/plugins/amp/src/Exception/InvalidService.php on line 57

If the issue with the undefined service is resolved by moving the paired_routing definition up higher in \AmpProject\AmpWP\AmpWpPlugin::SERVICES then what happens is infinite recursion where WP_Options_Manager::get_options() calls PairedRouting::get_paired_url_structure() which then calls WP_Options_Manager::get_options() and so on.

Instead of dynamically changing the template mode from reader to transitional in the options, what is needed instead is to update the UI on the settings screen to set the template mode to Transitional upon landing on the screen. This was similarly done before if a user had selected a Reader theme, but when they went back to the settings screen that theme was no longer installed and the theme had to be switched back to Legacy (see #5170). The same settings screen switch which was done for the unavailable Reader theme needs to also be done for the template mode when the selected Reader theme is the same as the active theme.

When the active theme is the same as the selected Reader theme and Reader mode is selected:

  1. AMP pages on the frontend should behave the same as if Transitional mode is active.
  2. Paired browsing should be available.
  3. Accessing the settings screen and the onboarding wizard should show "Transitional" as selected instead of "Reader".
  4. The themes admin screen should only show the active theme as being selected, without any theme showing as being the reader theme.
  5. The meta generator tag should show Transitional mode as being active as opposed to Reader mode. Fixes Generator meta tag unexpectedly indicates reader mode when transitional mode applied #5093.

Checklist

  • 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).

@westonruter westonruter added this to the v2.1 milestone Feb 19, 2021
@westonruter westonruter added Bug Something isn't working WS:Core Work stream for Plugin core labels Feb 19, 2021
@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #5899 (8e9a94d) into develop (fabe49d) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #5899      +/-   ##
=============================================
- Coverage      75.32%   75.28%   -0.04%     
- Complexity      5715     5722       +7     
=============================================
  Files            214      214              
  Lines          17261    17283      +22     
=============================================
+ Hits           13001    13012      +11     
- Misses          4260     4271      +11     
Flag Coverage Δ Complexity Δ
javascript 79.44% <ø> (ø) 0.00 <ø> (ø)
php 75.12% <100.00%> (-0.04%) 0.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...ssets/src/components/template-mode-option/index.js 83.33% <ø> (ø) 0.00 <0.00> (ø)
includes/amp-helper-functions.php 84.54% <100.00%> (+0.11%) 0.00 <0.00> (ø)
includes/options/class-amp-options-manager.php 92.82% <100.00%> (-0.15%) 75.00 <0.00> (-3.00)
src/Admin/PairedBrowsing.php 100.00% <100.00%> (ø) 29.00 <0.00> (+2.00)
src/ReaderThemeLoader.php 90.74% <100.00%> (ø) 30.00 <0.00> (ø)
src/MobileRedirection.php 82.85% <0.00%> (-7.43%) 74.00% <0.00%> (ø%)
includes/amp-post-template-functions.php 97.22% <0.00%> (ø) 0.00% <0.00%> (ø%)
includes/sanitizers/class-amp-video-sanitizer.php 99.15% <0.00%> (+0.02%) 58.00% <0.00%> (+2.00%)
includes/embeds/class-amp-core-block-handler.php 90.97% <0.00%> (+3.04%) 51.00% <0.00%> (+5.00%)

@westonruter westonruter marked this pull request as ready for review February 19, 2021 01:53
@westonruter westonruter added the RME Reader Mode Expansion label Feb 19, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 19, 2021

Plugin builds for 9baed99 are ready 🛎️!

…ansitional when reader theme is same as active theme
…r-theme-recursion-edge-case

* 'develop' of github.com:ampproject/amp-wp:
  Increase default Jest timeout to 30s
  Test that cron services are conditional
  Add comment for needed change to should_schedule_event
  Put SavePostValidationEvent and URLValidationCron behind a feature flag filter
@@ -159,7 +159,7 @@ export function ReaderThemeCarousel() {
{
sprintf(
/* translators: placeholder is the name of a WordPress theme. */
__( 'Your active theme “%s” is not available as a reader theme. If you wish to use it, Transitional mode may be the best option for you.', 'amp' ),
__( 'Your active theme “%s” is not listed below because it is AMP-compatible. If you wish to use it, Transitional mode is what you should choose.', 'amp' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

The copy in assets/src/components/reader-theme-selection/index.js would have to be updated as well:

__( 'Your active theme “%s” is not available as a reader theme. If you wish to use it, Transitional mode may be the best option for you.', 'amp' ),

We should extract the relevant duplicate components from both files into its own component to prevent the onboarding wizard and settings page from being out of sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I'm wondering if we should have a separate and more prominent notice for this, and leave the one for reader theme as is. Otherwise, it may be confusing for users to see that Transitional mode is being pre-selected without their manual consent. An example of such a notice:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

The ReaderThemesContextProvider for the Onboarding Wizard screen is configured to not filter the currently active theme when displaying the list of reader themes to choose from. That conflicts with the notice that is being shown, however:

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.

Should this be addressed as part of @delawski is doing in #5859? Or @delawski would you amend this PR with the feedback? You're more familiar with that codebase at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've extracted the common strings into a new component in 7e66e9f:

Settings Screen Onboarding Wizard
image 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.

The ReaderThemesContextProvider for the Onboarding Wizard screen is configured to not filter the currently active theme when displaying the list of reader themes to choose from.

Oh yeah, right. I remember we intentionally included the Reader mode-eligible active theme in the list of all Reader themes and then automatically switched to Transitional mode on the summary screen if they select the same one. But now this seems unnecessary. Perhaps we should align the behavior with what is on the settings screen: don't include the active theme in the list.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've hidden the active theme in the lit of Reader themes in the Onboarding Wizard in 335d306. I don't feel strongly that this is the right way to go, however. Maybe we were thinking in the Onboarding Wizard that it was better to not force the user to make that decision up-front. If we do indeed eliminate showing the active theme among the reader themes, then this is dead code:

const { readerModeWasOverridden } = useContext( Options );
return (
<>
{ readerModeWasOverridden && (
<AMPNotice type={ NOTICE_TYPE_INFO } size={ NOTICE_SIZE_LARGE }>
{ __( 'Because you selected a Reader theme that is the same as your site\'s active theme, your site has automatically been switched to Transitional template mode.', 'amp' ) }
</AMPNotice>
) }

Copy link
Member Author

Choose a reason for hiding this comment

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

Scratch that. I've gone back to showing all themes in the Onboarding Wizard, but I've removed the notice from being displayed there.

@westonruter
Copy link
Member Author

What's left is to ensure that this notice on the Onboarding Wizard summary screen:

image

Is also displayed after the “Template Mode” heading on the settings screen:

image

@westonruter
Copy link
Member Author

OK, now a notice appears in the “Template Mode” section and the “Customize Reader Theme” button his hidden when the active theme has been switched to be the same as the Reader theme:

image

The implementation is not ideal because there remains a flash of unconfigured content when loading:

flash-of-unconfigured-content.mov

This isn't the end of the world since this is going to be a very rare occurrence, but one of you'd surely implement it better to avoid this.

@westonruter
Copy link
Member Author

westonruter commented Mar 2, 2021

Oh no, actually the Reader themes are already being preloaded:

protected function add_preload_rest_paths() {
$paths = [
'/amp/v1/options',
'/amp/v1/reader-themes',
'/wp/v2/settings',
];
foreach ( $paths as $path ) {
$this->rest_preloader->add_preloaded_path( $path );

I'm curious why there is a delay.

@delawski
Copy link
Collaborator

delawski commented Mar 2, 2021

I'm curious why there is a delay.

As far as I understand, it's because the themes are requested only if they are really needed, i.e. when the theme_support is set to reader.

/**
* Fetches theme data when needed.
*/
useEffect( () => {
if ( error || fetchingThemes || ! readerThemesRestPath || themes ) {
return;
}
if ( READER !== themeSupport ) {
return;
}

Those requests are triggered sequentially. First, the options need to be fetched. Once it's done and the theme_mode is set to reader, the themes are requested. Even if it happens momentarily (thanks to the preload middleware), the DOM tree gets already rendered with the initial set of props. Right after that, the state gets updated with the themes causing a re-render. That's when we see the flash of unconfigured content.

In my first attempt, the entire "Template Mode" section rendering was delayed so that the state could become stable. However, it caused the next sections to jump around (as noted in the previous comment).

Another way would be to wait with the rendering of the entire settings page until the state is stable, but I think it could impact the UX.

The only other approach I could think of right now would be to somehow merge the two context providers (OptionsContextProvider and ReaderThemesContextProvider) and request both the options and the themes at the same time. This would actually be almost the same as the previous approach (halt rendering of the entire settings page until we have both requests back). But I guess this time besides the UX implications, it would make the developer's experience worse, too.

@westonruter Do you have something else on your mind? Maybe something we could do on the backend like getting the overridden set of options from the very beginning so that we don't have to update the state on the frontend before it even gets rendered?

…r-theme-recursion-edge-case

* 'develop' of github.com:ampproject/amp-wp: (153 commits)
  Update test snapshots
  Bump @wordpress/plugins from 2.24.5 to 2.24.6
  Add explanation for what sanitization entails
  Fix tests in WP 4.9
  Remove extraneous sprintf()
  Set placeholder attribute when we are sure we will match
  Use xpath query to obtain the placeholder
  Combine conditions
  Remove extraneous sprintf()
  Add test for tumblr in embed block
  Test removal of Tumblr script when wpautop was not involved
  Make use of MarkupComparison
  Make sure that Tumblr embed has a link to use as a placeholder
  Opt to combine add_twentytwentyone_overflow_button_fix into amend_twentytwentyone_styles
  Make sure TT1 stylesheet is enqueued before adding inline style
  Add style rule to place overflow button in bottom left corner of AMP component
  Set button type
  Allow hyphens in Server-Timing keys
  Fix indentation
  Apply style rules for AMP iframe resize button via core sanitizer
  ...
@westonruter
Copy link
Member Author

westonruter commented Mar 5, 2021

@delawski Thanks for your detailed explanation. I tested out the latest changes and the template modes section is OK, but it turns out to show the loading placeholder even in the normal reader state when the template mode is not overridden:

template-mode-not-overridden.mov

I thought that this would only be the case when an override is needed, but I see that it's needed now because we don't know if an override will be needed or not yet.

In the case of when an override is needed, there is something else that is not ideal: the “Customize Reader Theme” momentarily appears and then gets hidden when the override happens at which time the notice is also displayed:

template-mode-overridden.mov

Another way would be to wait with the rendering of the entire settings page until the state is stable, but I think it could impact the UX.

Instead of having the loading state/skeleton maybe it would indeed be then preferable to hide the entire UI (showing a spinner) until all of the data has been loaded, as you suggested? Since the data is already preloaded, it means the UI would only be held from rendering for another fraction of a second. This is much preferable to there being layout shifting.

@westonruter
Copy link
Member Author

This seems good to do even if we opt to set the overridden state in the options REST API response, since at present the initial render does not show the name of the selected Reader theme:

reader-theme-name-not-shown-on-first-render.mov

See how “Twenty Twenty-One” shows up a split second after the initial render. By hiding the UI until all data loads, we can avoid this and avoid showing the non-overridden state. We could also look at moving the override logic to the REST API, but still preventing rendering until all the state has been populated should be done to prevent the need to do any re-rendering.

@delawski
Copy link
Collaborator

delawski commented Mar 5, 2021

@westonruter I'm now leaning towards your approach, i.e. wait with the rendering until we have a stable state ready instead of showing skeleton components. This use case doesn't really require skeletons as we have the data already prefetched on the page. So the loading state shows up for a split of a second anyway.

I've revered some of my previous changes and pushed the updated logic that makes the rendering wait until the state is stable.

…r-theme-recursion-edge-case

* 'develop' of github.com:ampproject/amp-wp:
  Make attribute assertion more specific
  Utilize wp_print_inline_script_tag() to print mobile redirection script
@westonruter
Copy link
Member Author

This looks great! The UI feels much more stable now:

reader-theme-same-as-active-theme.mov
reader-theme-different-from-active-theme.mov

Copy link
Member Author

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

This is working great for me.

@pierlon Any changes you'd like to see or do you approve as well?

@@ -135,7 +136,7 @@ function Root( { appRoot } ) {
};
}, [ fetchingOptions ] );

if ( false !== fetchingOptions ) {
if ( false !== fetchingOptions || null === templateModeWasOverridden ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of templateModeWasOverridden maybe it would make more sense to use fetchingThemes here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess not since that's not sufficient to prevent the re-render.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not just check if fetchingOptions is truthy?

Suggested change
if ( false !== fetchingOptions || null === templateModeWasOverridden ) {
if ( fetchingOptions || null === templateModeWasOverridden ) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

We want to show the loading spinner not only when the options are being fetched (true), but also initially, when the state is not "stable" yet (null).

Although now with the second condition this case is covered via templateModeWasOverriden, I'd still keep the explicit false !== fetchingOptions in here.

@pierlon
Copy link
Contributor

pierlon commented Mar 10, 2021

Investigating why the PHP 7.1 test job is failing. It's also occurring on the develop branch so it's not due to any changes made in the PR.

@pierlon
Copy link
Contributor

pierlon commented Mar 10, 2021

So the cause of the errors are due to the EXIF data for test-image.jpg (sourced from WordPress test data) failing to be correctly processed. It has something to do with that image being encoded in motorola byte order (big-endian), and is why the other images used in that test produced no errors. These errors are only popping up now because Core is now only suppressing the errors for its test suite (see WordPress/wordpress-develop@26fb7c9).

As for why it's only occurring on PHP 7.1, it seems the issue of reading EXIF data for motorola byte order encoded images was patched in PHP 7.2, but was not backported to 7.1 possibly due to the large set of changes to be made as alluded to by one of the release managers for PHP 7.0 in this bug report:

[2017-07-22 21:06 UTC] [email protected]
I must admit, I don't think it will find its way to lower branches as its a part of a larger changeset to the exif extension in 7.2+ I'm afraid :(

So to resolve this, I think all that's needed is to use a different image to test against and avoid test-image.jpg.

@westonruter
Copy link
Member Author

westonruter commented Mar 10, 2021

Excellent sleuthing. I'm gonna try swapping with test-image-large.jpg other images.

@westonruter westonruter merged commit 978ca8a into develop Mar 10, 2021
@westonruter westonruter deleted the fix/reader-theme-recursion-edge-case branch March 10, 2021 20:17
westonruter added a commit that referenced this pull request Mar 10, 2021
@westonruter
Copy link
Member Author

I've cherry-picked 9baed99 onto the 2.0 branch.

@pierlon
Copy link
Contributor

pierlon commented Apr 26, 2021

QA Passed

With the Reader theme set to Twenty-Twenty One, I made the active theme also Twenty-Twenty Once via the command wp theme activate twentytwentyone which resulted in no errors:

Success: Switched to 'Twenty Twenty-One' theme.

On the themes admin screen, Twenty-Twenty One is shown as the active theme, and no reader theme is shown:

image

On the Settings page, the template mode is set to Transitional by default, and a notice is shown informing the user of the switch from Reader mode:

image

On the frontend, transitional mode can be verified by taking a look at the meta generator tag:

<meta name="generator" content="AMP Plugin v2.1.0-beta2; mode=transitional">

Paired Browsing is also available:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working RME Reader Mode Expansion WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generator meta tag unexpectedly indicates reader mode when transitional mode applied
4 participants