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

Use template_dir consistently as signal for Transitional mode #3286

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

westonruter
Copy link
Member

An issue reported by @archon810 was that if if a theme contains a theme support declaration like the following:

add_theme_support( 'amp', [
	'template_dir' => 'amp',
] );

The theme support option in the admin will default to Standard when first activating the plugin, as opposed to the Transitional mode, which is what is implied by supplying template_dir without paired. This is shown in amp_is_canonical():

amp-wp/amp.php

Lines 600 to 601 in 93276f7

// If there is a template_dir, then transitional mode is implied.
return empty( $args['template_dir'] );

The issue is that this same logic is not being used AMP_Theme_Support::read_theme_support():

$is_paired = ! empty( $args[ self::PAIRED_FLAG ] );

This is something that was likely missed in #2550.

So this PR ensures that AMP_Theme_Support::read_theme_support() results in the same theme support being derived as is indicated by amp_is_canonical().

To test this PR:

  1. Add the above add_theme_support() to a theme's functions.php
  2. Run wp option delete amp-options.
  3. Load the AMP settings screen.
  4. You should see Transitional as the selected mode as opposed to Standard.

@westonruter westonruter added this to the v1.3 milestone Sep 18, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Sep 18, 2019
@westonruter westonruter changed the title Use template_dir consistently as signal for paired mode Use template_dir consistently as signal for Transitional mode Sep 18, 2019
@westonruter westonruter added the Bug Something isn't working label Sep 18, 2019
@MackenzieHartung MackenzieHartung modified the milestones: v1.3, v1.3.1 Sep 18, 2019
Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Approved

Hi @westonruter,
This looks good.

Like you mentioned in your testing instructions, 'Transitional' now appears by default in 'AMP Settings' if the theme calls amp_theme_support( 'amp' ) with a template_dir.

Before

before-standard-displays

After

after-transitional

@@ -269,6 +269,95 @@ public function test_get_and_set_options() {
}
}


public function get_test_get_options_defaults_data() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great testing of this.

@westonruter westonruter merged commit 08c24be into develop Sep 18, 2019
@westonruter westonruter deleted the fix/default-template-mode branch September 18, 2019 18:14
@westonruter
Copy link
Member Author

Moved directly to Ready for Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants