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 1 commit
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
65 changes: 62 additions & 3 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,27 @@ class AMP_Theme_Support {
*/
const READER_MODE_TEMPLATE_DIRECTORY = 'amp';

/**
* 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';

/**
* Sanitizer classes.
*
Expand Down Expand Up @@ -1872,6 +1893,10 @@ public static function start_output_buffering() {
newrelic_disable_autorum();
}

if ( isset( $_GET[ self::DISABLE_POST_PROCESSING_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap() ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
Copy link
Member

Choose a reason for hiding this comment

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

Instead of introducing these as top-level query parameters, how about putting them all under some amp_flags query var? This would make it easier to scrub them from the URL and it would reduce opportunities for collision. So So here it could be checking isset( $_GET['amp_flags'][ self::DISABLE_POST_PROCESSING_QUERY_VAR ] ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a really good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

19363fc adds an amp_flags query var, to access these query vars like you mentioned.

For example:
https://loc.test/your-post/?amp&amp_flags[reject_all_errors]

return;
}

ob_start( [ __CLASS__, 'finish_output_buffering' ] );
self::$is_output_buffering = true;
}
Expand Down Expand Up @@ -2000,13 +2025,17 @@ public static function prepare_response( $response, $args = [] ) {
*
* @param bool $enable_response_caching Whether response caching is enabled.
*/
$enable_response_caching = apply_filters( 'amp_response_caching_enabled', ! ( defined( 'WP_DEBUG' ) && WP_DEBUG ) || ! empty( $args['enable_response_caching'] ) );
$enable_response_caching = apply_filters( 'amp_response_caching_enabled', ! ( defined( 'WP_DEBUG' ) && WP_DEBUG ) || ! empty( $args['enable_response_caching'] ) );
$is_disabled_with_query_var = isset( $_GET[ self::DISABLE_RESPONSE_CACHE_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended

$enable_response_caching = (
$enable_response_caching
&&
! AMP_Validation_Manager::should_validate_response()
&&
! is_customize_preview()
&&
! $is_disabled_with_query_var
);

// When response caching is enabled, determine if it should be turned off for cache misses.
Expand Down Expand Up @@ -2121,7 +2150,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
&&
! self::prevent_redirect_to_non_amp()
) {
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 @@ -2266,7 +2303,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() && ! self::prevent_redirect_to_non_amp() ) {
$response = esc_html__( 'Redirecting to non-AMP version.', 'amp' );

if ( $cache_response ) {
Expand All @@ -2278,6 +2315,15 @@ 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(
[
self::DISABLE_RESPONSE_CACHE_QUERY_VAR,
AMP_Validation_Error_Taxonomy::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR,
],
Copy link
Member

Choose a reason for hiding this comment

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

Per above, this could just remove amp_flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll apply this also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, it's applied here.

$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 Expand Up @@ -2313,6 +2359,19 @@ public static function prepare_response( $response, $args = [] ) {
return $response;
}

/**
* Whether to prevent a redirect to a non-AMP URL.
*
* Usually, if there are unaccepted validation errors,
* the AMP URL will redirect to a non-AMP URL.
* This overrides that behavior, for debugging purposes.
*
* @return bool Whether the prevent a redirect.
*/
public static function prevent_redirect_to_non_amp() {
return isset( $_GET[ self::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
}

/**
* Check for cache misses. When found, store in an option to retain the URL.
*
Expand Down
12 changes: 12 additions & 0 deletions includes/validation/class-amp-validation-error-taxonomy.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ class AMP_Validation_Error_Taxonomy {
*/
const VALIDATION_ERRORS_CLEARED_QUERY_VAR = 'amp_validation_errors_cleared';

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

/**
* The <option> value to not filter at all, like for 'All Statuses'.
*
Expand Down Expand Up @@ -465,6 +472,11 @@ public static function get_validation_error_sanitization( $error ) {
$forced = 'with_preview';
}

if ( isset( $_GET[ self::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap() ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
Copy link
Member

Choose a reason for hiding this comment

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

As opposed to adding a new condition here, I suggest using the amp_validation_error_sanitized filter below, since these debug scenarios are outside the normal band of validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, how does this look?

$status = self::VALIDATION_ERROR_ACK_REJECTED_STATUS;
$forced = 'with_query_var';
}

/**
* Filters whether the validation error should be sanitized.
*
Expand Down
2 changes: 1 addition & 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_Theme_Support::prevent_redirect_to_non_amp();

AMP_Validated_URL_Post_Type::register();
AMP_Validation_Error_Taxonomy::register();
Expand Down
35 changes: 34 additions & 1 deletion tests/php/test-class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ public function tearDown() {
AMP_Validation_Manager::reset_validation_results();
remove_theme_support( AMP_Theme_Support::SLUG );
remove_theme_support( 'custom-header' );
$_REQUEST = []; // phpcs:ignore WordPress.CSRF.NonceVerification.NoNonceVerification
$_REQUEST = [];
$_GET = [];
$_SERVER['QUERY_STRING'] = '';
unset( $_SERVER['REQUEST_URI'] );
unset( $_SERVER['REQUEST_METHOD'] );
Expand Down Expand Up @@ -1444,6 +1445,13 @@ function newrelic_disable_autorum() {
$this->assertEquals( $initial_ob_level + 1, ob_get_level() );
ob_end_flush();
$this->assertEquals( $initial_ob_level, ob_get_level() );

// When this query var is present, this method should exit early, and shouldn't buffer the output.
$_GET[ AMP_Theme_Support::DISABLE_POST_PROCESSING_QUERY_VAR ] = '';
wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );
$initial_ob_level = ob_get_level();
AMP_Theme_Support::start_output_buffering();
$this->assertEquals( $initial_ob_level, ob_get_level() );
}

/**
Expand Down Expand Up @@ -1730,7 +1738,14 @@ static function ( $url ) {
$last_server_timing_header = array_pop( $server_timing_headers );
$this->assertStringStartsWith( 'amp_processor_cache_hit;', $last_server_timing_header['value'] );
$this->assertCount( count( $server_timing_headers ), $initial_server_timing_headers );
$this->reset_post_processor_cache_effectiveness();

// Test that the response is not cached if a certain query var is present.
$_GET[ AMP_Theme_Support::DISABLE_RESPONSE_CACHE_QUERY_VAR ] = '';
wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );
$call_prepare_response();
$server_timing_headers = $this->get_server_timing_headers();
$this->assertCount( count( $server_timing_headers ), $this->get_server_timing_headers() );
// phpcs:enable WordPress.WP.EnqueuedResources.NonEnqueuedScript, WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
}

Expand Down Expand Up @@ -2090,6 +2105,24 @@ static function( $url ) use ( &$redirects ) {

}

/**
* Test prevent_redirect_to_non_amp
*
* @covers AMP_Theme_Support::prevent_redirect_to_non_amp()
*/
public function test_prevent_redirect_to_non_amp() {
// The query var isn't present, and the user doesn't have the right permission.
$this->assertFalse( AMP_Theme_Support::prevent_redirect_to_non_amp() );

// The query var is present, but the user doesn't have the right permission.
$_GET[ AMP_Theme_Support::PREVENT_REDIRECT_TO_NON_AMP_QUERY_VAR ] = '';
$this->assertFalse( AMP_Theme_Support::prevent_redirect_to_non_amp() );

// Now that the user has the right permission, this should be true.
wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );
$this->assertTrue( AMP_Theme_Support::prevent_redirect_to_non_amp() );
}

/**
* Test enqueue_assets().
*
Expand Down
14 changes: 14 additions & 0 deletions tests/php/validation/test-class-amp-validation-error-taxonomy.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class Test_AMP_Validation_Error_Taxonomy extends WP_UnitTestCase {
*/
public function tearDown() {
$_REQUEST = [];
$_GET = [];
remove_theme_support( AMP_Theme_Support::SLUG );
AMP_Theme_Support::read_theme_support();
remove_filter( 'amp_validation_error_sanitized', '__return_true' );
Expand Down Expand Up @@ -369,6 +370,19 @@ public function test_is_validation_error_sanitized_and_get_validation_error_sani
],
AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $error_bar )
);

// New rejected => Ack rejected, as the query var should force this to be rejected.
$_GET[ AMP_Validation_Error_Taxonomy::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ] = '';
wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );
$this->assertfalse( AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $error_foo ) );
$this->assertEquals(
[
'forced' => 'with_query_var',
'status' => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_REJECTED_STATUS,
'term_status' => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_REJECTED_STATUS,
],
AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $error_foo )
);
}

/**
Expand Down