From 30301da9e47a8ec0af92f5782f04f1cb179922e7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 17 Sep 2021 21:27:17 -0700 Subject: [PATCH 01/19] Allow validate requests to also store results --- includes/class-amp-theme-support.php | 32 +++++++++++++++++++ .../class-amp-validated-url-post-type.php | 2 +- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index b36c4089c27..55044c4af10 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2008,6 +2008,38 @@ public static function prepare_response( $response, $args = [] ) { $data['php_fatal_error'] = $last_error; } $data = array_merge( $data, AMP_Validation_Manager::get_validate_response_data( $sanitization_results ) ); + + if ( isset( $_GET['amp_store'] ) ) { + $validation_errors = wp_list_pluck( $data['results'], 'error' ); + unset( $data['results'] ); + + $validated_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors( + $validation_errors, + amp_get_current_url(), + $data + ); + if ( is_wp_error( $validated_url_post_id ) ) { + status_header( 500 ); + + $result = []; + foreach ( $validated_url_post_id->errors as $code => $messages ) { + foreach ( $messages as $message ) { + $result[] = [ + 'code' => $code, + 'message' => $message, + ]; + } + } + return wp_json_encode( $result ); + } else { + status_header( 201 ); + $data['validated_url_post'] = [ + 'id' => $validated_url_post_id, + 'edit_link' => get_edit_post_link( $validated_url_post_id, 'raw' ) + ]; + } + } + return wp_json_encode( $data, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES ); } diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index 5cf9cbd2b3e..0e4b5097e3e 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -765,7 +765,7 @@ protected static function normalize_url_for_storage( $url ) { // Query args to be removed from validated URLs. $removable_query_vars = array_merge( wp_removable_query_args(), - [ 'preview_id', 'preview_nonce', 'preview', QueryVar::NOAMP ] + [ 'preview_id', 'preview_nonce', 'preview', QueryVar::NOAMP, AMP_Validation_Manager::VALIDATE_QUERY_VAR, 'amp_store' ] ); // Normalize query args, removing all that are not recognized or which are removable. From 6f34359524e6969ae4a10d03f22e6149a2a1f16a Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 23 Sep 2021 10:40:49 -0700 Subject: [PATCH 02/19] Make is_validate_request a public method and member var protected --- includes/class-amp-theme-support.php | 6 ++--- .../class-amp-validation-manager.php | 22 ++++++++++++++---- tests/php/test-amp-helper-functions.php | 4 +++- tests/php/test-class-amp-base-sanitizer.php | 2 +- tests/php/test-class-amp-theme-support.php | 4 ++-- .../test-class-amp-validation-manager.php | 23 +++++++++++++------ 6 files changed, 43 insertions(+), 18 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 55044c4af10..00ea55d9ef5 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -1882,7 +1882,7 @@ public static function prepare_response( $response, $args = [] ) { } $did_redirect = $status_code >= 300 && $status_code < 400 && $sent_location_header; - if ( AMP_Validation_Manager::$is_validate_request && ! $did_redirect ) { + if ( AMP_Validation_Manager::is_validate_request() && ! $did_redirect ) { if ( ! headers_sent() ) { status_header( 400 ); header( 'Content-Type: application/json; charset=utf-8' ); @@ -1941,7 +1941,7 @@ public static function prepare_response( $response, $args = [] ) { $dom = Document::fromHtml( $response, Options::DEFAULTS ); - if ( AMP_Validation_Manager::$is_validate_request ) { + if ( AMP_Validation_Manager::is_validate_request() ) { AMP_Validation_Manager::remove_illegal_source_stack_comments( $dom ); } @@ -1997,7 +1997,7 @@ public static function prepare_response( $response, $args = [] ) { do_action( 'amp_server_timing_stop', 'amp_sanitizer' ); // Respond early with results if performing a validate request. - if ( AMP_Validation_Manager::$is_validate_request ) { + if ( AMP_Validation_Manager::is_validate_request() ) { status_header( 200 ); header( 'Content-Type: application/json; charset=utf-8' ); $data = [ diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index b90bdbdb547..0127ced7294 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -159,7 +159,7 @@ class AMP_Validation_Manager { * * @var bool */ - public static $is_validate_request = false; + protected static $is_validate_request = false; /** * Overrides for validation errors. @@ -468,7 +468,7 @@ public static function override_validation_error_statuses() { * @since 2.1 */ public static function maybe_fail_validate_request() { - if ( ! self::$is_validate_request || amp_is_request() ) { + if ( ! self::is_validate_request() || amp_is_request() ) { return; } @@ -482,6 +482,20 @@ public static function maybe_fail_validate_request() { wp_send_json( compact( 'code', 'message' ), 400 ); } + /** + * Whether a validate request is being performed. + * + * When responding to a request to validate a URL, instead of an HTML document being returned, a JSON document is + * returned with any errors that were encountered during validation. + * + * @see AMP_Validation_Manager::get_validate_response_data() + * + * @return bool + */ + public static function is_validate_request() { + return self::$is_validate_request; + } + /** * Initialize a validate request. * @@ -613,7 +627,7 @@ public static function add_validation_error( array $error, array $data = [] ) { $node = $data['node']; } - if ( self::$is_validate_request ) { + if ( self::is_validate_request() ) { if ( ! empty( $error['sources'] ) ) { $sources = $error['sources']; } elseif ( $node ) { @@ -1705,7 +1719,7 @@ public static function filter_sanitizer_args( $sanitizers ) { } if ( isset( $sanitizers[ AMP_Style_Sanitizer::class ] ) ) { - $sanitizers[ AMP_Style_Sanitizer::class ]['should_locate_sources'] = self::$is_validate_request; + $sanitizers[ AMP_Style_Sanitizer::class ]['should_locate_sources'] = self::is_validate_request(); $css_validation_errors = []; foreach ( self::$validation_error_status_overrides as $slug => $status ) { diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index a834d918955..78f3dff6636 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -9,6 +9,7 @@ use AmpProject\AmpWP\QueryVar; use AmpProject\AmpWP\Tests\Helpers\HandleValidation; use AmpProject\AmpWP\Tests\Helpers\LoadsCoreThemes; +use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; use AmpProject\AmpWP\Tests\DependencyInjectedTestCase; use AmpProject\AmpWP\AmpSlugCustomizationWatcher; @@ -19,6 +20,7 @@ class Test_AMP_Helper_Functions extends DependencyInjectedTestCase { use HandleValidation; use LoadsCoreThemes; + use PrivateAccess; /** * The mock Site Icon value to use in a filter. @@ -50,7 +52,7 @@ public function setUp() { */ public function tearDown() { AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::READER_MODE_SLUG ); - AMP_Validation_Manager::$is_validate_request = false; + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', false ); global $wp_scripts, $pagenow, $show_admin_bar, $current_screen; $wp_scripts = null; $show_admin_bar = null; diff --git a/tests/php/test-class-amp-base-sanitizer.php b/tests/php/test-class-amp-base-sanitizer.php index bc51d04f0a4..d0884ed57da 100644 --- a/tests/php/test-class-amp-base-sanitizer.php +++ b/tests/php/test-class-amp-base-sanitizer.php @@ -32,7 +32,7 @@ public function setUp() { public function tearDown() { parent::tearDown(); AMP_Validation_Manager::reset_validation_results(); - AMP_Validation_Manager::$is_validate_request = false; + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', false ); } /** diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php index 050c8e372ba..42bd7bfaf84 100644 --- a/tests/php/test-class-amp-theme-support.php +++ b/tests/php/test-class-amp-theme-support.php @@ -81,7 +81,7 @@ public function tearDown() { parent::tearDown(); unset( $GLOBALS['show_admin_bar'] ); - AMP_Validation_Manager::$is_validate_request = false; + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', false ); AMP_Validation_Manager::reset_validation_results(); $this->set_template_mode( AMP_Theme_Support::READER_MODE_SLUG ); remove_theme_support( 'custom-header' ); @@ -1890,7 +1890,7 @@ public function test_prepare_response_for_submitted_form() { * @covers AMP_Theme_Support::prepare_response() */ public function test_prepare_response_for_validating_invalid_amp_page() { - AMP_Validation_Manager::$is_validate_request = true; + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', true ); $response = AMP_Theme_Support::prepare_response( '' ); $this->assertJson( $response ); diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 5fc1cf905e4..7a804b80e2d 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -140,7 +140,7 @@ public function tearDown() { AMP_Validation_Manager::$validation_error_status_overrides = []; $_REQUEST = []; unset( $GLOBALS['current_screen'] ); - AMP_Validation_Manager::$is_validate_request = false; + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', false ); AMP_Validation_Manager::$hook_source_stack = []; AMP_Validation_Manager::$validation_results = []; AMP_Validation_Manager::reset_validation_results(); @@ -194,11 +194,11 @@ static function () { }; // Verify there is no output if it is not a validation request. - AMP_Validation_Manager::$is_validate_request = false; + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', false ); $this->assertEmpty( $get_output() ); // Verify there is no output if it is an AMP request. - AMP_Validation_Manager::$is_validate_request = true; + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', true ); AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); $this->go_to( get_permalink( $post_id ) ); $this->assertEmpty( $get_output() ); @@ -210,7 +210,7 @@ static function () { $this->assertStringContainsString( 'AMP_NOT_REQUESTED', $output ); // Verify correct response if AMP not available. - AMP_Validation_Manager::$is_validate_request = true; + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', true ); AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::READER_MODE_SLUG ); add_filter( @@ -228,6 +228,15 @@ static function ( $skipped, $_post_id ) use ( $post_id ) { $this->assertStringContainsString( 'AMP_NOT_AVAILABLE', $output ); } + /** @covers AMP_Validation_Manager::is_validate_request() */ + public function test_is_validate_request() { + $this->assertFalse( AMP_Validation_Manager::is_validate_request() ); + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', true ); + $this->assertTrue( AMP_Validation_Manager::is_validate_request() ); + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', false ); + $this->assertFalse( AMP_Validation_Manager::is_validate_request() ); + } + /** * Test init_validate_request without error. * @@ -236,12 +245,12 @@ static function ( $skipped, $_post_id ) use ( $post_id ) { public function test_init_validate_request_without_error() { $this->assertFalse( AMP_Validation_Manager::should_validate_response() ); AMP_Validation_Manager::init_validate_request(); - $this->assertFalse( AMP_Validation_Manager::$is_validate_request ); + $this->assertFalse( AMP_Validation_Manager::is_validate_request() ); $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ] = wp_slash( AMP_Validation_Manager::get_amp_validate_nonce() ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended $this->assertTrue( AMP_Validation_Manager::should_validate_response() ); AMP_Validation_Manager::init_validate_request(); - $this->assertTrue( AMP_Validation_Manager::$is_validate_request ); + $this->assertTrue( AMP_Validation_Manager::is_validate_request() ); } /** @@ -613,7 +622,7 @@ static function ( $caps, $cap, $user_id ) { * @covers AMP_Validation_Manager::add_validation_error() */ public function test_add_validation_error_track_removed() { - AMP_Validation_Manager::$is_validate_request = true; + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', true ); $this->assertEmpty( AMP_Validation_Manager::$validation_results ); $that = $this; From f57e397b18caa7cb7ca549a2cd90eff33c56b8dd Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 23 Sep 2021 10:41:27 -0700 Subject: [PATCH 03/19] fixup! Allow validate requests to also store results --- includes/class-amp-theme-support.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 00ea55d9ef5..4ca6dd8cf9c 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2009,7 +2009,7 @@ public static function prepare_response( $response, $args = [] ) { } $data = array_merge( $data, AMP_Validation_Manager::get_validate_response_data( $sanitization_results ) ); - if ( isset( $_GET['amp_store'] ) ) { + if ( isset( $_GET['amp_store'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended $validation_errors = wp_list_pluck( $data['results'], 'error' ); unset( $data['results'] ); @@ -2035,7 +2035,7 @@ public static function prepare_response( $response, $args = [] ) { status_header( 201 ); $data['validated_url_post'] = [ 'id' => $validated_url_post_id, - 'edit_link' => get_edit_post_link( $validated_url_post_id, 'raw' ) + 'edit_link' => get_edit_post_link( $validated_url_post_id, 'raw' ), ]; } } From af3c45bbc80af5109e6a71bb5ffdce2b99efb330 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 23 Sep 2021 10:47:10 -0700 Subject: [PATCH 04/19] Use constant for amp_store query param --- includes/class-amp-theme-support.php | 2 +- includes/validation/class-amp-validated-url-post-type.php | 2 +- includes/validation/class-amp-validation-manager.php | 7 +++++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 4ca6dd8cf9c..92bdae8326e 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2009,7 +2009,7 @@ public static function prepare_response( $response, $args = [] ) { } $data = array_merge( $data, AMP_Validation_Manager::get_validate_response_data( $sanitization_results ) ); - if ( isset( $_GET['amp_store'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended + if ( isset( $_GET[ AMP_Validation_Manager::STORE_QUERY_VAR ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended $validation_errors = wp_list_pluck( $data['results'], 'error' ); unset( $data['results'] ); diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index 0e4b5097e3e..83408cc44b2 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -765,7 +765,7 @@ protected static function normalize_url_for_storage( $url ) { // Query args to be removed from validated URLs. $removable_query_vars = array_merge( wp_removable_query_args(), - [ 'preview_id', 'preview_nonce', 'preview', QueryVar::NOAMP, AMP_Validation_Manager::VALIDATE_QUERY_VAR, 'amp_store' ] + [ 'preview_id', 'preview_nonce', 'preview', QueryVar::NOAMP, AMP_Validation_Manager::VALIDATE_QUERY_VAR, AMP_Validation_Manager::STORE_QUERY_VAR ] ); // Normalize query args, removing all that are not recognized or which are removable. diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 0127ced7294..f9dc159e2bd 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -29,6 +29,13 @@ class AMP_Validation_Manager { */ const VALIDATE_QUERY_VAR = 'amp_validate'; + /** + * Query param that indicates a validation request should store the results. + * + * @var string + */ + const STORE_QUERY_VAR = 'amp_store'; + /** * Meta capability for validation. * From f9309495b2d61babdb4960e031b127837c1ce11f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 23 Sep 2021 11:50:10 -0700 Subject: [PATCH 05/19] Remove needless pretty-printing of validation response --- includes/class-amp-theme-support.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 92bdae8326e..e60ee91be36 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2040,7 +2040,7 @@ public static function prepare_response( $response, $args = [] ) { } } - return wp_json_encode( $data, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES ); + return wp_json_encode( $data, JSON_UNESCAPED_SLASHES ); } /** From 808d969f0306dab585be139b98983be3ce346d96 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 23 Sep 2021 13:51:34 -0700 Subject: [PATCH 06/19] Add amp_available_override query param to force AMP to be available --- includes/amp-helper-functions.php | 9 +++++++++ src/QueryVar.php | 13 +++++++++++++ 2 files changed, 22 insertions(+) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index d7fbc6f0d4a..cff09bc681f 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -465,6 +465,15 @@ function amp_is_available() { return false; } + // Allow query parameter to force AMP to be available, depending on whether it's a validation request or the user can do validation. + if ( + isset( $_GET[ QueryVar::AMP_AVAILABLE_OVERRIDE ] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended + && + ( AMP_Validation_Manager::is_validate_request() || AMP_Validation_Manager::has_cap() ) + ) { + return true; + } + // Ensure that all templates can be accessed in AMP when a Reader theme is selected. $has_reader_theme = ( AMP_Theme_Support::READER_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) diff --git a/src/QueryVar.php b/src/QueryVar.php index a208329890d..6ceb6e0ee0d 100644 --- a/src/QueryVar.php +++ b/src/QueryVar.php @@ -64,6 +64,19 @@ interface QueryVar { */ const NOAMP_AVAILABLE = 'available'; + /** + * Whether AMP should be forcibly be made available for the given request. + * + * This causes the supportable templates to be disregarded as well as whether AMP is disabled for a given post. + * + * This is only honored during validation requests or if the user has validation capability. + * + * @see \AMP_Validation_Manager::is_validate_request() + * @see \AMP_Validation_Manager::has_cap() + * @var string + */ + const AMP_AVAILABLE_OVERRIDE = 'amp_available_override'; + /** * Value for the query var that allows switching to verbose server-timing output. * From c2a4738012158be7a7d443819120f1003442d685 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 24 Sep 2021 20:26:05 -0700 Subject: [PATCH 07/19] WIP: Try facilitating selective AMP-first URLs when in paired mode --- includes/amp-helper-functions.php | 41 +++++- includes/class-amp-theme-support.php | 3 +- .../sanitizers/class-amp-link-sanitizer.php | 1 + .../class-amp-validation-manager.php | 134 ++++++++++++++++++ src/PairedRouting.php | 32 +++++ src/PairedUrlStructure.php | 10 ++ src/QueryVar.php | 11 ++ 7 files changed, 230 insertions(+), 2 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index cff09bc681f..6983e6d089c 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -309,8 +309,20 @@ function amp_force_query_var_value( $query_vars ) { * @see AMP_Theme_Support::read_theme_support() * @return boolean Whether this is in AMP 'canonical' mode, that is whether it is AMP-first and there is not a separate (paired) AMP URL. */ -function amp_is_canonical() { +function amp_is_canonical( $url = '' ) { return AMP_Theme_Support::STANDARD_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ); + + if ( AMP_Theme_Support::STANDARD_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) ) { + return true; + } + + if ( ! $url ) { + $url = amp_get_current_url(); + } + + // Services::get( 'paired_routing' ) + + return false; } /** @@ -769,8 +781,13 @@ function amp_is_request() { amp_is_canonical() || amp_has_paired_endpoint() + || + ! Services::get( 'paired_routing' )->needs_endpoint() ); + error_log( json_encode( Services::get( 'paired_routing' )->needs_endpoint(), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES ) ); + + // If AMP is not available, then it's definitely not an AMP endpoint. if ( ! amp_is_available() ) { // But, if WP_Query was not available yet, then we will just assume the query is supported since at this point we do @@ -2036,6 +2053,28 @@ function amp_has_paired_endpoint( $url = '' ) { } } +/** + * Determine if a given URL needs to be paired to serve AMP (in other words, if the URL is not AMP-first). + * + * @since 2.2 + * + * @param string $url URL to examine. If empty, will use the current URL. + * @return bool True if URL needs paired endpoint to serve AMP. False if the URL is AMP-first (canonical). + */ +function amp_needs_paired_endpoint( $url = '' ) { + try { + return Services::get( 'paired_routing' )->needs_endpoint( $url ); + } catch ( InvalidService $e ) { + if ( ! amp_is_enabled() ) { + $reason = __( 'Function called while AMP is disabled via `amp_is_enabled` filter.', 'amp' ); + } else { + $reason = __( 'Function cannot be called before services are registered.', 'amp' ); + } + _doing_it_wrong( __FUNCTION__, esc_html( $reason ) . ' ' . esc_html( $e->getMessage() ), '2.2' ); + return false; + } +} + /** * Remove the paired AMP endpoint from a given URL. * diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index e60ee91be36..3585608c4f0 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -327,10 +327,11 @@ public static function finish_init() { return; } - if ( amp_is_legacy() ) { + if ( amp_is_legacy() && amp_needs_paired_endpoint() ) { // Make sure there is no confusion when serving the legacy Reader template that the normal theme hooks should not be used. remove_theme_support( self::SLUG ); + // @todo This doesn't work with Web Stories. add_filter( 'template_include', static function() { diff --git a/includes/sanitizers/class-amp-link-sanitizer.php b/includes/sanitizers/class-amp-link-sanitizer.php index 0652bcbbfbe..50a7bbc76e0 100644 --- a/includes/sanitizers/class-amp-link-sanitizer.php +++ b/includes/sanitizers/class-amp-link-sanitizer.php @@ -201,6 +201,7 @@ private function process_element( DOMElement $element, $attribute_name ) { in_array( Attribute::REL_NOAMPHTML, $rel, true ) || in_array( strtok( $url, '#' ), $this->args['excluded_urls'], true ) + // @todo || ! \AmpProject\AmpWP\Services::get( 'paired_routing' )->needs_endpoint( $url ) ); /** diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index f9dc159e2bd..b8e57a12f5f 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -7,6 +7,7 @@ use AmpProject\AmpWP\DevTools\UserAccess; use AmpProject\AmpWP\Icon; +use AmpProject\AmpWP\Option; use AmpProject\AmpWP\QueryVar; use AmpProject\AmpWP\Services; use AmpProject\Attribute; @@ -215,12 +216,140 @@ static function() { } ); + // Allow query parameter to force a response to be served with Standard mode (AMP-first). This query parameter + // is only honored when doing a validation request or when the user is able to do validation. This is used as + // part of Site Scanning in order to determine if the primary theme is suitable for serving AMP. + // There's two concerns: (1) serving AMP on canonical non-paired URL, and (2) using primary theme templates + // instead of Reader theme. + if ( self::is_validate_request() || self::has_cap() ) { + // cf. amp_to_amp_linking_element_excluded + + + add_filter( + 'amp_url_needs_paired_endpoint', + function ( $needs_endpoint, $url ) { + $query_string = wp_parse_url( $url, PHP_URL_QUERY ); + $query_vars = []; + if ( $query_string ) { + wp_parse_str( $query_string, $query_vars ); + } + if ( array_key_exists( QueryVar::AMP_FIRST, $query_vars ) ) { + $needs_endpoint = false; + } + return $needs_endpoint; + }, + 10, + 2 + ); + + // When an AMP-first request is made, use Standard mode. + if ( isset( $_GET[ QueryVar::AMP_FIRST ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended + add_filter( + 'option_' . AMP_Options_Manager::OPTION_NAME, + static function ( $options ) { + $options[ Option::THEME_SUPPORT ] = AMP_Theme_Support::STANDARD_MODE_SLUG; + return $options; + } + ); + } + +// wp_die( esc_html__( 'Please log-in to preview AMP for this URL.', 'amp' ) ); + } +// +// if ( isset( $_GET[ QueryVar::AMP_FIRST ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended +// if ( ! ( self::is_validate_request() || self::has_cap() ) ) { +// wp_die( esc_html__( 'Please log-in to preview AMP for this URL.', 'amp' ) ); +// } +// +// add_filter( +// 'amp_url_needs_paired_endpoint', +// function ( $url ) { +// return false === strpos( $url, 'amp_first' ); +// } +// ); +// +// add_filter( +// 'option_' . AMP_Options_Manager::OPTION_NAME, +// static function ( $options ) { +// $options[ Option::THEME_SUPPORT ] = AMP_Theme_Support::STANDARD_MODE_SLUG; +// return $options; +// } +// ); +// } + add_action( 'all_admin_notices', [ __CLASS__, 'print_plugin_notice' ] ); add_action( 'admin_bar_menu', [ __CLASS__, 'add_admin_bar_menu_items' ], 101 ); add_action( 'wp', [ __CLASS__, 'maybe_fail_validate_request' ] ); add_action( 'wp', [ __CLASS__, 'override_validation_error_statuses' ] ); } + + + /** + * Determine whether the request is AMP-first. + * + * @see \Google\Web_Stories\Integrations\AMP::get_request_post_type() + * + * @todo We need to help Web Stories eliminate the need for get_request_post_type(). What can we do to facilitate that? + * + * @return string|null + */ + protected function is_request_amp_first() { + if ( amp_is_canonical() ) { + return true; + } + + + + // phpcs:disable WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + + + + if ( did_action( 'wp' ) && is_singular() ) { + $post_type = get_post_type( get_queried_object_id() ); + return $post_type ?: null; + } + + if ( + is_admin() + && + isset( $_GET['action'], $_GET['post'] ) + && + 'amp_validate' === $_GET['action'] + && + get_post_type( (int) $_GET['post'] ) === self::AMP_VALIDATED_URL_POST_TYPE + ) { + return $this->get_validated_url_post_type( (int) $_GET['post'] ); + } + + $current_screen = $this->get_current_screen(); + + if ( $current_screen instanceof WP_Screen ) { + $current_post = get_post(); + + if ( self::AMP_VALIDATED_URL_POST_TYPE === $current_screen->post_type && $current_post instanceof WP_Post && $current_post->post_type === $current_screen->post_type ) { + $validated_url_post_type = $this->get_validated_url_post_type( $current_post->ID ); + if ( $validated_url_post_type ) { + return $validated_url_post_type; + } + } + + if ( $current_screen->post_type ) { + return $current_screen->post_type; + } + + return null; + } + + if ( isset( $_SERVER['REQUEST_URI'] ) && false !== strpos( (string) wp_unslash( $_SERVER['REQUEST_URI'] ), '/web-stories/v1/web-story/' ) ) { + return Story_Post_Type::POST_TYPE_SLUG; + } + + // phpcs:enable WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + + return null; + } + /** * Determine if a post supports AMP validation. * @@ -1814,10 +1943,15 @@ private static function validate_validation_url( $url ) { * } */ public static function validate_url( $url ) { + $query_string = wp_parse_url( $url, PHP_URL_QUERY ); if ( ! amp_is_canonical() && ! amp_has_paired_endpoint( $url ) ) { $url = amp_add_paired_endpoint( $url ); } + if ( false !== strpos( $query_string, 'amp_first' ) ) { + $url = amp_remove_paired_endpoint( $url ); + } + $added_query_vars = [ self::VALIDATE_QUERY_VAR => self::get_amp_validate_nonce(), self::CACHE_BUST_QUERY_VAR => wp_rand(), diff --git a/src/PairedRouting.php b/src/PairedRouting.php index 2fd8b1691d3..e1ce10648b0 100644 --- a/src/PairedRouting.php +++ b/src/PairedRouting.php @@ -915,6 +915,34 @@ public function has_endpoint( $url = '' ) { return $this->get_paired_url_structure()->has_endpoint( $url ); } + /** + * Determine if a given URL needs to be paired to serve AMP (in other words, if the URL is not AMP-first). + * + * @since 2.2 + * + * @param string $url URL to examine. If empty, will use the current URL. + * @return bool True if URL needs paired endpoint to serve AMP. False if the URL is AMP-first (canonical). + */ + public function needs_endpoint( $url = '' ) { + if ( ! $url ) { + $url = amp_get_current_url(); + } + + if ( $this->has_endpoint( $url ) ) { + $url = $this->remove_endpoint( $url ); + } + + /** + * Filters whether the given URL has a paired endpoint to access the AMP version. + * + * @since 2.2 + * + * @param bool $needs_paired Needs paired. + * @param string $url URL. + */ + return (bool) apply_filters( 'amp_url_needs_paired_endpoint', ! amp_is_canonical(), $url ); + } + /** * Turn a given URL into a paired AMP URL. * @@ -922,6 +950,10 @@ public function has_endpoint( $url = '' ) { * @return string AMP URL. */ public function add_endpoint( $url ) { + if ( ! $this->needs_endpoint( $url ) ) { + return $url; + } + return $this->get_paired_url_structure()->add_endpoint( $url ); } diff --git a/src/PairedUrlStructure.php b/src/PairedUrlStructure.php index 3a83743efc0..7630cb4eadf 100644 --- a/src/PairedUrlStructure.php +++ b/src/PairedUrlStructure.php @@ -41,6 +41,16 @@ public function has_endpoint( $url ) { return $url !== $this->remove_endpoint( $url ); } + /** + * Determine if the URL would need an endpoint. + * + * @param string $url URL (or REQUEST_URI). + * @return bool URL with AMP stripped. + */ + public function needs_endpoint( $url ) { + return true; + } + /** * Turn a given URL into a paired AMP URL. * diff --git a/src/QueryVar.php b/src/QueryVar.php index 6ceb6e0ee0d..191a4d7bba0 100644 --- a/src/QueryVar.php +++ b/src/QueryVar.php @@ -31,6 +31,17 @@ interface QueryVar { */ const AMP = 'amp'; + /** + * Query var which forces the template mode to be Standard (aka AMP-first aka canonical). + * + * This is only honored during validation requests or if the user has validation capability. + * + * @see \AMP_Validation_Manager::is_validate_request() + * @see \AMP_Validation_Manager::has_cap() + * @var string + */ + const AMP_FIRST = 'amp_first'; + /** * Query var used to signal the request for an non-AMP page. * From ee5cd4bc6299480a841f1a30dade545feabebd7a Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 24 Sep 2021 22:17:26 -0700 Subject: [PATCH 08/19] Adopt the Web Stories approach of selectively enabling Standard mode --- includes/amp-helper-functions.php | 41 +--- includes/class-amp-theme-support.php | 3 +- .../sanitizers/class-amp-link-sanitizer.php | 1 - .../class-amp-validation-manager.php | 193 ++++++++---------- src/PairedRouting.php | 32 --- src/PairedUrlStructure.php | 10 - src/QueryVar.php | 4 +- 7 files changed, 87 insertions(+), 197 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 6983e6d089c..cff09bc681f 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -309,20 +309,8 @@ function amp_force_query_var_value( $query_vars ) { * @see AMP_Theme_Support::read_theme_support() * @return boolean Whether this is in AMP 'canonical' mode, that is whether it is AMP-first and there is not a separate (paired) AMP URL. */ -function amp_is_canonical( $url = '' ) { +function amp_is_canonical() { return AMP_Theme_Support::STANDARD_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ); - - if ( AMP_Theme_Support::STANDARD_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) ) { - return true; - } - - if ( ! $url ) { - $url = amp_get_current_url(); - } - - // Services::get( 'paired_routing' ) - - return false; } /** @@ -781,13 +769,8 @@ function amp_is_request() { amp_is_canonical() || amp_has_paired_endpoint() - || - ! Services::get( 'paired_routing' )->needs_endpoint() ); - error_log( json_encode( Services::get( 'paired_routing' )->needs_endpoint(), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES ) ); - - // If AMP is not available, then it's definitely not an AMP endpoint. if ( ! amp_is_available() ) { // But, if WP_Query was not available yet, then we will just assume the query is supported since at this point we do @@ -2053,28 +2036,6 @@ function amp_has_paired_endpoint( $url = '' ) { } } -/** - * Determine if a given URL needs to be paired to serve AMP (in other words, if the URL is not AMP-first). - * - * @since 2.2 - * - * @param string $url URL to examine. If empty, will use the current URL. - * @return bool True if URL needs paired endpoint to serve AMP. False if the URL is AMP-first (canonical). - */ -function amp_needs_paired_endpoint( $url = '' ) { - try { - return Services::get( 'paired_routing' )->needs_endpoint( $url ); - } catch ( InvalidService $e ) { - if ( ! amp_is_enabled() ) { - $reason = __( 'Function called while AMP is disabled via `amp_is_enabled` filter.', 'amp' ); - } else { - $reason = __( 'Function cannot be called before services are registered.', 'amp' ); - } - _doing_it_wrong( __FUNCTION__, esc_html( $reason ) . ' ' . esc_html( $e->getMessage() ), '2.2' ); - return false; - } -} - /** * Remove the paired AMP endpoint from a given URL. * diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 3585608c4f0..e60ee91be36 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -327,11 +327,10 @@ public static function finish_init() { return; } - if ( amp_is_legacy() && amp_needs_paired_endpoint() ) { + if ( amp_is_legacy() ) { // Make sure there is no confusion when serving the legacy Reader template that the normal theme hooks should not be used. remove_theme_support( self::SLUG ); - // @todo This doesn't work with Web Stories. add_filter( 'template_include', static function() { diff --git a/includes/sanitizers/class-amp-link-sanitizer.php b/includes/sanitizers/class-amp-link-sanitizer.php index 50a7bbc76e0..0652bcbbfbe 100644 --- a/includes/sanitizers/class-amp-link-sanitizer.php +++ b/includes/sanitizers/class-amp-link-sanitizer.php @@ -201,7 +201,6 @@ private function process_element( DOMElement $element, $attribute_name ) { in_array( Attribute::REL_NOAMPHTML, $rel, true ) || in_array( strtok( $url, '#' ), $this->args['excluded_urls'], true ) - // @todo || ! \AmpProject\AmpWP\Services::get( 'paired_routing' )->needs_endpoint( $url ) ); /** diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index b8e57a12f5f..819f8c66570 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -216,138 +216,116 @@ static function() { } ); + add_action( 'all_admin_notices', [ __CLASS__, 'print_plugin_notice' ] ); + add_action( 'admin_bar_menu', [ __CLASS__, 'add_admin_bar_menu_items' ], 101 ); + add_action( 'wp', [ __CLASS__, 'maybe_fail_validate_request' ] ); + add_action( 'wp', [ __CLASS__, 'override_validation_error_statuses' ] ); + // Allow query parameter to force a response to be served with Standard mode (AMP-first). This query parameter // is only honored when doing a validation request or when the user is able to do validation. This is used as // part of Site Scanning in order to determine if the primary theme is suitable for serving AMP. - // There's two concerns: (1) serving AMP on canonical non-paired URL, and (2) using primary theme templates - // instead of Reader theme. - if ( self::is_validate_request() || self::has_cap() ) { - // cf. amp_to_amp_linking_element_excluded - - + if ( ! amp_is_canonical() ) { add_filter( - 'amp_url_needs_paired_endpoint', - function ( $needs_endpoint, $url ) { - $query_string = wp_parse_url( $url, PHP_URL_QUERY ); - $query_vars = []; - if ( $query_string ) { - wp_parse_str( $query_string, $query_vars ); - } - if ( array_key_exists( QueryVar::AMP_FIRST, $query_vars ) ) { - $needs_endpoint = false; - } - return $needs_endpoint; - }, - 10, - 2 - ); - - // When an AMP-first request is made, use Standard mode. - if ( isset( $_GET[ QueryVar::AMP_FIRST ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended - add_filter( - 'option_' . AMP_Options_Manager::OPTION_NAME, - static function ( $options ) { + 'option_' . AMP_Options_Manager::OPTION_NAME, + static function ( $options ) { + if ( self::is_amp_first_override_request() ) { $options[ Option::THEME_SUPPORT ] = AMP_Theme_Support::STANDARD_MODE_SLUG; - return $options; } - ); - } - -// wp_die( esc_html__( 'Please log-in to preview AMP for this URL.', 'amp' ) ); - } -// -// if ( isset( $_GET[ QueryVar::AMP_FIRST ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended -// if ( ! ( self::is_validate_request() || self::has_cap() ) ) { -// wp_die( esc_html__( 'Please log-in to preview AMP for this URL.', 'amp' ) ); -// } -// -// add_filter( -// 'amp_url_needs_paired_endpoint', -// function ( $url ) { -// return false === strpos( $url, 'amp_first' ); -// } -// ); -// -// add_filter( -// 'option_' . AMP_Options_Manager::OPTION_NAME, -// static function ( $options ) { -// $options[ Option::THEME_SUPPORT ] = AMP_Theme_Support::STANDARD_MODE_SLUG; -// return $options; -// } -// ); -// } - - add_action( 'all_admin_notices', [ __CLASS__, 'print_plugin_notice' ] ); - add_action( 'admin_bar_menu', [ __CLASS__, 'add_admin_bar_menu_items' ], 101 ); - add_action( 'wp', [ __CLASS__, 'maybe_fail_validate_request' ] ); - add_action( 'wp', [ __CLASS__, 'override_validation_error_statuses' ] ); + return $options; + } + ); + } } - - /** - * Determine whether the request is AMP-first. + * Determine whether the request includes the AMP-first override. * - * @see \Google\Web_Stories\Integrations\AMP::get_request_post_type() + * The logic in here is admittedly a mess. It was first worked out in the context of the Web Stories plugin to + * force a single web story to be served without any paired endpoint when a site is running the AMP plugin in + * a paired template mode (Transitional or Reader). * - * @todo We need to help Web Stories eliminate the need for get_request_post_type(). What can we do to facilitate that? + * @since 2.2 + * @see \Google\Web_Stories\Integrations\AMP::get_request_post_type() + * @link https://github.com/google/web-stories-wp/pull/3621 * - * @return string|null + * @return bool Whether */ - protected function is_request_amp_first() { - if ( amp_is_canonical() ) { + private static function is_amp_first_override_request() { + // phpcs:disable WordPress.Security.NonceVerification.Recommended + + // Frontend. + if ( + isset( $_GET[ QueryVar::AMP_FIRST ] ) + && + ( self::is_validate_request() || self::has_cap() ) + ) { return true; } - - - // phpcs:disable WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized - - - - if ( did_action( 'wp' ) && is_singular() ) { - $post_type = get_post_type( get_queried_object_id() ); - return $post_type ?: null; + // If not in the admin or the user doesn't have the validate capability, then abort. + if ( ! is_admin() || ! self::has_cap() ) { + return false; } + // Admin request for validation. if ( - is_admin() - && - isset( $_GET['action'], $_GET['post'] ) + isset( $_GET['action'] ) && - 'amp_validate' === $_GET['action'] + self::VALIDATE_QUERY_VAR === $_GET['action'] && - get_post_type( (int) $_GET['post'] ) === self::AMP_VALIDATED_URL_POST_TYPE + ( + // First admin request to validate a URL. + ( + isset( $_GET['url'] ) + && + self::is_amp_first_override_url( esc_url_raw( $_GET['url'] ) ) + ) + || + // Subsequent admin request to validate a URL. + ( + isset( $_GET['post'] ) + && + get_post_type( (int) $_GET['post'] ) === AMP_Validated_URL_Post_Type::POST_TYPE_SLUG + && + self::is_amp_first_override_url( get_post( (int) $_GET['post'] )->post_title ) + ) + ) ) { - return $this->get_validated_url_post_type( (int) $_GET['post'] ); - } - - $current_screen = $this->get_current_screen(); - - if ( $current_screen instanceof WP_Screen ) { - $current_post = get_post(); - - if ( self::AMP_VALIDATED_URL_POST_TYPE === $current_screen->post_type && $current_post instanceof WP_Post && $current_post->post_type === $current_screen->post_type ) { - $validated_url_post_type = $this->get_validated_url_post_type( $current_post->ID ); - if ( $validated_url_post_type ) { - return $validated_url_post_type; - } - } - - if ( $current_screen->post_type ) { - return $current_screen->post_type; - } - - return null; + return true; } - if ( isset( $_SERVER['REQUEST_URI'] ) && false !== strpos( (string) wp_unslash( $_SERVER['REQUEST_URI'] ), '/web-stories/v1/web-story/' ) ) { - return Story_Post_Type::POST_TYPE_SLUG; + // Admin screen for validated URL screen and Validated URLs post list table (where this may only return true + // selectively based on the current post in the loop). + $current_screen = function_exists( 'get_current_screen' ) ? get_current_screen() : null; + if ( + $current_screen instanceof WP_Screen + && + AMP_Validated_URL_Post_Type::POST_TYPE_SLUG === $current_screen->post_type + && + self::is_amp_first_override_url( get_post()->post_title ) + ) { + return true; } - // phpcs:enable WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + // phpcs:enable WordPress.Security.NonceVerification.Recommended + return false; + } - return null; + /** + * Determine whether the URL includes the AMP-first override query var. + * + * @since 2.2 + * + * @param string $url URL. + * @return bool Whether the URL has the AMP-first override. + */ + private static function is_amp_first_override_url( $url ) { + $query_string = wp_parse_url( $url, PHP_URL_QUERY ); + $query_vars = []; + if ( $query_string ) { + wp_parse_str( $query_string, $query_vars ); + } + return array_key_exists( QueryVar::AMP_FIRST, $query_vars ); } /** @@ -1943,15 +1921,10 @@ private static function validate_validation_url( $url ) { * } */ public static function validate_url( $url ) { - $query_string = wp_parse_url( $url, PHP_URL_QUERY ); if ( ! amp_is_canonical() && ! amp_has_paired_endpoint( $url ) ) { $url = amp_add_paired_endpoint( $url ); } - if ( false !== strpos( $query_string, 'amp_first' ) ) { - $url = amp_remove_paired_endpoint( $url ); - } - $added_query_vars = [ self::VALIDATE_QUERY_VAR => self::get_amp_validate_nonce(), self::CACHE_BUST_QUERY_VAR => wp_rand(), diff --git a/src/PairedRouting.php b/src/PairedRouting.php index e1ce10648b0..2fd8b1691d3 100644 --- a/src/PairedRouting.php +++ b/src/PairedRouting.php @@ -915,34 +915,6 @@ public function has_endpoint( $url = '' ) { return $this->get_paired_url_structure()->has_endpoint( $url ); } - /** - * Determine if a given URL needs to be paired to serve AMP (in other words, if the URL is not AMP-first). - * - * @since 2.2 - * - * @param string $url URL to examine. If empty, will use the current URL. - * @return bool True if URL needs paired endpoint to serve AMP. False if the URL is AMP-first (canonical). - */ - public function needs_endpoint( $url = '' ) { - if ( ! $url ) { - $url = amp_get_current_url(); - } - - if ( $this->has_endpoint( $url ) ) { - $url = $this->remove_endpoint( $url ); - } - - /** - * Filters whether the given URL has a paired endpoint to access the AMP version. - * - * @since 2.2 - * - * @param bool $needs_paired Needs paired. - * @param string $url URL. - */ - return (bool) apply_filters( 'amp_url_needs_paired_endpoint', ! amp_is_canonical(), $url ); - } - /** * Turn a given URL into a paired AMP URL. * @@ -950,10 +922,6 @@ public function needs_endpoint( $url = '' ) { * @return string AMP URL. */ public function add_endpoint( $url ) { - if ( ! $this->needs_endpoint( $url ) ) { - return $url; - } - return $this->get_paired_url_structure()->add_endpoint( $url ); } diff --git a/src/PairedUrlStructure.php b/src/PairedUrlStructure.php index 7630cb4eadf..3a83743efc0 100644 --- a/src/PairedUrlStructure.php +++ b/src/PairedUrlStructure.php @@ -41,16 +41,6 @@ public function has_endpoint( $url ) { return $url !== $this->remove_endpoint( $url ); } - /** - * Determine if the URL would need an endpoint. - * - * @param string $url URL (or REQUEST_URI). - * @return bool URL with AMP stripped. - */ - public function needs_endpoint( $url ) { - return true; - } - /** * Turn a given URL into a paired AMP URL. * diff --git a/src/QueryVar.php b/src/QueryVar.php index 191a4d7bba0..9174fbad86c 100644 --- a/src/QueryVar.php +++ b/src/QueryVar.php @@ -32,7 +32,7 @@ interface QueryVar { const AMP = 'amp'; /** - * Query var which forces the template mode to be Standard (aka AMP-first aka canonical). + * Query var which overrides the template mode to be Standard (aka AMP-first aka canonical). * * This is only honored during validation requests or if the user has validation capability. * @@ -40,7 +40,7 @@ interface QueryVar { * @see \AMP_Validation_Manager::has_cap() * @var string */ - const AMP_FIRST = 'amp_first'; + const AMP_FIRST = 'amp-first'; /** * Query var used to signal the request for an non-AMP page. From 3eadf15f8fa6442fa0c1ac98f7769cde168def4b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 24 Sep 2021 22:20:59 -0700 Subject: [PATCH 09/19] Revert "Add amp_available_override query param to force AMP to be available" This reverts commit 808d969f0306dab585be139b98983be3ce346d96. --- includes/amp-helper-functions.php | 9 --------- src/QueryVar.php | 13 ------------- 2 files changed, 22 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index cff09bc681f..d7fbc6f0d4a 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -465,15 +465,6 @@ function amp_is_available() { return false; } - // Allow query parameter to force AMP to be available, depending on whether it's a validation request or the user can do validation. - if ( - isset( $_GET[ QueryVar::AMP_AVAILABLE_OVERRIDE ] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended - && - ( AMP_Validation_Manager::is_validate_request() || AMP_Validation_Manager::has_cap() ) - ) { - return true; - } - // Ensure that all templates can be accessed in AMP when a Reader theme is selected. $has_reader_theme = ( AMP_Theme_Support::READER_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) diff --git a/src/QueryVar.php b/src/QueryVar.php index 9174fbad86c..83564806bb3 100644 --- a/src/QueryVar.php +++ b/src/QueryVar.php @@ -75,19 +75,6 @@ interface QueryVar { */ const NOAMP_AVAILABLE = 'available'; - /** - * Whether AMP should be forcibly be made available for the given request. - * - * This causes the supportable templates to be disregarded as well as whether AMP is disabled for a given post. - * - * This is only honored during validation requests or if the user has validation capability. - * - * @see \AMP_Validation_Manager::is_validate_request() - * @see \AMP_Validation_Manager::has_cap() - * @var string - */ - const AMP_AVAILABLE_OVERRIDE = 'amp_available_override'; - /** * Value for the query var that allows switching to verbose server-timing output. * From 8a61a749db9e3f81be7d33e1683684275c460c04 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 24 Sep 2021 22:30:29 -0700 Subject: [PATCH 10/19] Move validate logic to validation manager --- includes/class-amp-theme-support.php | 44 +-------------- .../class-amp-validation-manager.php | 55 +++++++++++++++++++ 2 files changed, 56 insertions(+), 43 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index e60ee91be36..b7bdd807a5d 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -1998,49 +1998,7 @@ public static function prepare_response( $response, $args = [] ) { // Respond early with results if performing a validate request. if ( AMP_Validation_Manager::is_validate_request() ) { - status_header( 200 ); - header( 'Content-Type: application/json; charset=utf-8' ); - $data = [ - 'http_status_code' => $status_code, - 'php_fatal_error' => false, - ]; - if ( $last_error && in_array( $last_error['type'], [ E_ERROR, E_RECOVERABLE_ERROR, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_PARSE ], true ) ) { - $data['php_fatal_error'] = $last_error; - } - $data = array_merge( $data, AMP_Validation_Manager::get_validate_response_data( $sanitization_results ) ); - - if ( isset( $_GET[ AMP_Validation_Manager::STORE_QUERY_VAR ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended - $validation_errors = wp_list_pluck( $data['results'], 'error' ); - unset( $data['results'] ); - - $validated_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors( - $validation_errors, - amp_get_current_url(), - $data - ); - if ( is_wp_error( $validated_url_post_id ) ) { - status_header( 500 ); - - $result = []; - foreach ( $validated_url_post_id->errors as $code => $messages ) { - foreach ( $messages as $message ) { - $result[] = [ - 'code' => $code, - 'message' => $message, - ]; - } - } - return wp_json_encode( $result ); - } else { - status_header( 201 ); - $data['validated_url_post'] = [ - 'id' => $validated_url_post_id, - 'edit_link' => get_edit_post_link( $validated_url_post_id, 'raw' ), - ]; - } - } - - return wp_json_encode( $data, JSON_UNESCAPED_SLASHES ); + return AMP_Validation_Manager::send_validate_response( $sanitization_results, $status_code, $last_error ); } /** diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 819f8c66570..821e3a42b10 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -1632,6 +1632,61 @@ public static function remove_illegal_source_stack_comments( Document $dom ) { } } + /** + * Send validate response. + * + * @param array $sanitization_results Sanitization results. + * @param int $status_code Status code. + * @param array|null $last_error Last error. + * + * @return string JSON. + */ + public static function send_validate_response( $sanitization_results, $status_code, $last_error ) { + status_header( 200 ); + header( 'Content-Type: application/json; charset=utf-8' ); + $data = [ + 'http_status_code' => $status_code, + 'php_fatal_error' => false, + ]; + if ( $last_error && in_array( $last_error['type'], [ E_ERROR, E_RECOVERABLE_ERROR, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_PARSE ], true ) ) { + $data['php_fatal_error'] = $last_error; + } + $data = array_merge( $data, self::get_validate_response_data( $sanitization_results ) ); + + if ( isset( $_GET[ self::STORE_QUERY_VAR ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended + $validation_errors = wp_list_pluck( $data['results'], 'error' ); + unset( $data['results'] ); + + $validated_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors( + $validation_errors, + amp_get_current_url(), + $data + ); + if ( is_wp_error( $validated_url_post_id ) ) { + status_header( 500 ); + + $result = []; + foreach ( $validated_url_post_id->errors as $code => $messages ) { + foreach ( $messages as $message ) { + $result[] = [ + 'code' => $code, + 'message' => $message, + ]; + } + } + return wp_json_encode( $result ); + } else { + status_header( 201 ); + $data['validated_url_post'] = [ + 'id' => $validated_url_post_id, + 'edit_link' => get_edit_post_link( $validated_url_post_id, 'raw' ), + ]; + } + } + + return wp_json_encode( $data, JSON_UNESCAPED_SLASHES ); + } + /** * Finalize validation. * From b3a12bd76ce5b25300902b7e46078311729b297d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 27 Sep 2021 11:20:20 -0700 Subject: [PATCH 11/19] Improve send_validate_response logic and add test coverage --- includes/class-amp-theme-support.php | 7 +- .../class-amp-validation-manager.php | 34 +++--- tests/php/test-class-amp-theme-support.php | 56 +++++++++- .../test-class-amp-validation-manager.php | 102 ++++++++++++++++++ 4 files changed, 180 insertions(+), 19 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index b7bdd807a5d..ca6640fb1b2 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -1998,7 +1998,12 @@ public static function prepare_response( $response, $args = [] ) { // Respond early with results if performing a validate request. if ( AMP_Validation_Manager::is_validate_request() ) { - return AMP_Validation_Manager::send_validate_response( $sanitization_results, $status_code, $last_error ); + return AMP_Validation_Manager::send_validate_response( + $sanitization_results, + $status_code, + $last_error, + isset( $_GET[ AMP_Validation_Manager::STORE_QUERY_VAR ] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended + ); } /** diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index ef7380d8d61..a6c96fc04c5 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -1707,15 +1707,20 @@ public static function remove_illegal_source_stack_comments( Document $dom ) { /** * Send validate response. * + * @since 2.2 + * @see AMP_Theme_Support::prepare_response() + * * @param array $sanitization_results Sanitization results. * @param int $status_code Status code. * @param array|null $last_error Last error. - * + * @param bool $store Whether to store the results. * @return string JSON. */ - public static function send_validate_response( $sanitization_results, $status_code, $last_error ) { + public static function send_validate_response( $sanitization_results, $status_code, $last_error, $store ) { status_header( 200 ); - header( 'Content-Type: application/json; charset=utf-8' ); + if ( ! headers_sent() ) { + header( 'Content-Type: application/json; charset=utf-8' ); + } $data = [ 'http_status_code' => $status_code, 'php_fatal_error' => false, @@ -1725,9 +1730,11 @@ public static function send_validate_response( $sanitization_results, $status_co } $data = array_merge( $data, self::get_validate_response_data( $sanitization_results ) ); - if ( isset( $_GET[ self::STORE_QUERY_VAR ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended + if ( $store ) { $validation_errors = wp_list_pluck( $data['results'], 'error' ); - unset( $data['results'] ); + + $args = $data; + unset( $args['results'] ); $validated_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors( $validation_errors, @@ -1736,17 +1743,12 @@ public static function send_validate_response( $sanitization_results, $status_co ); if ( is_wp_error( $validated_url_post_id ) ) { status_header( 500 ); - - $result = []; - foreach ( $validated_url_post_id->errors as $code => $messages ) { - foreach ( $messages as $message ) { - $result[] = [ - 'code' => $code, - 'message' => $message, - ]; - } - } - return wp_json_encode( $result ); + return wp_json_encode( + [ + 'code' => $validated_url_post_id->get_error_code(), + 'message' => $validated_url_post_id->get_error_message(), + ] + ); } else { status_header( 201 ); $data['validated_url_post'] = [ diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php index 42bd7bfaf84..15e40425d5e 100644 --- a/tests/php/test-class-amp-theme-support.php +++ b/tests/php/test-class-amp-theme-support.php @@ -1885,11 +1885,11 @@ public function test_prepare_response_for_submitted_form() { } /** - * Test prepare_response when validating an invalid AMP page. + * Test prepare_response when validating a non-AMP page. * * @covers AMP_Theme_Support::prepare_response() */ - public function test_prepare_response_for_validating_invalid_amp_page() { + public function test_prepare_response_for_validating_non_amp_page() { $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', true ); $response = AMP_Theme_Support::prepare_response( '' ); @@ -1897,6 +1897,58 @@ public function test_prepare_response_for_validating_invalid_amp_page() { $this->assertStringContainsString( 'RENDERED_PAGE_NOT_AMP', $response ); } + /** @return array */ + public function get_data_to_test_prepare_response_for_validating_amp_page() { + return [ + 'no-store' => [ + false, + ], + 'store' => [ + true, + ], + ]; + } + + /** + * Test prepare_response when validating an AMP page. + * + * @dataProvider get_data_to_test_prepare_response_for_validating_amp_page + * @covers AMP_Theme_Support::prepare_response() + * @covers AMP_Validation_Manager::send_validate_response() + * + * @param bool $store Whether to store results. + */ + public function test_prepare_response_for_validating_amp_page( $store ) { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $this->set_template_mode( AMP_Theme_Support::STANDARD_MODE_SLUG ); + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', true ); + $this->go_to( '/' ); + + if ( $store ) { + $_GET[ AMP_Validation_Manager::STORE_QUERY_VAR ] = ''; + } + $response = AMP_Theme_Support::prepare_response( '' ); + $this->assertJson( $response ); + $data = json_decode( $response, true ); + $this->assertArrayHasKey( 'http_status_code', $data ); + $this->assertArrayHasKey( 'php_fatal_error', $data ); + $this->assertArrayHasKey( 'queried_object', $data ); + $this->assertArrayHasKey( 'url', $data ); + $this->assertArrayHasKey( 'stylesheets', $data ); + $this->assertArrayHasKey( 'results', $data ); + $this->assertCount( 1, $data['results'] ); + $this->assertEquals( 'SPECIFIED_LAYOUT_INVALID', $data['results'][0]['error']['code'] ); + + if ( $store ) { + $this->assertArrayHasKey( 'validated_url_post', $data ); + $this->assertArrayHasKey( 'id', $data['validated_url_post'] ); + $this->assertArrayHasKey( 'edit_link', $data['validated_url_post'] ); + $this->assertEquals( AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, get_post_type( $data['validated_url_post']['id'] ) ); + } else { + $this->assertArrayNotHasKey( 'validated_url_post', $data ); + } + } + /** * Initializes and returns the original HTML. */ diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 58e10a1182f..6b2a8023e7b 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -1970,6 +1970,108 @@ public function test_remove_illegal_source_stack_comments() { $this->assertEquals( '/* start custom scripts! */document.write("hello!")/* end custom scripts! */', $dom->getElementById( 'third' )->textContent ); } + public function get_data_to_test_send_validate_response() { + return [ + 'ok_no_error_no_store' => [ + 'status_code' => 200, + 'last_error' => null, + 'store' => false, + 'save_error' => false, + ], + 'fatal_error_store' => [ + 'status_code' => 500, + 'last_error' => [ + 'type' => E_ERROR, + 'message' => 'Something bad happened.', + 'file' => __FILE__, + 'line' => __LINE__, + ], + 'store' => true, + 'save_error' => false, + ], + 'warning_store' => [ + 'status_code' => 200, + 'last_error' => [ + 'type' => E_WARNING, + 'message' => 'Something kinda bad happened.', + 'file' => __FILE__, + 'line' => __LINE__, + ], + 'store' => true, + 'save_error' => false, + ], + 'store_failure' => [ + 'status_code' => 200, + 'last_error' => null, + 'store' => true, + 'save_error' => true, + ], + ]; + } + + /** + * @dataProvider get_data_to_test_send_validate_response + * @covers \AMP_Validation_Manager::send_validate_response() + */ + public function test_send_validate_response( $status_code, $last_error, $store, $save_error ) { + $source_html = ''; + $sanitizer_classes = amp_get_content_sanitizers(); + $sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( $sanitizer_classes ); + $sanitization_results = AMP_Content_Sanitizer::sanitize_document( + AMP_DOM_Utils::get_dom_from_content( $source_html ), + $sanitizer_classes, + [] + ); + + if ( $save_error ) { + add_filter( 'wp_insert_post_empty_content', '__return_true' ); + } + + $response = AMP_Validation_Manager::send_validate_response( $sanitization_results, $status_code, $last_error, $store ); + $this->assertJson( $response ); + $data = json_decode( $response, true ); + + if ( $save_error ) { + $this->assertSame( + [ + 'code' => 'empty_content', + 'message' => 'Content, title, and excerpt are empty.', + ], + $data + ); + return; + } + + $this->assertArrayHasKey( 'http_status_code', $data ); + $this->assertArrayHasKey( 'php_fatal_error', $data ); + $this->assertArrayHasKey( 'queried_object', $data ); + $this->assertArrayHasKey( 'url', $data ); + $this->assertArrayHasKey( 'stylesheets', $data ); + $this->assertArrayHasKey( 'results', $data ); + + $this->assertEquals( $status_code, $data['http_status_code'] ); + if ( $last_error && in_array( $last_error['type'], [ E_ERROR, E_RECOVERABLE_ERROR, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_PARSE ], true ) ) { + $this->assertIsArray( $data['php_fatal_error'] ); + $this->assertEquals( $last_error, $data['php_fatal_error'] ); + } else { + $this->assertFalse( $data['php_fatal_error'] ); + } + + $this->assertCount( 1, $data['results'] ); + $this->assertEquals( 'SPECIFIED_LAYOUT_INVALID', $data['results'][0]['error']['code'] ); + + $this->assertCount( 1, $data['stylesheets'] ); + + if ( $store ) { + $this->assertArrayHasKey( 'validated_url_post', $data ); + $this->assertArrayHasKey( 'id', $data['validated_url_post'] ); + $this->assertArrayHasKey( 'edit_link', $data['validated_url_post'] ); + $this->assertEquals( AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, get_post_type( $data['validated_url_post']['id'] ) ); + } else { + $this->assertArrayNotHasKey( 'validated_url_post', $data ); + } + } + /** * Test finalize_validation. * From c24e0ef243add50121151ed192a3733ce905e130 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 27 Sep 2021 12:12:37 -0700 Subject: [PATCH 12/19] Add test coverage for AMP-first override requests --- .../class-amp-validation-manager.php | 20 +- .../test-class-amp-validation-manager.php | 198 ++++++++++++++++++ 2 files changed, 212 insertions(+), 6 deletions(-) diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index a6c96fc04c5..8ca69e53726 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -237,16 +237,24 @@ static function() { if ( ! amp_is_canonical() ) { add_filter( 'option_' . AMP_Options_Manager::OPTION_NAME, - static function ( $options ) { - if ( self::is_amp_first_override_request() ) { - $options[ Option::THEME_SUPPORT ] = AMP_Theme_Support::STANDARD_MODE_SLUG; - } - return $options; - } + [ __CLASS__, 'filter_options_for_standard_mode_when_amp_first_override' ] ); } } + /** + * Filter AMP options to set Standard template mode if it is an AMP-override request. + * + * @param array $options Options. + * @return array Filtered options. + */ + public static function filter_options_for_standard_mode_when_amp_first_override( $options ) { + if ( self::is_amp_first_override_request() ) { + $options[ Option::THEME_SUPPORT ] = AMP_Theme_Support::STANDARD_MODE_SLUG; + } + return $options; + } + /** * Determine whether the request includes the AMP-first override. * diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 6b2a8023e7b..cf56cf97b29 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -144,6 +144,7 @@ public function tearDown() { AMP_Validation_Manager::$hook_source_stack = []; AMP_Validation_Manager::$validation_results = []; AMP_Validation_Manager::reset_validation_results(); + unset( $_REQUEST['post_type'] ); // phpcs:ignore parent::tearDown(); } @@ -168,6 +169,203 @@ public function test_init() { $this->assertEquals( 10, has_action( 'wp', [ self::TESTED_CLASS, 'maybe_fail_validate_request' ] ) ); $this->assertEquals( 10, has_action( 'wp', [ self::TESTED_CLASS, 'override_validation_error_statuses' ] ) ); + + $this->assertEquals( + 10, + has_filter( + 'option_' . AMP_Options_Manager::OPTION_NAME, + [ self::TESTED_CLASS, 'filter_options_for_standard_mode_when_amp_first_override' ] + ) + ); + } + + /** @return array */ + public function get_data_to_test_filter_options_for_standard_mode_when_amp_first_override() { + $set_query_var = static function () { + $_GET[ QueryVar::AMP_FIRST ] = ''; + }; + $set_admin_user = static function () { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + }; + $set_validate_request = function () { + $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', true ); + }; + + $set_admin_dashboard = static function () { + set_current_screen( 'index.php' ); + }; + + $set_global_validated_url_post = static function ( $url ) { + $GLOBALS['post'] = self::factory()->post->create_and_get( + [ + 'post_title' => $url, + 'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, + ] + ); + setup_postdata( $GLOBALS['post'] ); + }; + + $set_validated_url_post_list_screen = static function () { + $GLOBALS['hook_suffix'] = 'edit.php'; + $_REQUEST['post_type'] = AMP_Validated_URL_Post_Type::POST_TYPE_SLUG; + set_current_screen(); + }; + + $set_validated_url_post_edit_screen = static function () { + $GLOBALS['hook_suffix'] = 'post.php'; + $_REQUEST['post_type'] = AMP_Validated_URL_Post_Type::POST_TYPE_SLUG; + set_current_screen(); + }; + + return [ + 'frontend_no_query_var' => [ + 'set_up' => static function () {}, + 'expect_override' => false, + ], + 'frontend_query_var_not_allowed' => [ + 'set_up' => $set_query_var, + 'expect_override' => false, + ], + 'frontend_query_var_with_admin_user' => [ + 'set_up' => static function () use ( $set_query_var, $set_admin_user ) { + $set_query_var(); + $set_admin_user(); + }, + 'expect_override' => true, + ], + 'frontend_query_var_with_validate_request' => [ + 'set_up' => static function () use ( $set_query_var, $set_validate_request ) { + $set_query_var(); + $set_validate_request(); + }, + 'expect_override' => true, + ], + 'frontend_query_var_with_admin_user_and_validate_request' => [ + 'set_up' => static function () use ( $set_query_var, $set_admin_user, $set_validate_request ) { + $set_query_var(); + $set_admin_user(); + $set_validate_request(); + }, + 'expect_override' => true, + ], + 'frontend_no_query_var_with_admin_user_and_validate_request' => [ + 'set_up' => static function () use ( $set_query_var, $set_admin_user, $set_validate_request ) { + $set_admin_user(); + $set_validate_request(); + }, + 'expect_override' => false, + ], + + 'admin_validation_request_for_new_non_override_url' => [ + 'set_up' => static function () use ( $set_admin_dashboard, $set_admin_user ) { + $set_admin_user(); + $set_admin_dashboard(); + $_GET['action'] = AMP_Validation_Manager::VALIDATE_QUERY_VAR; + $_GET['url'] = home_url(); + }, + 'expect_override' => false, + ], + + 'admin_validation_request_for_new_yes_override_url' => [ + 'set_up' => static function () use ( $set_admin_dashboard, $set_admin_user ) { + $set_admin_user(); + $set_admin_dashboard(); + $_GET['action'] = AMP_Validation_Manager::VALIDATE_QUERY_VAR; + $_GET['url'] = add_query_arg( QueryVar::AMP_FIRST, '', home_url( '/' ) ); + }, + 'expect_override' => true, + ], + + 'admin_validation_request_for_existing_non_override_url' => [ + 'set_up' => static function () use ( $set_admin_dashboard, $set_admin_user ) { + $set_admin_user(); + $set_admin_dashboard(); + $_GET['action'] = AMP_Validation_Manager::VALIDATE_QUERY_VAR; + $_GET['post'] = self::factory()->post->create( + [ + 'post_title' => home_url(), + 'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, + ] + ); + }, + 'expect_override' => false, + ], + + 'admin_validation_request_for_existing_yes_override_url' => [ + 'set_up' => static function () use ( $set_admin_dashboard, $set_admin_user ) { + $set_admin_user(); + $set_admin_dashboard(); + $_GET['action'] = AMP_Validation_Manager::VALIDATE_QUERY_VAR; + $_GET['post'] = self::factory()->post->create( + [ + 'post_title' => add_query_arg( QueryVar::AMP_FIRST, '', home_url( '/' ) ), + 'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, + ] + ); + }, + 'expect_override' => true, + ], + + 'admin_on_dashboard' => [ + 'set_up' => static function () use ( $set_admin_user, $set_admin_dashboard ) { + $set_admin_user(); + $set_admin_dashboard(); + }, + 'expect_override' => false, + ], + 'admin_on_post_list_screen_not_amp_override_url' => [ + 'set_up' => static function () use ( $set_admin_user, $set_global_validated_url_post, $set_validated_url_post_list_screen ) { + $set_admin_user(); + $set_global_validated_url_post( home_url( '/' ) ); + $set_validated_url_post_list_screen(); + }, + 'expect_override' => false, + ], + 'admin_on_post_list_screen_yes_amp_override_url' => [ + 'set_up' => static function () use ( $set_admin_user, $set_global_validated_url_post, $set_validated_url_post_list_screen ) { + $set_admin_user(); + $set_global_validated_url_post( add_query_arg( QueryVar::AMP_FIRST, '', home_url( '/' ) ) ); + $set_validated_url_post_list_screen(); + }, + 'expect_override' => true, + ], + + 'admin_on_edit_post_screen_not_amp_override_url' => [ + 'set_up' => static function () use ( $set_admin_user, $set_global_validated_url_post, $set_validated_url_post_edit_screen ) { + $set_admin_user(); + $set_global_validated_url_post( home_url( '/' ) ); + $set_validated_url_post_edit_screen(); + }, + 'expect_override' => false, + ], + + 'admin_on_edit_post_screen_not_amp_override_url' => [ + 'set_up' => static function () use ( $set_admin_user, $set_global_validated_url_post, $set_validated_url_post_edit_screen ) { + $set_admin_user(); + $set_global_validated_url_post( add_query_arg( QueryVar::AMP_FIRST, '', home_url( '/' ) ) ); + $set_validated_url_post_edit_screen(); + }, + 'expect_override' => true, + ], + ]; + } + + /** + * @dataProvider get_data_to_test_filter_options_for_standard_mode_when_amp_first_override + * @covers AMP_Validation_Manager::is_amp_first_override_request() + * @covers AMP_Validation_Manager::filter_options_for_standard_mode_when_amp_first_override() + * @covers AMP_Validation_Manager::is_amp_first_override_url() + */ + public function test_filter_options_for_standard_mode_when_amp_first_override( $set_up, $expect_override ) { + $set_up(); + + $options_with_reader = [ Option::THEME_SUPPORT => AMP_Theme_Support::READER_MODE_SLUG ]; + $options_with_standard = [ Option::THEME_SUPPORT => AMP_Theme_Support::STANDARD_MODE_SLUG ]; + + $this->assertEquals( + $expect_override ? $options_with_standard : $options_with_reader, + AMP_Validation_Manager::filter_options_for_standard_mode_when_amp_first_override( $options_with_reader ) + ); } /** From 06284dfce41dd5b1fdcb3298ed0cce6bb850c4a3 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Oct 2021 16:33:45 -0700 Subject: [PATCH 13/19] Introduce args for validate requests to cache, serve cached, and omit stylesheets --- includes/class-amp-theme-support.php | 2 +- .../class-amp-validated-url-post-type.php | 2 +- .../class-amp-validation-manager.php | 173 ++++++++++++++++-- tests/php/src/PluginSuppressionTest.php | 6 +- tests/php/test-class-amp-theme-support.php | 7 +- .../test-class-amp-validation-manager.php | 11 +- 6 files changed, 173 insertions(+), 28 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index ca6640fb1b2..4e0679a759a 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2002,7 +2002,7 @@ public static function prepare_response( $response, $args = [] ) { $sanitization_results, $status_code, $last_error, - isset( $_GET[ AMP_Validation_Manager::STORE_QUERY_VAR ] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended + AMP_Validation_Manager::get_validate_request_args() ); } diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index 83408cc44b2..10c140b6e3b 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -765,7 +765,7 @@ protected static function normalize_url_for_storage( $url ) { // Query args to be removed from validated URLs. $removable_query_vars = array_merge( wp_removable_query_args(), - [ 'preview_id', 'preview_nonce', 'preview', QueryVar::NOAMP, AMP_Validation_Manager::VALIDATE_QUERY_VAR, AMP_Validation_Manager::STORE_QUERY_VAR ] + [ 'preview_id', 'preview_nonce', 'preview', QueryVar::NOAMP, AMP_Validation_Manager::VALIDATE_QUERY_VAR ] ); // Normalize query args, removing all that are not recognized or which are removable. diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 8ca69e53726..2fd25524b68 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -31,11 +31,40 @@ class AMP_Validation_Manager { const VALIDATE_QUERY_VAR = 'amp_validate'; /** - * Query param that indicates a validation request should store the results. + * Key for amp_validate query var array for nonce to authorize validation. * * @var string */ - const STORE_QUERY_VAR = 'amp_store'; + const VALIDATE_QUERY_VAR_NONCE = 'nonce'; + + /** + * Key for amp_validate query var array for whether to store the validation results in an amp_validated_url post. + * + * @var string + */ + const VALIDATE_QUERY_VAR_CACHE = 'cache'; + + /** + * Key for amp_validate query var array for whether to return previously-stored the validation results if an + * amp_validated_url post exists for the URL and it is not stale. + * + * @var string + */ + const VALIDATE_QUERY_VAR_CACHED_IF_FRESH = 'cached_if_fresh'; + + /** + * Key for amp_validate query var array for whether to omit stylesheets data. + * + * @var string + */ + const VALIDATE_QUERY_VAR_OMIT_STYLESHEETS = 'omit_stylesheets'; + + /** + * Key for amp_validate query var array to bust the cache. + * + * @var string + */ + const VALIDATE_QUERY_VAR_CACHE_BUST = 'cache_bust'; /** * Meta capability for validation. @@ -62,13 +91,6 @@ class AMP_Validation_Manager { */ const VALIDATION_ERROR_TERM_STATUS_QUERY_VAR = 'amp_validation_error_term_status'; - /** - * Query var for cache-busting. - * - * @var string - */ - const CACHE_BUST_QUERY_VAR = 'amp_cache_bust'; - /** * Transient key to store validation errors when activating a plugin. * @@ -628,6 +650,56 @@ public static function is_validate_request() { return self::$is_validate_request; } + /** + * Get validate request args. + * + * @return array|null { + * Args or null if the query var was not supplied. + * + * @type string $nonce None to authorize validate request. + * @type bool $cache Whether to store results in amp_validated_url post. + * @type bool $cached_if_fresh Whether to return previously-stored results if not stale. + * @type bool $omit_stylesheets Whether to omit stylesheet data in the validate response. + * } + */ + public static function get_validate_request_args() { + if ( ! isset( $_GET[ self::VALIDATE_QUERY_VAR ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended + return null; + } + + $unsanitized_values = $_GET[ self::VALIDATE_QUERY_VAR ]; // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + + if ( is_string( $unsanitized_values ) ) { + $unsanitized_values = [ + self::VALIDATE_QUERY_VAR_NONCE => $unsanitized_values, + ]; + } + + $defaults = [ + self::VALIDATE_QUERY_VAR_NONCE => null, + self::VALIDATE_QUERY_VAR_CACHE => false, + self::VALIDATE_QUERY_VAR_CACHED_IF_FRESH => false, + self::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS => false, + ]; + + if ( ! is_array( $unsanitized_values ) ) { + return $defaults; + } + + $args = $defaults; + foreach ( $unsanitized_values as $key => $unsanitized_value ) { + switch ( $key ) { + case self::VALIDATE_QUERY_VAR_NONCE: + $args[ $key ] = sanitize_key( $unsanitized_value ); + break; + default: + $args[ $key ] = rest_sanitize_boolean( $unsanitized_value ); + } + } + + return $args; + } + /** * Initialize a validate request. * @@ -642,6 +714,60 @@ public static function init_validate_request() { $should_validate_response = self::should_validate_response(); if ( true === $should_validate_response ) { + + $args = self::get_validate_request_args(); + if ( $args[ self::VALIDATE_QUERY_VAR_CACHED_IF_FRESH ] ) { + $post = AMP_Validated_URL_Post_Type::get_invalid_url_post( amp_get_current_url() ); + if ( $post instanceof WP_Post && count( AMP_Validated_URL_Post_Type::get_post_staleness( $post ) ) === 0 ) { + + $response = [ + 'http_status_code' => 200, // Note: This is not currently cached in postmeta. + 'php_fatal_error' => false, + 'results' => [], + 'queried_object' => null, + 'url' => null, + 'validated_url_post' => [ + 'id' => $post->ID, + 'edit_link' => get_edit_post_link( $post->ID, 'raw' ), // @todo This is too early because the user has not been logged-in yet. + 'staleness' => AMP_Validated_URL_Post_Type::get_post_staleness( $post ), + ], + ]; + + if ( ! $args[ self::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) { + $stylesheets = get_post_meta( $post->ID, AMP_Validated_URL_Post_Type::STYLESHEETS_POST_META_KEY, true ); + if ( $stylesheets ) { + $response['stylesheets'] = json_decode( $stylesheets, true ); + } + } + + $stored_validation_errors = json_decode( $post->post_content, true ); + if ( is_array( $stored_validation_errors ) ) { + $response['results'] = array_map( + static function ( $stored_validation_error ) { + $error = $stored_validation_error['data']; + $sanitized = AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $error ); + return compact( 'error', 'sanitized' ); + }, + $stored_validation_errors + ); + } + + $queried_object = get_post_meta( $post->ID, AMP_Validated_URL_Post_Type::QUERIED_OBJECT_POST_META_KEY, true ); + if ( $queried_object ) { + $response['queried_object'] = $queried_object; + } + + $php_fatal_error = get_post_meta( $post->ID, AMP_Validated_URL_Post_Type::PHP_FATAL_ERROR_POST_META_KEY, true ); + if ( $php_fatal_error ) { + $response['php_fatal_error'] = $php_fatal_error; + } + + $response['url'] = AMP_Validated_URL_Post_Type::get_url_from_post( $post ); + + wp_send_json( $response, 200, JSON_UNESCAPED_SLASHES ); + } + } + self::add_validation_error_sourcing(); self::$is_validate_request = true; @@ -1608,12 +1734,13 @@ public static function get_amp_validate_nonce() { * validate response should be served. */ public static function should_validate_response() { - if ( ! isset( $_GET[ self::VALIDATE_QUERY_VAR ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended + $args = self::get_validate_request_args(); + + if ( ! $args ) { return false; } - $validate_key = wp_unslash( $_GET[ self::VALIDATE_QUERY_VAR ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized - if ( ! hash_equals( self::get_amp_validate_nonce(), $validate_key ) ) { + if ( ! hash_equals( self::get_amp_validate_nonce(), $args[ self::VALIDATE_QUERY_VAR_NONCE ] ) ) { return new WP_Error( 'http_request_failed', __( 'Nonce authentication failed.', 'amp' ) @@ -1721,10 +1848,10 @@ public static function remove_illegal_source_stack_comments( Document $dom ) { * @param array $sanitization_results Sanitization results. * @param int $status_code Status code. * @param array|null $last_error Last error. - * @param bool $store Whether to store the results. + * @param array $request_args Request args. * @return string JSON. */ - public static function send_validate_response( $sanitization_results, $status_code, $last_error, $store ) { + public static function send_validate_response( $sanitization_results, $status_code, $last_error, $request_args ) { status_header( 200 ); if ( ! headers_sent() ) { header( 'Content-Type: application/json; charset=utf-8' ); @@ -1738,12 +1865,9 @@ public static function send_validate_response( $sanitization_results, $status_co } $data = array_merge( $data, self::get_validate_response_data( $sanitization_results ) ); - if ( $store ) { + if ( $request_args[ self::VALIDATE_QUERY_VAR_CACHE ] ) { $validation_errors = wp_list_pluck( $data['results'], 'error' ); - $args = $data; - unset( $args['results'] ); - $validated_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors( $validation_errors, amp_get_current_url(), @@ -1762,10 +1886,17 @@ public static function send_validate_response( $sanitization_results, $status_co $data['validated_url_post'] = [ 'id' => $validated_url_post_id, 'edit_link' => get_edit_post_link( $validated_url_post_id, 'raw' ), + 'staleness' => AMP_Validated_URL_Post_Type::get_post_staleness( $validated_url_post_id ), ]; } } + if ( $request_args[ self::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) { + unset( $data['stylesheets'] ); + } + + $data['url'] = remove_query_arg( self::VALIDATE_QUERY_VAR, $data['url'] ); + return wp_json_encode( $data, JSON_UNESCAPED_SLASHES ); } @@ -2063,8 +2194,10 @@ public static function validate_url( $url ) { } $added_query_vars = [ - self::VALIDATE_QUERY_VAR => self::get_amp_validate_nonce(), - self::CACHE_BUST_QUERY_VAR => wp_rand(), + self::VALIDATE_QUERY_VAR => [ + self::VALIDATE_QUERY_VAR_NONCE => self::get_amp_validate_nonce(), + self::VALIDATE_QUERY_VAR_CACHE_BUST => wp_rand(), + ], ]; // Ensure the URL to be validated is on the site. diff --git a/tests/php/src/PluginSuppressionTest.php b/tests/php/src/PluginSuppressionTest.php index db780110723..751797cc248 100644 --- a/tests/php/src/PluginSuppressionTest.php +++ b/tests/php/src/PluginSuppressionTest.php @@ -45,11 +45,13 @@ public function setUp() { add_filter( 'pre_http_request', function( $r, /** @noinspection PhpUnusedParameterInspection */ $args, $url ) { - if ( false === strpos( $url, 'amp_validate=' ) ) { + $url_query_vars = []; + parse_str( wp_parse_url( $url, PHP_URL_QUERY ), $url_query_vars ); + if ( ! array_key_exists( AMP_Validation_Manager::VALIDATE_QUERY_VAR, $url_query_vars ) ) { return $r; } - $this->attempted_validate_request_urls[] = remove_query_arg( [ 'amp_validate', 'amp_cache_bust' ], $url ); + $this->attempted_validate_request_urls[] = remove_query_arg( AMP_Validation_Manager::VALIDATE_QUERY_VAR, $url ); return [ 'body' => '', 'response' => [ diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php index 15e40425d5e..d166127dff9 100644 --- a/tests/php/test-class-amp-theme-support.php +++ b/tests/php/test-class-amp-theme-support.php @@ -1921,12 +1921,15 @@ public function get_data_to_test_prepare_response_for_validating_amp_page() { public function test_prepare_response_for_validating_amp_page( $store ) { wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); $this->set_template_mode( AMP_Theme_Support::STANDARD_MODE_SLUG ); - $this->set_private_property( AMP_Validation_Manager::class, 'is_validate_request', true ); $this->go_to( '/' ); + $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ] = [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), + ]; if ( $store ) { - $_GET[ AMP_Validation_Manager::STORE_QUERY_VAR ] = ''; + $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ][ AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE ] = 'true'; } + AMP_Validation_Manager::init_validate_request(); $response = AMP_Theme_Support::prepare_response( '' ); $this->assertJson( $response ); $data = json_decode( $response, true ); diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index cf56cf97b29..962249b5d9a 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -2225,7 +2225,11 @@ public function test_send_validate_response( $status_code, $last_error, $store, add_filter( 'wp_insert_post_empty_content', '__return_true' ); } - $response = AMP_Validation_Manager::send_validate_response( $sanitization_results, $status_code, $last_error, $store ); + $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ] = [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE => $store, + ]; + + $response = AMP_Validation_Manager::send_validate_response( $sanitization_results, $status_code, $last_error, AMP_Validation_Manager::get_validate_request_args() ); $this->assertJson( $response ); $data = json_decode( $response, true ); @@ -2614,7 +2618,10 @@ public function test_validate_url( $validation_errors, $after_matter ) { ]; $stylesheets = [ [ 'CSS!' ] ]; $filter = function( $pre, $r, $url ) use ( $validation_errors, $php_error, $queried_object, $stylesheets, $after_matter ) { - $this->assertStringContainsString( AMP_Validation_Manager::VALIDATE_QUERY_VAR . '=', $url ); + $url_query_vars = []; + parse_str( wp_parse_url( $url, PHP_URL_QUERY ), $url_query_vars ); + $this->assertArrayHasKey( AMP_Validation_Manager::VALIDATE_QUERY_VAR, $url_query_vars ); + $this->assertIsArray( $url_query_vars[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ] ); $validation = [ 'results' => [], From aa02b96da56f7285cae1d70bd62660144f0bcce7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Oct 2021 17:44:40 -0700 Subject: [PATCH 14/19] Move logic into maybe_send_cached_validate_response method --- includes/class-amp-theme-support.php | 3 +- .../class-amp-validation-manager.php | 173 ++++++++++-------- .../test-class-amp-validation-manager.php | 14 +- 3 files changed, 112 insertions(+), 78 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 4e0679a759a..3f8cc11fd90 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2001,8 +2001,7 @@ public static function prepare_response( $response, $args = [] ) { return AMP_Validation_Manager::send_validate_response( $sanitization_results, $status_code, - $last_error, - AMP_Validation_Manager::get_validate_request_args() + $last_error ); } diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 2fd25524b68..6030344e789 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -250,7 +250,8 @@ static function() { add_action( 'all_admin_notices', [ __CLASS__, 'print_plugin_notice' ] ); add_action( 'admin_bar_menu', [ __CLASS__, 'add_admin_bar_menu_items' ], 101 ); - add_action( 'wp', [ __CLASS__, 'maybe_fail_validate_request' ] ); + add_action( 'wp', [ __CLASS__, 'maybe_fail_validate_request' ], 10 ); + add_action( 'wp', [ __CLASS__, 'maybe_send_cached_validate_response' ], 20 ); add_action( 'wp', [ __CLASS__, 'override_validation_error_statuses' ] ); // Allow query parameter to force a response to be served with Standard mode (AMP-first). This query parameter @@ -653,18 +654,25 @@ public static function is_validate_request() { /** * Get validate request args. * - * @return array|null { - * Args or null if the query var was not supplied. + * @return array { + * Args. * - * @type string $nonce None to authorize validate request. - * @type bool $cache Whether to store results in amp_validated_url post. - * @type bool $cached_if_fresh Whether to return previously-stored results if not stale. - * @type bool $omit_stylesheets Whether to omit stylesheet data in the validate response. + * @type string|null $nonce None to authorize validate request or null if none was supplied. + * @type bool $cache Whether to store results in amp_validated_url post. + * @type bool $cached_if_fresh Whether to return previously-stored results if not stale. + * @type bool $omit_stylesheets Whether to omit stylesheet data in the validate response. * } */ - public static function get_validate_request_args() { + private static function get_validate_request_args() { + $defaults = [ + self::VALIDATE_QUERY_VAR_NONCE => null, + self::VALIDATE_QUERY_VAR_CACHE => false, + self::VALIDATE_QUERY_VAR_CACHED_IF_FRESH => false, + self::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS => false, + ]; + if ( ! isset( $_GET[ self::VALIDATE_QUERY_VAR ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended - return null; + return $defaults; } $unsanitized_values = $_GET[ self::VALIDATE_QUERY_VAR ]; // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized @@ -675,13 +683,6 @@ public static function get_validate_request_args() { ]; } - $defaults = [ - self::VALIDATE_QUERY_VAR_NONCE => null, - self::VALIDATE_QUERY_VAR_CACHE => false, - self::VALIDATE_QUERY_VAR_CACHED_IF_FRESH => false, - self::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS => false, - ]; - if ( ! is_array( $unsanitized_values ) ) { return $defaults; } @@ -714,60 +715,6 @@ public static function init_validate_request() { $should_validate_response = self::should_validate_response(); if ( true === $should_validate_response ) { - - $args = self::get_validate_request_args(); - if ( $args[ self::VALIDATE_QUERY_VAR_CACHED_IF_FRESH ] ) { - $post = AMP_Validated_URL_Post_Type::get_invalid_url_post( amp_get_current_url() ); - if ( $post instanceof WP_Post && count( AMP_Validated_URL_Post_Type::get_post_staleness( $post ) ) === 0 ) { - - $response = [ - 'http_status_code' => 200, // Note: This is not currently cached in postmeta. - 'php_fatal_error' => false, - 'results' => [], - 'queried_object' => null, - 'url' => null, - 'validated_url_post' => [ - 'id' => $post->ID, - 'edit_link' => get_edit_post_link( $post->ID, 'raw' ), // @todo This is too early because the user has not been logged-in yet. - 'staleness' => AMP_Validated_URL_Post_Type::get_post_staleness( $post ), - ], - ]; - - if ( ! $args[ self::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) { - $stylesheets = get_post_meta( $post->ID, AMP_Validated_URL_Post_Type::STYLESHEETS_POST_META_KEY, true ); - if ( $stylesheets ) { - $response['stylesheets'] = json_decode( $stylesheets, true ); - } - } - - $stored_validation_errors = json_decode( $post->post_content, true ); - if ( is_array( $stored_validation_errors ) ) { - $response['results'] = array_map( - static function ( $stored_validation_error ) { - $error = $stored_validation_error['data']; - $sanitized = AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $error ); - return compact( 'error', 'sanitized' ); - }, - $stored_validation_errors - ); - } - - $queried_object = get_post_meta( $post->ID, AMP_Validated_URL_Post_Type::QUERIED_OBJECT_POST_META_KEY, true ); - if ( $queried_object ) { - $response['queried_object'] = $queried_object; - } - - $php_fatal_error = get_post_meta( $post->ID, AMP_Validated_URL_Post_Type::PHP_FATAL_ERROR_POST_META_KEY, true ); - if ( $php_fatal_error ) { - $response['php_fatal_error'] = $php_fatal_error; - } - - $response['url'] = AMP_Validated_URL_Post_Type::get_url_from_post( $post ); - - wp_send_json( $response, 200, JSON_UNESCAPED_SLASHES ); - } - } - self::add_validation_error_sourcing(); self::$is_validate_request = true; @@ -1736,7 +1683,7 @@ public static function get_amp_validate_nonce() { public static function should_validate_response() { $args = self::get_validate_request_args(); - if ( ! $args ) { + if ( null === $args[ self::VALIDATE_QUERY_VAR_NONCE ] ) { return false; } @@ -1848,10 +1795,9 @@ public static function remove_illegal_source_stack_comments( Document $dom ) { * @param array $sanitization_results Sanitization results. * @param int $status_code Status code. * @param array|null $last_error Last error. - * @param array $request_args Request args. * @return string JSON. */ - public static function send_validate_response( $sanitization_results, $status_code, $last_error, $request_args ) { + public static function send_validate_response( $sanitization_results, $status_code, $last_error ) { status_header( 200 ); if ( ! headers_sent() ) { header( 'Content-Type: application/json; charset=utf-8' ); @@ -1865,7 +1811,9 @@ public static function send_validate_response( $sanitization_results, $status_co } $data = array_merge( $data, self::get_validate_response_data( $sanitization_results ) ); - if ( $request_args[ self::VALIDATE_QUERY_VAR_CACHE ] ) { + $args = self::get_validate_request_args(); + + if ( $args[ self::VALIDATE_QUERY_VAR_CACHE ] ) { $validation_errors = wp_list_pluck( $data['results'], 'error' ); $validated_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors( @@ -1891,7 +1839,7 @@ public static function send_validate_response( $sanitization_results, $status_co } } - if ( $request_args[ self::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) { + if ( $args[ self::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) { unset( $data['stylesheets'] ); } @@ -1900,6 +1848,81 @@ public static function send_validate_response( $sanitization_results, $status_co return wp_json_encode( $data, JSON_UNESCAPED_SLASHES ); } + /** + * Send cached validate response if it is requested and available. + * + * When a validate request is made with the `amp_validate[cached_if_fresh]=true` query parameter, before a page + * begins rendering a check is made for whether there is already an `amp_validated_url` post for the current URL. + * If there is, and the post is not stale, then the previous results are returned without re-rendering page and + * obtaining the validation data. + */ + public static function maybe_send_cached_validate_response() { + if ( ! self::is_validate_request() ) { + return; + } + $args = self::get_validate_request_args(); + + if ( ! $args[ self::VALIDATE_QUERY_VAR_CACHED_IF_FRESH ] ) { + return; + } + + $post = AMP_Validated_URL_Post_Type::get_invalid_url_post( amp_get_current_url() ); + if ( ! ( $post instanceof WP_Post ) ) { + return; + } + + $staleness = AMP_Validated_URL_Post_Type::get_post_staleness( $post ); + if ( count( $staleness ) > 0 ) { + return; + } + + $response = [ + 'http_status_code' => 200, // Note: This is not currently cached in postmeta. + 'php_fatal_error' => false, + 'results' => [], + 'queried_object' => null, + 'url' => null, + 'validated_url_post' => [ + 'id' => $post->ID, + 'edit_link' => get_edit_post_link( $post->ID, 'raw' ), + 'staleness' => $staleness, + ], + ]; + + if ( ! $args[ self::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) { + $stylesheets = get_post_meta( $post->ID, AMP_Validated_URL_Post_Type::STYLESHEETS_POST_META_KEY, true ); + if ( $stylesheets ) { + $response['stylesheets'] = json_decode( $stylesheets, true ); + } + } + + $stored_validation_errors = json_decode( $post->post_content, true ); + if ( is_array( $stored_validation_errors ) ) { + $response['results'] = array_map( + static function ( $stored_validation_error ) { + $error = $stored_validation_error['data']; + $sanitized = AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $error ); + return compact( 'error', 'sanitized' ); + }, + $stored_validation_errors + ); + } + + $queried_object = get_post_meta( $post->ID, AMP_Validated_URL_Post_Type::QUERIED_OBJECT_POST_META_KEY, true ); + if ( $queried_object ) { + $response['queried_object'] = $queried_object; + } + + $php_fatal_error = get_post_meta( $post->ID, AMP_Validated_URL_Post_Type::PHP_FATAL_ERROR_POST_META_KEY, true ); + if ( $php_fatal_error ) { + $response['php_fatal_error'] = $php_fatal_error; + } + + $response['url'] = AMP_Validated_URL_Post_Type::get_url_from_post( $post ); + + wp_send_json( $response, 200, JSON_UNESCAPED_SLASHES ); + } + /** * Finalize validation. * diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 962249b5d9a..64d660bb9f1 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -168,6 +168,7 @@ public function test_init() { $this->assertEquals( 101, has_action( 'admin_bar_menu', [ self::TESTED_CLASS, 'add_admin_bar_menu_items' ] ) ); $this->assertEquals( 10, has_action( 'wp', [ self::TESTED_CLASS, 'maybe_fail_validate_request' ] ) ); + $this->assertEquals( 20, has_action( 'wp', [ self::TESTED_CLASS, 'maybe_send_cached_validate_response' ] ) ); $this->assertEquals( 10, has_action( 'wp', [ self::TESTED_CLASS, 'override_validation_error_statuses' ] ) ); $this->assertEquals( @@ -2087,6 +2088,7 @@ public function test_get_amp_validate_nonce() { * Test should_validate_response. * * @covers AMP_Validation_Manager::should_validate_response() + * @covers AMP_Validation_Manager::get_validate_request_args() */ public function test_should_validate_response() { $this->assertFalse( AMP_Validation_Manager::should_validate_response() ); @@ -2168,6 +2170,7 @@ public function test_remove_illegal_source_stack_comments() { $this->assertEquals( '/* start custom scripts! */document.write("hello!")/* end custom scripts! */', $dom->getElementById( 'third' )->textContent ); } + /** @return array */ public function get_data_to_test_send_validate_response() { return [ 'ok_no_error_no_store' => [ @@ -2210,6 +2213,7 @@ public function get_data_to_test_send_validate_response() { /** * @dataProvider get_data_to_test_send_validate_response * @covers \AMP_Validation_Manager::send_validate_response() + * @covers \AMP_Validation_Manager::get_validate_request_args() */ public function test_send_validate_response( $status_code, $last_error, $store, $save_error ) { $source_html = ''; @@ -2229,7 +2233,7 @@ public function test_send_validate_response( $status_code, $last_error, $store, AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE => $store, ]; - $response = AMP_Validation_Manager::send_validate_response( $sanitization_results, $status_code, $last_error, AMP_Validation_Manager::get_validate_request_args() ); + $response = AMP_Validation_Manager::send_validate_response( $sanitization_results, $status_code, $last_error ); $this->assertJson( $response ); $data = json_decode( $response, true ); @@ -2274,6 +2278,14 @@ public function test_send_validate_response( $status_code, $last_error, $store, } } + /** + * @covers \AMP_Validation_Manager::maybe_send_cached_validate_response() + * @covers \AMP_Validation_Manager::get_validate_request_args() + */ + public function test_maybe_send_cached_validate_response() { + $this->markTestIncomplete(); + } + /** * Test finalize_validation. * From 4d12ffe3b6b3ec42f57e5696af8cbcd457ff5c30 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Oct 2021 20:01:19 -0700 Subject: [PATCH 15/19] Replace redundante staleness with revalidated prop --- includes/validation/class-amp-validation-manager.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 6030344e789..1b5415c43c5 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -1813,6 +1813,8 @@ public static function send_validate_response( $sanitization_results, $status_co $args = self::get_validate_request_args(); + $data['revalidated'] = true; + if ( $args[ self::VALIDATE_QUERY_VAR_CACHE ] ) { $validation_errors = wp_list_pluck( $data['results'], 'error' ); @@ -1834,7 +1836,6 @@ public static function send_validate_response( $sanitization_results, $status_co $data['validated_url_post'] = [ 'id' => $validated_url_post_id, 'edit_link' => get_edit_post_link( $validated_url_post_id, 'raw' ), - 'staleness' => AMP_Validated_URL_Post_Type::get_post_staleness( $validated_url_post_id ), ]; } } @@ -1882,10 +1883,10 @@ public static function maybe_send_cached_validate_response() { 'results' => [], 'queried_object' => null, 'url' => null, + 'revalidated' => false, // Since cached was used. 'validated_url_post' => [ 'id' => $post->ID, 'edit_link' => get_edit_post_link( $post->ID, 'raw' ), - 'staleness' => $staleness, ], ]; From c3294c929e4f9ea2dd494ebbf514b00283fa3c4c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Oct 2021 20:36:29 -0700 Subject: [PATCH 16/19] Test omission of stylesheets in send_validate_response --- .../class-amp-validation-manager.php | 4 +- .../test-class-amp-validation-manager.php | 40 +++++++++++++------ 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 1b5415c43c5..e3c6644c20d 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -681,9 +681,7 @@ private static function get_validate_request_args() { $unsanitized_values = [ self::VALIDATE_QUERY_VAR_NONCE => $unsanitized_values, ]; - } - - if ( ! is_array( $unsanitized_values ) ) { + } elseif ( ! is_array( $unsanitized_values ) ) { return $defaults; } diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 64d660bb9f1..b0228b42ac3 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -2173,10 +2173,19 @@ public function test_remove_illegal_source_stack_comments() { /** @return array */ public function get_data_to_test_send_validate_response() { return [ + 'ok_no_error_store' => [ + 'status_code' => 200, + 'last_error' => null, + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE => true, + AMP_Validation_Manager::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS => true, + ], + 'save_error' => false, + ], 'ok_no_error_no_store' => [ 'status_code' => 200, 'last_error' => null, - 'store' => false, + 'args' => [], 'save_error' => false, ], 'fatal_error_store' => [ @@ -2187,7 +2196,9 @@ public function get_data_to_test_send_validate_response() { 'file' => __FILE__, 'line' => __LINE__, ], - 'store' => true, + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE => true, + ], 'save_error' => false, ], 'warning_store' => [ @@ -2198,13 +2209,17 @@ public function get_data_to_test_send_validate_response() { 'file' => __FILE__, 'line' => __LINE__, ], - 'store' => true, + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE => true, + ], 'save_error' => false, ], 'store_failure' => [ 'status_code' => 200, 'last_error' => null, - 'store' => true, + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE => true, + ], 'save_error' => true, ], ]; @@ -2215,7 +2230,7 @@ public function get_data_to_test_send_validate_response() { * @covers \AMP_Validation_Manager::send_validate_response() * @covers \AMP_Validation_Manager::get_validate_request_args() */ - public function test_send_validate_response( $status_code, $last_error, $store, $save_error ) { + public function test_send_validate_response( $status_code, $last_error, $args, $save_error ) { $source_html = ''; $sanitizer_classes = amp_get_content_sanitizers(); $sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( $sanitizer_classes ); @@ -2229,9 +2244,7 @@ public function test_send_validate_response( $status_code, $last_error, $store, add_filter( 'wp_insert_post_empty_content', '__return_true' ); } - $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ] = [ - AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE => $store, - ]; + $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ] = $args; $response = AMP_Validation_Manager::send_validate_response( $sanitization_results, $status_code, $last_error ); $this->assertJson( $response ); @@ -2252,7 +2265,12 @@ public function test_send_validate_response( $status_code, $last_error, $store, $this->assertArrayHasKey( 'php_fatal_error', $data ); $this->assertArrayHasKey( 'queried_object', $data ); $this->assertArrayHasKey( 'url', $data ); - $this->assertArrayHasKey( 'stylesheets', $data ); + if ( ! empty( $args[ AMP_Validation_Manager::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) ) { + $this->assertArrayNotHasKey( 'stylesheets', $data ); + } else { + $this->assertArrayHasKey( 'stylesheets', $data ); + $this->assertCount( 1, $data['stylesheets'] ); + } $this->assertArrayHasKey( 'results', $data ); $this->assertEquals( $status_code, $data['http_status_code'] ); @@ -2266,9 +2284,7 @@ public function test_send_validate_response( $status_code, $last_error, $store, $this->assertCount( 1, $data['results'] ); $this->assertEquals( 'SPECIFIED_LAYOUT_INVALID', $data['results'][0]['error']['code'] ); - $this->assertCount( 1, $data['stylesheets'] ); - - if ( $store ) { + if ( ! empty( $args[ AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE ] ) ) { $this->assertArrayHasKey( 'validated_url_post', $data ); $this->assertArrayHasKey( 'id', $data['validated_url_post'] ); $this->assertArrayHasKey( 'edit_link', $data['validated_url_post'] ); From cdc013257d1c9f10046b85ac726db50de74d6bfd Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Oct 2021 21:16:40 -0700 Subject: [PATCH 17/19] Add test for maybe_send_cached_validate_response --- .../test-class-amp-validation-manager.php | 139 +++++++++++++++++- 1 file changed, 137 insertions(+), 2 deletions(-) diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index b0228b42ac3..92cbd83f89c 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -2294,12 +2294,147 @@ public function test_send_validate_response( $status_code, $last_error, $args, $ } } + /** @return array */ + public function get_data_to_test_maybe_send_cached_validate_response() { + return [ + 'no_validate_request' => [ + 'cached' => null, + 'stale' => null, + 'args' => [], + 'expected' => false, + ], + 'not_asking_for_cached' => [ + 'cached' => false, + 'stale' => false, + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), + ], + 'expected' => false, + ], + 'asking_for_fresh_cache_without_one_stored' => [ + 'cached' => false, + 'stale' => false, + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHED_IF_FRESH => true, + ], + 'expected' => false, + ], + 'asking_for_fresh_cache_but_stale_stored' => [ + 'cached' => true, + 'stale' => true, + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHED_IF_FRESH => true, + ], + 'expected' => false, + ], + 'asking_for_fresh_cache_and_fresh_stored' => [ + 'cached' => true, + 'stale' => false, + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHED_IF_FRESH => true, + ], + 'expected' => true, + ], + 'asking_for_fresh_cache_and_fresh_stored_sans_styles' => [ + 'cached' => true, + 'stale' => false, + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHED_IF_FRESH => true, + AMP_Validation_Manager::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS => true, + ], + 'expected' => true, + ], + ]; + } + /** + * @dataProvider get_data_to_test_maybe_send_cached_validate_response * @covers \AMP_Validation_Manager::maybe_send_cached_validate_response() * @covers \AMP_Validation_Manager::get_validate_request_args() */ - public function test_maybe_send_cached_validate_response() { - $this->markTestIncomplete(); + public function test_maybe_send_cached_validate_response( $cached, $stale, $args, $expected ) { + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + $post_id = self::factory()->post->create(); + $url = get_permalink( $post_id ); + $this->go_to( $url ); + $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ] = $args; + AMP_Validation_Manager::init_validate_request(); + + $validated_url_post_id = null; + if ( $cached ) { + $source_html = ''; + $sanitizer_classes = amp_get_content_sanitizers(); + $sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( $sanitizer_classes ); + $sanitization_results = AMP_Content_Sanitizer::sanitize_document( + AMP_DOM_Utils::get_dom_from_content( $source_html ), + $sanitizer_classes, + [] + ); + $data = AMP_Validation_Manager::get_validate_response_data( $sanitization_results ); + + $validation_errors = wp_list_pluck( $data['results'], 'error' ); + + $validated_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors( + $validation_errors, + $url, + $data + ); + $this->assertIsInt( $validated_url_post_id ); + + if ( $stale ) { + $validated_environment = get_post_meta( $validated_url_post_id, AMP_Validated_URL_Post_Type::VALIDATED_ENVIRONMENT_POST_META_KEY, true ); + + $validated_environment['plugins']['foo'] = '1.0'; + update_post_meta( $validated_url_post_id, AMP_Validated_URL_Post_Type::VALIDATED_ENVIRONMENT_POST_META_KEY, $validated_environment ); + } + } + + add_filter( 'wp_doing_ajax', '__return_true' ); + add_filter( + 'wp_die_ajax_handler', + function () { + return function () {}; + } + ); + + $response = get_echo( [ AMP_Validation_Manager::class, 'maybe_send_cached_validate_response' ] ); + + if ( ! $expected ) { + $this->assertEmpty( $response ); + return; + } + + $data = json_decode( $response, true ); + $this->assertEquals( JSON_ERROR_NONE, json_last_error() ); + $this->assertIsArray( $data ); + + $this->assertArrayHasKey( 'http_status_code', $data ); + $this->assertArrayHasKey( 'php_fatal_error', $data ); + $this->assertArrayHasKey( 'queried_object', $data ); + $this->assertArrayHasKey( 'url', $data ); + $this->assertEquals( $url, $data['url'] ); + if ( ! empty( $args[ AMP_Validation_Manager::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) ) { + $this->assertArrayNotHasKey( 'stylesheets', $data ); + } else { + $this->assertArrayHasKey( 'stylesheets', $data ); + $this->assertCount( 1, $data['stylesheets'] ); + } + + $this->assertArrayHasKey( 'results', $data ); + $this->assertFalse( $data['php_fatal_error'] ); + + $this->assertCount( 1, $data['results'] ); + $this->assertEquals( 'SPECIFIED_LAYOUT_INVALID', $data['results'][0]['error']['code'] ); + + $this->assertArrayHasKey( 'validated_url_post', $data ); + $this->assertArrayHasKey( 'id', $data['validated_url_post'] ); + $this->assertEquals( $validated_url_post_id, $data['validated_url_post']['id'] ); + $this->assertArrayHasKey( 'edit_link', $data['validated_url_post'] ); + $this->assertEquals( AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, get_post_type( $data['validated_url_post']['id'] ) ); } /** From 9f3a80c978eb83c937674810741e4ba295f5e827 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Oct 2021 21:29:36 -0700 Subject: [PATCH 18/19] Improve tests for send_validate_response --- tests/php/test-class-amp-theme-support.php | 43 ++++++++++++------- .../test-class-amp-validation-manager.php | 3 +- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php index d166127dff9..475f4db55d7 100644 --- a/tests/php/test-class-amp-theme-support.php +++ b/tests/php/test-class-amp-theme-support.php @@ -1900,11 +1900,23 @@ public function test_prepare_response_for_validating_non_amp_page() { /** @return array */ public function get_data_to_test_prepare_response_for_validating_amp_page() { return [ - 'no-store' => [ - false, + 'no-store' => [ + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), + ], ], - 'store' => [ - true, + 'store' => [ + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE => true, + ], + ], + 'store_but_omit_styleshets' => [ + 'args' => [ + AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), + AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE => true, + AMP_Validation_Manager::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS => true, + ], ], ]; } @@ -1915,34 +1927,33 @@ public function get_data_to_test_prepare_response_for_validating_amp_page() { * @dataProvider get_data_to_test_prepare_response_for_validating_amp_page * @covers AMP_Theme_Support::prepare_response() * @covers AMP_Validation_Manager::send_validate_response() - * - * @param bool $store Whether to store results. */ - public function test_prepare_response_for_validating_amp_page( $store ) { + public function test_prepare_response_for_validating_amp_page( $args ) { wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); $this->set_template_mode( AMP_Theme_Support::STANDARD_MODE_SLUG ); $this->go_to( '/' ); - $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ] = [ - AMP_Validation_Manager::VALIDATE_QUERY_VAR_NONCE => AMP_Validation_Manager::get_amp_validate_nonce(), - ]; - if ( $store ) { - $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ][ AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE ] = 'true'; - } + $_GET[ AMP_Validation_Manager::VALIDATE_QUERY_VAR ] = $args; AMP_Validation_Manager::init_validate_request(); - $response = AMP_Theme_Support::prepare_response( '' ); + AMP_Theme_Support::finish_init(); + $response = AMP_Theme_Support::prepare_response( '' ); $this->assertJson( $response ); $data = json_decode( $response, true ); $this->assertArrayHasKey( 'http_status_code', $data ); $this->assertArrayHasKey( 'php_fatal_error', $data ); $this->assertArrayHasKey( 'queried_object', $data ); $this->assertArrayHasKey( 'url', $data ); - $this->assertArrayHasKey( 'stylesheets', $data ); + if ( ! empty( $args[ AMP_Validation_Manager::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) ) { + $this->assertArrayNotHasKey( 'stylesheets', $data ); + } else { + $this->assertArrayHasKey( 'stylesheets', $data ); + } $this->assertArrayHasKey( 'results', $data ); $this->assertCount( 1, $data['results'] ); $this->assertEquals( 'SPECIFIED_LAYOUT_INVALID', $data['results'][0]['error']['code'] ); + $this->assertTrue( $data['revalidated'] ); - if ( $store ) { + if ( ! empty( $args[ AMP_Validation_Manager::VALIDATE_QUERY_VAR_CACHE ] ) ) { $this->assertArrayHasKey( 'validated_url_post', $data ); $this->assertArrayHasKey( 'id', $data['validated_url_post'] ); $this->assertArrayHasKey( 'edit_link', $data['validated_url_post'] ); diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 92cbd83f89c..5ee13fd203a 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -2408,14 +2408,15 @@ function () { return; } + $this->assertJson( $response ); $data = json_decode( $response, true ); - $this->assertEquals( JSON_ERROR_NONE, json_last_error() ); $this->assertIsArray( $data ); $this->assertArrayHasKey( 'http_status_code', $data ); $this->assertArrayHasKey( 'php_fatal_error', $data ); $this->assertArrayHasKey( 'queried_object', $data ); $this->assertArrayHasKey( 'url', $data ); + $this->assertFalse( $data['revalidated'] ); $this->assertEquals( $url, $data['url'] ); if ( ! empty( $args[ AMP_Validation_Manager::VALIDATE_QUERY_VAR_OMIT_STYLESHEETS ] ) ) { $this->assertArrayNotHasKey( 'stylesheets', $data ); From c226596b7a6814a9bbe1e498842d44e1a2324d29 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 25 Oct 2021 16:38:15 -0700 Subject: [PATCH 19/19] Remove extraneous action priority --- includes/validation/class-amp-validation-manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index e3c6644c20d..24786552b60 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -250,7 +250,7 @@ static function() { add_action( 'all_admin_notices', [ __CLASS__, 'print_plugin_notice' ] ); add_action( 'admin_bar_menu', [ __CLASS__, 'add_admin_bar_menu_items' ], 101 ); - add_action( 'wp', [ __CLASS__, 'maybe_fail_validate_request' ], 10 ); + add_action( 'wp', [ __CLASS__, 'maybe_fail_validate_request' ] ); add_action( 'wp', [ __CLASS__, 'maybe_send_cached_validate_response' ], 20 ); add_action( 'wp', [ __CLASS__, 'override_validation_error_statuses' ] );