-
Notifications
You must be signed in to change notification settings - Fork 383
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 theme support settings to admin screen; prevent serving dirty AMP #1199
Merged
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
5a81073
Prevent making AMP blocks available when AMP is not native
westonruter 92424fb
Remove menu-toggle in Twenty Fifteen if there is nothing to open
westonruter 555c079
Add options for enabling amp theme support and configuring validation…
westonruter 5bae33e
Remove dirty AMP support; rename Disabled to Classic
westonruter a5756d9
Add link to settings screen among plugin action links
westonruter 0068e79
Rename Theme Support to Template Mode
westonruter be0557b
Fix undefined index notice
westonruter b94a830
Merge branch 'url-handling' into add/admin-theme-support-options
westonruter 0c339d9
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter 9971e22
Omit displaying the amphtml link if there are known unaccepted valida…
westonruter 32a093e
Include HTML comment explaining that amphtml version is not available…
westonruter f864d67
Simplify how validation errors get auto-accepted based on user prefer…
westonruter d890b6c
Introduce "Flagged" status for validation errors which are Rejected b…
westonruter ec95351
Merge flagged status into Rejected and Accepted
westonruter 365d571
Prevent raising removed_unused_css_rules as error when tree-shaking i…
westonruter 4aac14b
Improve UI to reflect sanitization status when required
westonruter 5c6b486
Add AMP link to admin bar with indication of AMP status and re-valida…
westonruter 616dad7
Omit AMP admin bar menu item from admin
westonruter 421dc74
Add duplicate admin bar sub item link for sake of mobile
westonruter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,6 +96,7 @@ class AMP_Theme_Support { | |
* @since 0.7 | ||
*/ | ||
public static function init() { | ||
self::apply_options(); | ||
if ( ! current_theme_supports( 'amp' ) ) { | ||
return; | ||
} | ||
|
@@ -117,7 +118,7 @@ public static function init() { | |
$args = array_shift( $support ); | ||
if ( ! is_array( $args ) ) { | ||
trigger_error( esc_html__( 'Expected AMP theme support arg to be array.', 'amp' ) ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error | ||
} elseif ( count( array_diff( array_keys( $args ), array( 'template_dir', 'available_callback', 'comments_live_list' ) ) ) !== 0 ) { | ||
} elseif ( count( array_diff( array_keys( $args ), array( 'template_dir', 'available_callback', 'comments_live_list', '__added_via_option' ) ) ) !== 0 ) { | ||
trigger_error( esc_html__( 'Expected AMP theme support to only have template_dir and/or available_callback.', 'amp' ) ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error | ||
} | ||
} | ||
|
@@ -132,16 +133,36 @@ public static function init() { | |
add_action( 'wp', array( __CLASS__, 'finish_init' ), PHP_INT_MAX ); | ||
} | ||
|
||
/** | ||
* Apply options for whether theme support is enabled via admin and what sanitization is performed by default. | ||
* | ||
* @see AMP_Post_Type_Support::add_post_type_support() For where post type support is added, since it is irrespective of theme support. | ||
*/ | ||
public static function apply_options() { | ||
if ( ! current_theme_supports( 'amp' ) ) { | ||
$theme_support_option = AMP_Options_Manager::get_option( 'theme_support' ); | ||
if ( 'disabled' === $theme_support_option ) { | ||
return; | ||
} | ||
|
||
$args = array( | ||
'__added_via_option' => true, | ||
); | ||
if ( 'paired' === $theme_support_option ) { | ||
$args['template_dir'] = './'; | ||
} | ||
add_theme_support( 'amp', $args ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's nice how this can force theme support. |
||
} | ||
} | ||
|
||
/** | ||
* Finish initialization once query vars are set. | ||
* | ||
* @since 0.7 | ||
*/ | ||
public static function finish_init() { | ||
if ( ! is_amp_endpoint() ) { | ||
if ( self::is_paired_available() ) { | ||
amp_add_frontend_actions(); | ||
} | ||
amp_add_frontend_actions(); | ||
return; | ||
} | ||
|
||
|
@@ -1187,7 +1208,10 @@ public static function prepare_response( $response, $args = array() ) { | |
$dom_serialize_start = microtime( true ); | ||
self::ensure_required_markup( $dom ); | ||
|
||
if ( ! AMP_Validation_Manager::should_validate_response() && AMP_Validation_Manager::has_blocking_validation_errors() ) { | ||
$blocking_error_count = AMP_Validation_Manager::count_blocking_validation_errors(); | ||
if ( ! AMP_Validation_Manager::should_validate_response() && $blocking_error_count > 0 ) { | ||
|
||
// Note the canonical check will not currently ever be met because dirty AMP is not yet supported; all validation errors will forcibly be sanitized. | ||
if ( amp_is_canonical() ) { | ||
$dom->documentElement->removeAttribute( 'amp' ); | ||
|
||
|
@@ -1201,7 +1225,19 @@ public static function prepare_response( $response, $args = array() ) { | |
$head->appendChild( $script ); | ||
} | ||
} else { | ||
self::redirect_ampless_url( false ); | ||
$current_url = amp_get_current_url(); | ||
$ampless_url = amp_remove_endpoint( $current_url ); | ||
$ampless_url = add_query_arg( | ||
AMP_Validation_Manager::VALIDATION_ERRORS_QUERY_VAR, | ||
$blocking_error_count, | ||
$ampless_url | ||
); | ||
|
||
/* | ||
* Temporary redirect because AMP URL may return when blocking validation errors | ||
* occur or when a non-canonical AMP theme is used. | ||
*/ | ||
wp_safe_redirect( $ampless_url, 302 ); | ||
return esc_html__( 'Redirecting to non-AMP version.', 'amp' ); | ||
} | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think the methods
tally_content_requiring_amp_scripts()
andprint_dirty_amp_scripts()
could be removed, given that they're not used anymore? Maybe the comment below could link to them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're probably right. It's easy enough to look back in history to find the removal of these methods to resurrect them. 👍
Done in 5bae33e.