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

Add debugging query arguments #3448

Closed
wants to merge 34 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
37de1c3
Add debugging query arguments
kienstra Oct 4, 2019
e738bcf
Use the filter 'amp_validation_error_sanitized' instead of another co…
kienstra Oct 4, 2019
19363fc
Add a separate filter for 'amp_validation_error_sanitized', add top-l…
kienstra Oct 4, 2019
aeeda26
Merge branch 'develop' into add/query-args-debugging
kienstra Oct 8, 2019
388de8e
Add a query var to accept 'excessive_css' errors
kienstra Oct 10, 2019
17747a4
Merge branch 'develop' into add/query-args-debugging
kienstra Oct 12, 2019
2c0b1d0
Add a query var to skip tree shaking
kienstra Oct 13, 2019
e41505d
Add a debug query var to disable AMP
kienstra Oct 13, 2019
ac1311e
In the case of redirecting to the non-AMP URL, remove the flags query…
kienstra Oct 13, 2019
fffbbbd
Begin to add the submenu in the admin bar for AMP debugging options
kienstra Oct 14, 2019
fe602ac
Address failed PHPUnit test in Travis
kienstra Oct 14, 2019
405d30d
Attempt to address failed PHPUnit test by removing a filter
kienstra Oct 14, 2019
39b0e2f
Merge branch 'develop' into add/query-args-debugging
kienstra Oct 14, 2019
e4be908
Delete the remove_all_filters() calls, instead register 'wp-element'
kienstra Oct 14, 2019
81be395
Add the remaining query vars to the submenu
kienstra Oct 14, 2019
114e5d8
Change skip_tree_shaking to disable_tree_shaking
kienstra Oct 14, 2019
2bd69c3
Add the toggles for the query vars
kienstra Oct 15, 2019
5f186af
Add the debug options to the admin bar even on non-AMP endpoints
kienstra Oct 15, 2019
8b0c96f
Continue with query args.
kienstra Oct 15, 2019
7b41a0b
Remove the JS to add the admin bar debug options
kienstra Oct 18, 2019
c5b0d3b
Add a submenu to the admin bar for the debug options
kienstra Oct 18, 2019
00358e4
Merge branch 'develop' into add/query-args-debugging
kienstra Oct 18, 2019
7a3733a
Remove the entry in webpack.config.js for the previous JS file
kienstra Oct 18, 2019
687cf47
Fix failing unit test for AMP_Validation_Error_Taxonomy
kienstra Oct 18, 2019
ef0720c
Use different emojis for the checkboxes
kienstra Oct 18, 2019
675f875
Fix unit test of dependent function
kienstra Oct 18, 2019
996157a
Thanks to Alain's suggestion, add AMP_Debug class
kienstra Oct 24, 2019
e3bad9e
Remove 2 constants that were moved to AMP_Debug
kienstra Oct 24, 2019
4b6a48f
Simplify tests and improve DocBlocks
kienstra Oct 24, 2019
d5e2b2d
Add a test for passing a random string as a query var
kienstra Oct 24, 2019
1d40623
Add another assertion for a numeric string returning false
kienstra Oct 24, 2019
42a29d0
Add a little documentation for AMP_Debug::has_flag()
kienstra Oct 24, 2019
a1929ab
Change the behavior for the query var being present but empty
kienstra Oct 24, 2019
03a2395
Merge branch 'develop' of github.com:ampproject/amp-wp into add/query…
westonruter Oct 25, 2019
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
5 changes: 5 additions & 0 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,11 @@ function is_amp_endpoint() {
return true;
}

// If a certain debugging flag is present, AMP should be disabled.
if ( AMP_Debug::has_flag( AMP_Debug::DISABLE_AMP_QUERY_VAR ) ) {
return false;
}

$has_amp_query_var = (
isset( $_GET[ amp_get_slug() ] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended
||
Expand Down
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ class AMP_Autoloader {
'AMP_Validation_Callback_Wrapper' => 'includes/validation/class-amp-validation-callback-wrapper',
'AMP_Validated_URL_Post_Type' => 'includes/validation/class-amp-validated-url-post-type',
'AMP_Validation_Error_Taxonomy' => 'includes/validation/class-amp-validation-error-taxonomy',
'AMP_Debug' => 'includes/validation/class-amp-debug',
Copy link
Contributor Author

@kienstra kienstra Oct 24, 2019

Choose a reason for hiding this comment

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

It's debatable whether class-amp-debug.php should be in the validation/ directory. Most of the debugging flags have to do with validation, but some don't, like disable_amp.

'AMP_CLI_Namespace' => 'includes/cli/class-amp-cli-namespace',
'AMP_CLI_Validation_Command' => 'includes/cli/class-amp-cli-validation-command',
'AMP_String_Utils' => 'includes/utils/class-amp-string-utils',
Expand Down
23 changes: 20 additions & 3 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ public static function ensure_proper_amp_location( $exit = true ) {
*/
public static function redirect_non_amp_url( $status = 302, $exit = true ) {
$current_url = amp_get_current_url();
$non_amp_url = amp_remove_endpoint( $current_url );
$non_amp_url = remove_query_arg( AMP_Debug::AMP_FLAGS_QUERY_VAR, amp_remove_endpoint( $current_url ) );
if ( $non_amp_url === $current_url ) {
return false;
}
Expand Down Expand Up @@ -1906,6 +1906,10 @@ public static function start_output_buffering() {
newrelic_disable_autorum();
}

if ( AMP_Debug::has_flag( AMP_Debug::DISABLE_POST_PROCESSING_QUERY_VAR ) ) {
return;
}

ob_start( [ __CLASS__, 'finish_output_buffering' ] );
self::$is_output_buffering = true;
}
Expand Down Expand Up @@ -2041,6 +2045,8 @@ public static function prepare_response( $response, $args = [] ) {
! AMP_Validation_Manager::should_validate_response()
&&
! is_customize_preview()
&&
! AMP_Debug::has_flag( AMP_Debug::DISABLE_RESPONSE_CACHE_QUERY_VAR )
);

// When response caching is enabled, determine if it should be turned off for cache misses.
Expand Down Expand Up @@ -2155,7 +2161,15 @@ public static function prepare_response( $response, $args = [] ) {
AMP_HTTP::send_server_timing( 'amp_processor_cache_hit', -$prepare_response_start );

// Redirect to non-AMP version.
if ( ! amp_is_canonical() && ! is_singular( AMP_Story_Post_Type::POST_TYPE_SLUG ) && $blocking_error_count > 0 ) {
if (
! amp_is_canonical()
&&
! is_singular( AMP_Story_Post_Type::POST_TYPE_SLUG )
&&
$blocking_error_count > 0
&&
! AMP_Debug::has_flag( AMP_Debug::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR )
) {
if ( AMP_Validation_Manager::has_cap() ) {
$non_amp_url = add_query_arg( AMP_Validation_Manager::VALIDATION_ERRORS_QUERY_VAR, $blocking_error_count, $non_amp_url );
}
Expand Down Expand Up @@ -2300,7 +2314,7 @@ public static function prepare_response( $response, $args = [] ) {
$script->appendChild( $dom->createTextNode( 'document.addEventListener( "DOMContentLoaded", function() { document.write = function( text ) { throw new Error( "[AMP-WP] Prevented document.write() call with: " + text ); }; } );' ) );
$head->appendChild( $script );
}
} elseif ( ! self::is_customize_preview_iframe() ) {
} elseif ( ! self::is_customize_preview_iframe() && ! AMP_Debug::has_flag( AMP_Debug::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR ) ) {
$response = esc_html__( 'Redirecting to non-AMP version.', 'amp' );

if ( $cache_response ) {
Expand All @@ -2312,6 +2326,9 @@ public static function prepare_response( $response, $args = [] ) {
$non_amp_url = add_query_arg( AMP_Validation_Manager::VALIDATION_ERRORS_QUERY_VAR, $blocking_error_count, $non_amp_url );
}

// Remove debugging query args.
$non_amp_url = remove_query_arg( AMP_Debug::AMP_FLAGS_QUERY_VAR, $non_amp_url );

/*
* Temporary redirect because AMP page may return with blocking validation errors when auto-accepting sanitization
* is not enabled. A 302 will allow the errors to be fixed without needing to bust any redirect caches.
Expand Down
6 changes: 3 additions & 3 deletions includes/sanitizers/class-amp-style-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -2627,7 +2627,7 @@ private function finalize_styles() {
esc_html__( 'The %s element is populated with:', 'amp' ),
'style[amp-custom]'
) . "\n" . implode( "\n", $included_sources ) . "\n";
if ( self::has_required_php_css_parser() ) {
if ( self::has_required_php_css_parser() && ! AMP_Debug::has_flag( AMP_Debug::DISABLE_TREE_SHAKING_QUERY_VAR ) ) {
$comment .= sprintf(
/* translators: 1: number of included bytes. 2: percentage of total CSS actually included after tree shaking. 3: total included size. */
esc_html__( 'Total included size: %1$s bytes (%2$d%% of %3$s total after tree shaking)', 'amp' ),
Expand Down Expand Up @@ -3033,7 +3033,7 @@ function( $id ) {
$this->has_used_attributes( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] )
)
);
if ( $should_include ) {
if ( $should_include || AMP_Debug::has_flag( AMP_Debug::DISABLE_TREE_SHAKING_QUERY_VAR ) ) {
$selectors[] = $selector;
}
}
Expand Down Expand Up @@ -3130,7 +3130,7 @@ function( $a, $b ) {
}

// Report validation error if size is now too big.
if ( $current_concatenated_size + $this->pending_stylesheets[ $i ]['size'] > $max_bytes ) {
if ( $current_concatenated_size + $this->pending_stylesheets[ $i ]['size'] > $max_bytes && ! AMP_Debug::has_flag( AMP_Debug::DISABLE_TREE_SHAKING_QUERY_VAR ) ) {
$validation_error = [
'code' => 'excessive_css',
'type' => AMP_Validation_Error_Taxonomy::CSS_ERROR_TYPE,
Expand Down
119 changes: 119 additions & 0 deletions includes/validation/class-amp-debug.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
<?php
/**
* Class AMP_Debug
*
* @package AMP
*/

/**
* Class AMP_Debug
*
* @since 1.4.1
*/
final class AMP_Debug {

/**
* A top-level query var for AMP flags.
*
* Debugging query vars should follow this.
* For example, https://example.com/?amp&amp_flags[disable_post_processing]=1
*
* @var string
*/
const AMP_FLAGS_QUERY_VAR = 'amp_flags';

/**
* A query var to disable post processing.
*
* @var string
*/
const DISABLE_POST_PROCESSING_QUERY_VAR = 'disable_post_processing';

/**
* A query var to disable the response cache.
*
* @var string
*/
const DISABLE_RESPONSE_CACHE_QUERY_VAR = 'disable_response_cache';

/**
* A query var to prevent a redirect to a non-AMP URL.
*
* @var string
*/
const PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR = 'prevent_redirect';

/**
* A query var to disable tree shaking.
*
* @var string
*/
const DISABLE_TREE_SHAKING_QUERY_VAR = 'disable_tree_shaking';

/**
* Query var to reject all validation errors.
*
* @var string
*/
const REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR = 'reject_all_errors';

/**
* Query var to accept 'excessive_css' validation errors.
*
* @var string
*/
const ACCEPT_EXCESSIVE_CSS_ERROR_QUERY_VAR = 'accept_excessive_css';

/**
* Query var to disable AMP.
*
* @var string
*/
const DISABLE_AMP_QUERY_VAR = 'disable_amp';

/**
* Gets whether the flag as a query var should be considered true.
*
* If the flag is present but '', this will be true, like &amp_flags[disable_amp].
* It will also be true if it is '1' or 'true', but false for most other values.
* The flag should be after the top-level flag of 'amp_flags'.
* For example, with the flag 'disable_amp':
* https://example.com/?amp&amp_flags[disable_amp]=1
*
* @param string $flag The name of the flag (query var).
* @return bool Whether the flag as a query var should be considered true.
*/
public static function has_flag( $flag ) {
if ( ! isset( $_GET[ self::AMP_FLAGS_QUERY_VAR ][ $flag ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
return false;
}

$flag_query_var = $_GET[ self::AMP_FLAGS_QUERY_VAR ][ $flag ]; // phpcs:ignore WordPress.Security.NonceVerification.Recommended
if (
array_key_exists( $flag, self::get_all_query_vars() )
&&
AMP_Validation_Manager::has_cap()
) {
return '' === $flag_query_var ? true : filter_var( $flag_query_var, FILTER_VALIDATE_BOOLEAN );
}

return false;
}

/**
* Gets all of the debugging query vars.
*
* @return array An associative array of query var name => title.
*/
public static function get_all_query_vars() {
return [
self::DISABLE_POST_PROCESSING_QUERY_VAR => __( 'Disable post-processing', 'amp' ),
self::DISABLE_RESPONSE_CACHE_QUERY_VAR => __( 'Disable response cache', 'amp' ),
self::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR => __( 'Prevent redirect', 'amp' ),
self::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR => __( 'Reject all errors', 'amp' ),
self::ACCEPT_EXCESSIVE_CSS_ERROR_QUERY_VAR => __( 'Accept excessive CSS', 'amp' ),
self::DISABLE_AMP_QUERY_VAR => __( 'Disable AMP', 'amp' ),
self::DISABLE_TREE_SHAKING_QUERY_VAR => __( 'Disable tree shaking', 'amp' ),
];
}
}
29 changes: 29 additions & 0 deletions includes/validation/class-amp-validation-error-taxonomy.php
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ public static function register() {
}

self::accept_validation_errors( AMP_Core_Theme_Sanitizer::get_acceptable_errors( get_template() ) );
add_filter( 'amp_validation_error_sanitized', [ __CLASS__, 'conditionally_change_sanitization' ], 10, 2 );
}

/**
Expand Down Expand Up @@ -2859,4 +2860,32 @@ public static function get_status_text_with_icon( $sanitization ) {
}
return sprintf( '<span class="status-text %s">%s</span>', esc_attr( $class ), esc_html( $text ) );
}

/**
* Possibly changes the sanitization of an error, based on the presence of query vars and the proper permission.
*
* This enables rejecting all validation errors, given the proper permissions and a query var.
* Likewise, it enables accepting only 'excessive_css' errors.
*
* @param bool|null $sanitized A boolean to change the sanitization of the error, or null to not change it.
* @param array $error The error to examine.
* @return bool|null The filtered sanitization, false meaning that it will be rejected, and null meaning to not change the sanitization.
*/
public static function conditionally_change_sanitization( $sanitized, $error ) {
if ( AMP_Debug::has_flag( AMP_Debug::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ) ) {
return false;
}

if (
AMP_Debug::has_flag( AMP_Debug::ACCEPT_EXCESSIVE_CSS_ERROR_QUERY_VAR )
&&
isset( $error['code'] )
&&
'excessive_css' === $error['code']
) {
return true;
}

return $sanitized;
}
}
49 changes: 48 additions & 1 deletion includes/validation/class-amp-validation-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ public static function init( $args = [] ) {
$args
);

self::$should_locate_sources = $args['should_locate_sources'];
self::$should_locate_sources = $args['should_locate_sources'] && ! AMP_Debug::has_flag( AMP_Debug::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR );

AMP_Validated_URL_Post_Type::register();
AMP_Validation_Error_Taxonomy::register();
Expand Down Expand Up @@ -485,12 +485,15 @@ public static function add_admin_bar_menu_items( $wp_admin_bar ) {
*/
if ( $is_single_version_available ) {
$wp_admin_bar->add_node( $validate_item );
self::add_debugging_option_nodes( $wp_admin_bar, $amp_url );
} elseif ( ! is_amp_endpoint() ) {
$wp_admin_bar->add_node( $link_item );
$wp_admin_bar->add_node( $validate_item );
self::add_debugging_option_nodes( $wp_admin_bar, $amp_url );
} else {
$wp_admin_bar->add_node( $validate_item );
$wp_admin_bar->add_node( $link_item );
self::add_debugging_option_nodes( $wp_admin_bar, $amp_url );
}

// Scrub the query var from the URL.
Expand Down Expand Up @@ -519,6 +522,50 @@ function() {
self::$amp_admin_bar_item_added = true;
}

/**
* Adds the debugging option links to the admin bar.
*
* @param WP_Admin_Bar $wp_admin_bar The admin bar to add the nodes to.
* @param string $amp_url The URL of the AMP endpoint.
*/
public static function add_debugging_option_nodes( &$wp_admin_bar, $amp_url ) {
$parent_node_id = 'amp-debug';
$wp_admin_bar->add_node(
[
'parent' => 'amp',
'id' => $parent_node_id,
'title' => esc_html__( 'Debugging options', 'amp' ),
]
);

foreach ( AMP_Debug::get_all_query_vars() as $query_var => $title ) {
$wp_admin_bar->add_node(
[
'parent' => $parent_node_id,
'id' => $query_var,
'title' => self::get_debugging_option_title( $title, $query_var ),
'href' => add_query_arg( AMP_Debug::AMP_FLAGS_QUERY_VAR, [ $query_var => 'true' ], $amp_url ),
]
);
}
}

/**
* Gets the debugging option title.
*
* If the current URL has the debugging option query var present, the title starts with a 'check mark' emoji.
* For example, if the URL has the 'Prevent redirect' query var present,
* the title in the Admin Bar will be '☑ Prevent redirect'.
*
* @param string $title_text The title of the option, like 'Prevent redirect'.
* @param string $query_var The query var of the option.
* @return string The debugging option title, possible with a 'check mark' emoji at the beginning.
*/
public static function get_debugging_option_title( $title_text, $query_var ) {
$checkbox = AMP_Debug::has_flag( $query_var ) ? '☑' : '☐';
return sprintf( '<span class="amp-debug-option" style="font-family:Roboto,Oxygen-Sans,sans-serif;font-size:17px">%s</span> %s', $checkbox, esc_html( $title_text ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why this can't be a checkbox input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, a checkbox could be good, it'd just add some complexity 😄

There used to be checkboxes in this PR, here are some comments about that.

}

/**
* Add hooks for doing determining sources for validation errors during preprocessing/sanitizing.
*/
Expand Down
19 changes: 19 additions & 0 deletions tests/php/stubs.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,22 @@ public function get_styles() {
return [ 'styles' ];
}
}

// Stub class for WP_Admin_Bar.
class AMP_Test_WP_Admin_Bar {
/**
* The nodes in the admin bar.
*
* @var array
*/
public $nodes = [];

/**
* Adds a node to display in the admin bar.
*
* @param array $node The node to add.
*/
public function add_node( $node ) {
array_push( $this->nodes, $node );
}
}
Loading