From 5e0055a62590a7919a3c1cb03e8fbbd89d513d31 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Jun 2018 11:58:53 -0700 Subject: [PATCH 1/7] Add amp_get_current_url() helper function --- includes/amp-frontend-actions.php | 6 ++---- includes/amp-helper-functions.php | 20 ++++++++++++++++++++ tests/test-amp-helper-functions.php | 26 ++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/includes/amp-frontend-actions.php b/includes/amp-frontend-actions.php index f5d7ebca13d..aed8469eb6a 100644 --- a/includes/amp-frontend-actions.php +++ b/includes/amp-frontend-actions.php @@ -29,10 +29,8 @@ function amp_frontend_add_canonical() { $amp_url = null; if ( is_singular() ) { $amp_url = amp_get_permalink( get_queried_object_id() ); - } elseif ( isset( $_SERVER['REQUEST_URI'] ) ) { - $host_url = preg_replace( '#(^https?://[^/]+)/.*#', '$1', home_url( '/' ) ); - $self_url = esc_url_raw( $host_url . wp_unslash( $_SERVER['REQUEST_URI'] ) ); - $amp_url = add_query_arg( amp_get_slug(), '', $self_url ); + } else { + $amp_url = add_query_arg( amp_get_slug(), '', amp_get_current_url() ); } if ( $amp_url ) { printf( '', esc_url( $amp_url ) ); diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 35e5b16cc1e..f83fcc848d7 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -49,6 +49,26 @@ function amp_get_slug() { return $query_var; } +/** + * Get the URL for the current request. + * + * This is essentially the REQUEST_URI prefixed by the scheme and host for the home URL. + * This is needed in particular due to subdirectory installs. + * + * @since 1.0 + * + * @return string Current URL. + */ +function amp_get_current_url() { + $url = preg_replace( '#(^https?://[^/]+)/.*#', '$1', home_url( '/' ) ); + if ( isset( $_SERVER['REQUEST_URI'] ) ) { + $url = esc_url_raw( $url . wp_unslash( $_SERVER['REQUEST_URI'] ) ); + } else { + $url .= '/'; + } + return $url; +} + /** * Retrieves the full AMP-specific permalink for the given post ID. * diff --git a/tests/test-amp-helper-functions.php b/tests/test-amp-helper-functions.php index 06cfdc52e5a..7e7abf7b698 100644 --- a/tests/test-amp-helper-functions.php +++ b/tests/test-amp-helper-functions.php @@ -48,6 +48,32 @@ public function test_amp_get_slug() { $this->assertSame( 'amp', amp_get_slug() ); } + /** + * Test amp_get_current_url(). + * + * @covers \amp_get_current_url() + */ + public function test_amp_get_current_url() { + $request_uris = array( + '/foo', + '/bar?baz', + null, + ); + + foreach ( $request_uris as $request_uri ) { + if ( $request_uri ) { + $_SERVER['REQUEST_URI'] = wp_slash( $request_uri ); + } else { + unset( $_SERVER['REQUEST_URI'] ); + } + $this->assertEquals( + home_url( $request_uri ? $request_uri : '/' ), + amp_get_current_url(), + sprintf( 'Unexpected for URI: %s', wp_json_encode( $request_uri, 64 /* JSON_UNESCAPED_SLASHES */ ) ) + ); + } + } + /** * Test amp_get_permalink() without pretty permalinks. * From 029a325a95b935390fa3d968900cac8aa95da1d5 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Jun 2018 12:45:28 -0700 Subject: [PATCH 2/7] Redirect from /amp/ URL endpoint to URL with ?amp query param in paired amp theme support --- includes/class-amp-theme-support.php | 85 ++++++++++++++++------ tests/test-class-amp-theme-support.php | 97 ++++++++++++++++++++++++-- 2 files changed, 155 insertions(+), 27 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 5097844e350..616c04bf846 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -149,9 +149,9 @@ public static function finish_init() { return; } - if ( amp_is_canonical() ) { - self::redirect_canonical_amp(); - } else { + self::ensure_proper_amp_location(); + + if ( ! amp_is_canonical() ) { self::register_paired_hooks(); } @@ -168,32 +168,75 @@ public static function finish_init() { } /** - * Redirect to canonical URL if the AMP URL was loaded, since canonical is now AMP. + * Ensure that the current AMP location is correct. * - * @since 0.7 - * @since 1.0 Added $exit param. - * @todo Rename to redirect_non_amp(). + * @since 1.0 * * @param bool $exit Whether to exit after redirecting. + * @return bool Whether redirection was done. Naturally this is irrelevant if $exit is true. */ - public static function redirect_canonical_amp( $exit = true ) { - if ( false !== get_query_var( amp_get_slug(), false ) ) { // Because is_amp_endpoint() now returns true if amp_is_canonical(). - $url = preg_replace( '#^(https?://.+?)(/.*)$#', '$1', home_url( '/' ) ); - if ( isset( $_SERVER['REQUEST_URI'] ) ) { - $url .= wp_unslash( $_SERVER['REQUEST_URI'] ); - } - - $url = amp_remove_endpoint( $url ); + public static function ensure_proper_amp_location( $exit = true ) { + $has_query_var = false !== get_query_var( amp_get_slug(), false ); // May come from URL param or endpoint slug. + $has_url_param = isset( $_GET[ amp_get_slug() ] ); // WPCS: CSRF OK. + if ( amp_is_canonical() ) { /* - * Temporary redirect because AMP URL may return when blocking validation errors - * occur or when a non-canonical AMP theme is used. + * When AMP native/canonical, then when there is an /amp/ endpoint or ?amp URL param, + * then a redirect needs to be done to the URL without any AMP indicator in the URL. */ - wp_safe_redirect( $url, 302 ); - if ( $exit ) { - exit; + if ( $has_query_var || $has_url_param ) { + return self::redirect_ampless_url( $exit ); + } + } else { + /* + * When in AMP paired mode *with* theme support, then the proper AMP URL has the 'amp' URL param + * and not the /amp/ endpoint. The URL param is now the exclusive way to mark AMP in paired mode + * when amp theme support present. This is important for plugins to be able to reliably call + * is_amp_endpoint() before the parse_query action. + */ + if ( $has_query_var && ! $has_url_param ) { + $old_url = amp_get_current_url(); + $new_url = add_query_arg( amp_get_slug(), '', amp_remove_endpoint( $old_url ) ); + if ( $old_url !== $new_url ) { + wp_safe_redirect( $new_url, 302 ); + if ( $exit ) { + exit; + } + return true; + } } } + return false; + } + + /** + * Redirect to non-AMP version of the current URL, such as because AMP is canonical or there are unaccepted validation errors. + * + * If the current URL is already AMP-less then do nothing. + * + * @since 0.7 + * @since 1.0 Added $exit param. + * @since 1.0 Renamed from redirect_canonical_amp(). + * + * @param bool $exit Whether to exit after redirecting. + * @return bool Whether redirection was done. Naturally this is irrelevant if $exit is true. + */ + public static function redirect_ampless_url( $exit = true ) { + $current_url = amp_get_current_url(); + $ampless_url = amp_remove_endpoint( $current_url ); + if ( $ampless_url === $current_url ) { + return false; + } + + /* + * Temporary redirect because AMP URL may return when blocking validation errors + * occur or when a non-canonical AMP theme is used. + */ + wp_safe_redirect( $ampless_url, 302 ); + if ( $exit ) { + exit; + } + return true; } /** @@ -1162,7 +1205,7 @@ public static function prepare_response( $response, $args = array() ) { $head->appendChild( $script ); } } else { - self::redirect_canonical_amp( false ); + self::redirect_ampless_url( false ); return esc_html__( 'Redirecting to non-AMP version.', 'amp' ); } } diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 6a7d088fb34..5e8f8e62e2e 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -130,19 +130,104 @@ public function test_amphtml_link() { } /** - * Test redirect_canonical_amp. + * Test ensure_proper_amp_location for canonical. * - * @covers AMP_Theme_Support::redirect_canonical_amp() + * @covers AMP_Theme_Support::ensure_proper_amp_location() */ - public function test_redirect_canonical_amp() { - set_query_var( amp_get_slug(), 1 ); + public function test_ensure_proper_amp_location_canonical() { + add_theme_support( 'amp' ); + $e = null; + + // Already canonical. + $_SERVER['REQUEST_URI'] = '/foo/bar/'; + $this->assertFalse( AMP_Theme_Support::ensure_proper_amp_location( false ) ); + + // URL query param. + $_GET[ amp_get_slug() ] = ''; + $_SERVER['REQUEST_URI'] = add_query_arg( amp_get_slug(), '', '/foo/bar' ); + try { + $this->assertTrue( AMP_Theme_Support::ensure_proper_amp_location( false ) ); + } catch ( Exception $exception ) { + $e = $exception; + } + $this->assertTrue( isset( $e ) ); // wp_safe_redirect() modifies the headers, and causes an error. + $this->assertContains( 'headers already sent', $e->getMessage() ); + $e = null; + + // Endpoint. + unset( $_GET[ amp_get_slug() ] ); + set_query_var( amp_get_slug(), '' ); + $_SERVER['REQUEST_URI'] = '/2016/01/24/foo/amp/'; + try { + $this->assertTrue( AMP_Theme_Support::ensure_proper_amp_location( false ) ); + } catch ( Exception $exception ) { + $e = $exception; + } + $this->assertContains( 'headers already sent', $e->getMessage() ); + $e = null; + } + + /** + * Test ensure_proper_amp_location for paired. + * + * @covers AMP_Theme_Support::ensure_proper_amp_location() + */ + public function test_ensure_proper_amp_location_paired() { + add_theme_support( 'amp', array( + 'template_dir' => './', + ) ); + $e = null; + + // URL query param, no redirection. + $_GET[ amp_get_slug() ] = ''; + $_SERVER['REQUEST_URI'] = add_query_arg( amp_get_slug(), '', '/foo/bar' ); + $this->assertFalse( AMP_Theme_Support::ensure_proper_amp_location( false ) ); + + // Endpoint, redirect. + unset( $_GET[ amp_get_slug() ] ); + set_query_var( amp_get_slug(), '' ); + $_SERVER['REQUEST_URI'] = '/2016/01/24/foo/amp/'; + try { + $this->assertTrue( AMP_Theme_Support::ensure_proper_amp_location( false ) ); + } catch ( Exception $exception ) { + $e = $exception; + } + $this->assertContains( 'headers already sent', $e->getMessage() ); + } + + /** + * Test redirect_ampless_url. + * + * @covers AMP_Theme_Support::redirect_ampless_url() + */ + public function test_redirect_ampless_url() { + $e = null; + + // Try AMP URL param. + $_SERVER['REQUEST_URI'] = add_query_arg( amp_get_slug(), '', '/foo/bar' ); try { - AMP_Theme_Support::redirect_canonical_amp(); + $this->assertTrue( AMP_Theme_Support::redirect_ampless_url() ); } catch ( Exception $exception ) { $e = $exception; } - // wp_safe_redirect() modifies the headers, and causes an error. $this->assertTrue( isset( $e ) ); + $this->assertContains( 'headers already sent', $e->getMessage() ); + $e = null; + + // Try AMP URL endpoint. + $_SERVER['REQUEST_URI'] = '/2016/01/24/foo/amp/'; + try { + $this->assertTrue( AMP_Theme_Support::redirect_ampless_url() ); + } catch ( Exception $exception ) { + $e = $exception; + } + $this->assertTrue( isset( $e ) ); // wp_safe_redirect() modifies the headers, and causes an error. + $this->assertContains( 'headers already sent', $e->getMessage() ); + $e = null; + + // Make sure that if the URL doesn't have AMP that there should be no redirect. + $_SERVER['REQUEST_URI'] = '/foo/bar'; + $this->assertFalse( AMP_Theme_Support::redirect_ampless_url() ); } /** From a7fbe8f4ffaffdba9e0cc16e2aa26d02821320cb Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Jun 2018 13:07:47 -0700 Subject: [PATCH 3/7] Let amp_add_amphtml_link() replace poorly-named amp_frontend_add_canonical() Deprecate amp-frontend-actions.php in favor of adding function to amp-helper-functions.php. --- amp.php | 2 +- includes/amp-frontend-actions.php | 28 ++------- includes/amp-helper-functions.php | 28 +++++++++ includes/amp-post-template-actions.php | 1 + includes/class-amp-theme-support.php | 6 +- tests/test-amp-frontend-actions.php | 87 -------------------------- tests/test-amp-helper-functions.php | 61 ++++++++++++++++++ tests/test-class-amp-theme-support.php | 10 +-- 8 files changed, 103 insertions(+), 120 deletions(-) delete mode 100644 tests/test-amp-frontend-actions.php diff --git a/amp.php b/amp.php index f1534a491e0..71b5c389239 100644 --- a/amp.php +++ b/amp.php @@ -312,7 +312,7 @@ function amp_load_classes() { * @since 0.2 */ function amp_add_frontend_actions() { - require_once AMP__DIR__ . '/includes/amp-frontend-actions.php'; + add_action( 'wp_head', 'amp_add_amphtml_link' ); } /** diff --git a/includes/amp-frontend-actions.php b/includes/amp-frontend-actions.php index aed8469eb6a..75ce9030a35 100644 --- a/includes/amp-frontend-actions.php +++ b/includes/amp-frontend-actions.php @@ -2,37 +2,21 @@ /** * Callbacks for adding AMP-related things to the main theme. * + * @deprecated Function in this file has been moved to amp-helper-functions.php. * @package AMP */ -add_action( 'wp_head', 'amp_frontend_add_canonical' ); +_deprecated_file( __FILE__, '1.0', null, esc_html__( 'Use amp_add_amphtml_link() function which is already included from amp-helper-functions.php', 'amp' ) ); /** * Add amphtml link to frontend. * - * @todo This function's name is incorrect. It's not about adding a canonical link but adding the amphtml link. + * @deprecated * * @since 0.2 + * @since 1.0 Deprecated */ function amp_frontend_add_canonical() { - - /** - * Filters whether to show the amphtml link on the frontend. - * - * @todo This filter's name is incorrect. It's not about adding a canonical link but adding the amphtml link. - * @since 0.2 - */ - if ( false === apply_filters( 'amp_frontend_show_canonical', true ) ) { - return; - } - - $amp_url = null; - if ( is_singular() ) { - $amp_url = amp_get_permalink( get_queried_object_id() ); - } else { - $amp_url = add_query_arg( amp_get_slug(), '', amp_get_current_url() ); - } - if ( $amp_url ) { - printf( '', esc_url( $amp_url ) ); - } + _deprecated_function( __FUNCTION__, '1.0', 'amp_add_amphtml_link' ); + amp_add_amphtml_link(); } diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index f83fcc848d7..fdb513b6eb0 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -166,6 +166,34 @@ function amp_remove_endpoint( $url ) { return $url; } +/** + * Add amphtml link. + * + * @since 1.0 + */ +function amp_add_amphtml_link() { + + /** + * Filters whether to show the amphtml link on the frontend. + * + * @todo This filter's name is incorrect. It's not about adding a canonical link but adding the amphtml link. + * @since 0.2 + */ + if ( false === apply_filters( 'amp_frontend_show_canonical', true ) ) { + return; + } + + $amp_url = null; + if ( is_singular() ) { + $amp_url = amp_get_permalink( get_queried_object_id() ); + } else { + $amp_url = add_query_arg( amp_get_slug(), '', amp_get_current_url() ); + } + if ( $amp_url ) { + printf( '', esc_url( $amp_url ) ); + } +} + /** * Determine whether a given post supports AMP. * diff --git a/includes/amp-post-template-actions.php b/includes/amp-post-template-actions.php index 88d717c2c6e..fa3fc301a28 100644 --- a/includes/amp-post-template-actions.php +++ b/includes/amp-post-template-actions.php @@ -2,6 +2,7 @@ /** * Callbacks for adding content to an AMP template. * + * @todo Rename this file from amp-post-template-actions.php to amp-post-template-functions.php. * @package AMP */ diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 616c04bf846..9c1ee245c94 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -139,12 +139,8 @@ public static function init() { */ public static function finish_init() { if ( ! is_amp_endpoint() ) { - // Add amphtml link when paired mode is available. if ( self::is_paired_available() ) { - amp_add_frontend_actions(); // @todo This function is poor in how it requires a file that then does add_action(). - if ( ! has_action( 'wp_head', 'amp_frontend_add_canonical' ) ) { - add_action( 'wp_head', 'amp_frontend_add_canonical' ); - } + amp_add_frontend_actions(); } return; } diff --git a/tests/test-amp-frontend-actions.php b/tests/test-amp-frontend-actions.php deleted file mode 100644 index 31809e82e65..00000000000 --- a/tests/test-amp-frontend-actions.php +++ /dev/null @@ -1,87 +0,0 @@ -assertEquals( 10, has_action( 'wp_head', 'amp_frontend_add_canonical' ) ); - } - - /** - * URLs to test amphtml link. - * - * @return array - */ - public function get_amphtml_urls() { - $post_id = $this->factory()->post->create(); - return array( - 'home' => array( - home_url( '/' ), - add_query_arg( amp_get_slug(), '', home_url( '/' ) ), - ), - '404' => array( - home_url( '/no-existe/' ), - add_query_arg( amp_get_slug(), '', home_url( '/no-existe/' ) ), - ), - 'post' => array( - get_permalink( $post_id ), - amp_get_permalink( $post_id ), - ), - ); - } - - /** - * Adding link when theme support is not present. - * - * @dataProvider get_amphtml_urls - * @covers \amp_frontend_add_canonical() - * @param string $canonical_url Canonical URL. - * @param string $amphtml_url The amphtml URL. - */ - public function test_amp_frontend_add_canonical( $canonical_url, $amphtml_url ) { - $this->go_to( $canonical_url ); - ob_start(); - amp_frontend_add_canonical(); - $output = ob_get_clean(); - $this->assertEquals( - sprintf( '', esc_url( $amphtml_url ) ), - $output - ); - - // Make sure adding the filter hides the amphtml link. - add_filter( 'amp_frontend_show_canonical', '__return_false' ); - ob_start(); - amp_frontend_add_canonical(); - $output = ob_get_clean(); - $this->assertEmpty( $output ); - } -} diff --git a/tests/test-amp-helper-functions.php b/tests/test-amp-helper-functions.php index 7e7abf7b698..9d3e5a096a7 100644 --- a/tests/test-amp-helper-functions.php +++ b/tests/test-amp-helper-functions.php @@ -223,6 +223,67 @@ public function test_amp_remove_endpoint() { $this->assertEquals( 'https://example.com/foo/?blaz', amp_remove_endpoint( 'https://example.com/foo/amp/?blaz' ) ); } + + /** + * Test that hook is added. + * + * @covers \amp_add_frontend_actions() + */ + public function test_amp_add_frontend_actions() { + $this->assertFalse( has_action( 'wp_head', 'amp_add_amphtml_link' ) ); + amp_add_frontend_actions(); + $this->assertEquals( 10, has_action( 'wp_head', 'amp_add_amphtml_link' ) ); + } + + /** + * URLs to test amphtml link. + * + * @return array + */ + public function get_amphtml_urls() { + $post_id = $this->factory()->post->create(); + return array( + 'home' => array( + home_url( '/' ), + add_query_arg( amp_get_slug(), '', home_url( '/' ) ), + ), + '404' => array( + home_url( '/no-existe/' ), + add_query_arg( amp_get_slug(), '', home_url( '/no-existe/' ) ), + ), + 'post' => array( + get_permalink( $post_id ), + amp_get_permalink( $post_id ), + ), + ); + } + + /** + * Adding link when theme support is not present. + * + * @dataProvider get_amphtml_urls + * @covers \amp_add_amphtml_link() + * @param string $canonical_url Canonical URL. + * @param string $amphtml_url The amphtml URL. + */ + public function test_amp_add_amphtml_link( $canonical_url, $amphtml_url ) { + $this->go_to( $canonical_url ); + ob_start(); + amp_add_amphtml_link(); + $output = ob_get_clean(); + $this->assertEquals( + sprintf( '', esc_url( $amphtml_url ) ), + $output + ); + + // Make sure adding the filter hides the amphtml link. + add_filter( 'amp_frontend_show_canonical', '__return_false' ); + ob_start(); + amp_add_amphtml_link(); + $output = ob_get_clean(); + $this->assertEmpty( $output ); + } + /** * Test is_amp_endpoint() function. * diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 5e8f8e62e2e..4977d142c3f 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -110,23 +110,23 @@ public function test_amphtml_link() { ) ); // Test paired mode singular. - remove_action( 'wp_head', 'amp_frontend_add_canonical' ); + remove_action( 'wp_head', 'amp_add_amphtml_link' ); $this->go_to( get_permalink( $post_id ) ); AMP_Theme_Support::finish_init(); - $this->assertEquals( 10, has_action( 'wp_head', 'amp_frontend_add_canonical' ) ); + $this->assertEquals( 10, has_action( 'wp_head', 'amp_add_amphtml_link' ) ); // Test paired mode homepage. - remove_action( 'wp_head', 'amp_frontend_add_canonical' ); + remove_action( 'wp_head', 'amp_add_amphtml_link' ); $this->go_to( home_url() ); AMP_Theme_Support::finish_init(); - $this->assertFalse( has_action( 'wp_head', 'amp_frontend_add_canonical' ) ); + $this->assertFalse( has_action( 'wp_head', 'amp_add_amphtml_link' ) ); // Test canonical. remove_theme_support( 'amp' ); add_theme_support( 'amp' ); $this->go_to( get_permalink( $post_id ) ); AMP_Theme_Support::finish_init(); - $this->assertFalse( has_action( 'wp_head', 'amp_frontend_add_canonical' ) ); + $this->assertFalse( has_action( 'wp_head', 'amp_add_amphtml_link' ) ); } /** From 31b9d52aca6e2c6a72dbf3b492c99ac431dbe2e7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Jun 2018 21:21:44 -0700 Subject: [PATCH 4/7] Fix old_slug_redirect_url handling for AMP theme support --- amp.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/amp.php b/amp.php index 71b5c389239..0daee30a4b0 100644 --- a/amp.php +++ b/amp.php @@ -436,8 +436,12 @@ function _amp_bootstrap_customizer() { */ function amp_redirect_old_slug_to_new_url( $link ) { - if ( is_amp_endpoint() ) { - $link = trailingslashit( trailingslashit( $link ) . amp_get_slug() ); + if ( is_amp_endpoint() && ! amp_is_canonical() ) { + if ( current_theme_supports( 'amp' ) ) { + $link = add_query_arg( amp_get_slug(), '', $link ); + } else { + $link = trailingslashit( trailingslashit( $link ) . amp_get_slug() ); + } } return $link; From 5ef42c2823eeb50d8f9f8554d377a4e04f31dac0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 6 Jun 2018 23:04:03 -0700 Subject: [PATCH 5/7] Fix handling of re-validating URLs when redirects happen or post slugs change * Fix validation when switching between native and paired modes. * Only store canonical URLs for amp_invalid_url posts. * Fix issue with get_current_screen() not returning an object. --- .../class-amp-invalid-url-post-type.php | 100 ++++++++++----- .../class-amp-validation-error-taxonomy.php | 2 +- .../class-amp-validation-manager.php | 116 ++++++++++++++---- tests/test-class-amp-validation-utils.php | 5 +- 4 files changed, 166 insertions(+), 57 deletions(-) diff --git a/includes/validation/class-amp-invalid-url-post-type.php b/includes/validation/class-amp-invalid-url-post-type.php index aa9e9b71f1d..61c8df8232d 100644 --- a/includes/validation/class-amp-invalid-url-post-type.php +++ b/includes/validation/class-amp-invalid-url-post-type.php @@ -277,24 +277,52 @@ public static function display_invalid_url_validation_error_counts_summary( $pos * @return WP_Post|null The post of the existing custom post, or null. */ public static function get_invalid_url_post( $url ) { + $url = remove_query_arg( amp_get_slug(), $url ); return get_page_by_path( md5( $url ), OBJECT, self::POST_TYPE_SLUG ); } + /** + * Get the URL from a given amp_invalid_url post. + * + * The URL will be returned with the amp query var added to it if the site is not canonical. The post_title + * is always stored using the canonical AMP-less URL. + * + * @param int|WP_post $post Post. + * @return string|null The URL stored for the post or null if post does not exist or it is not the right type. + */ + public static function get_url_from_post( $post ) { + $post = get_post( $post ); + if ( ! $post || self::POST_TYPE_SLUG !== $post->post_type ) { + return null; + } + $url = $post->post_title; + if ( ! amp_is_canonical() ) { + $url = add_query_arg( amp_get_slug(), '', $url ); + } + return $url; + } + /** * Stores the validation errors. * * If there are no validation errors provided, then any existing amp_invalid_url post is deleted. * - * @param array $validation_errors Validation errors. - * @param string $url URL on which the validation errors occurred. + * @param array $validation_errors Validation errors. + * @param string $url URL on which the validation errors occurred. Will be normalized to non-AMP version. + * @param int|WP_Post $post Post to update. Optional. If empty, then post is looked up by URL. * @return int|WP_Error $post_id The post ID of the custom post type used, null if post was deleted due to no validation errors, or WP_Error on failure. * @global WP $wp */ - public static function store_validation_errors( $validation_errors, $url ) { - $post_slug = md5( $url ); - $post = get_page_by_path( $post_slug, OBJECT, self::POST_TYPE_SLUG ); - if ( ! $post ) { - $post = get_page_by_path( $post_slug . '__trashed', OBJECT, self::POST_TYPE_SLUG ); + public static function store_validation_errors( $validation_errors, $url, $post = null ) { + $url = remove_query_arg( amp_get_slug(), $url ); // Only ever store the canonical version. + $slug = md5( $url ); + if ( $post ) { + $post = get_post( $post ); + } else { + $post = get_page_by_path( $slug, OBJECT, self::POST_TYPE_SLUG ); + if ( ! $post ) { + $post = get_page_by_path( $slug . '__trashed', OBJECT, self::POST_TYPE_SLUG ); + } } // Since there are no validation errors and there is an existing $existing_post_id, just delete the post. @@ -367,7 +395,7 @@ public static function store_validation_errors( $validation_errors, $url ) { 'ID' => $post ? $post->ID : null, 'post_type' => self::POST_TYPE_SLUG, 'post_title' => $url, - 'post_name' => $post_slug, + 'post_name' => $slug, 'post_content' => $placeholder, // Content is provided via wp_insert_post_data filter above to guard against Kses-corruption. 'post_status' => 'publish', ) ), @@ -609,10 +637,15 @@ public static function filter_row_actions( $actions, $post ) { esc_html__( 'Details', 'amp' ) ); unset( $actions['inline hide-if-no-js'] ); - $url = $post->post_title; - $view_url = add_query_arg( AMP_Validation_Manager::VALIDATE_QUERY_VAR, '', $url ); // Prevent redirection to non-AMP page. - $actions['view'] = sprintf( '%s', esc_url( $view_url ), esc_html__( 'View', 'amp' ) ); + $url = self::get_url_from_post( $post ); + if ( $url ) { + $actions['view'] = sprintf( + '%s', + esc_url( add_query_arg( AMP_Validation_Manager::VALIDATE_QUERY_VAR, '', $url ) ), + esc_html__( 'View', 'amp' ) + ); + } $actions[ self::RECHECK_ACTION ] = sprintf( '%s', @@ -653,19 +686,20 @@ public static function handle_bulk_action( $redirect, $action, $items ) { if ( empty( $post ) ) { continue; } - $url = $post->post_title; + + $url = self::get_url_from_post( $post ); if ( empty( $url ) ) { continue; } - $validation_errors = AMP_Validation_Manager::validate_url( $url ); - if ( ! is_array( $validation_errors ) ) { - continue; + $validity = AMP_Validation_Manager::validate_url( $url ); + if ( ! is_array( $validity ) ) { + continue; // @todo Count this error. } - self::store_validation_errors( $validation_errors, $url ); - if ( ! empty( $validation_errors ) ) { - $remaining_invalid_urls[] = $url; + self::store_validation_errors( $validity['validation_errors'], $validity['url'], $post ); + if ( ! empty( $validity['validation_errors'] ) ) { + $remaining_invalid_urls[] = $validity['url']; } } @@ -777,16 +811,19 @@ function( $result ) { */ public static function recheck_post( $post ) { $post = get_post( $post ); - $url = $post->post_title; + $url = self::get_url_from_post( $post ); + if ( ! $url ) { + return new WP_Error( 'missing_url' ); + } - $validation_errors = AMP_Validation_Manager::validate_url( $url ); - if ( is_wp_error( $validation_errors ) ) { - return $validation_errors; + $validity = AMP_Validation_Manager::validate_url( $url ); + if ( is_wp_error( $validity ) ) { + return $validity; } $validation_results = array(); - self::store_validation_errors( $validation_errors, $url ); - foreach ( $validation_errors as $error ) { + self::store_validation_errors( $validity['validation_errors'], $validity['url'], $post->ID ); + foreach ( $validity['validation_errors'] as $error ) { $sanitized = AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $error ); $validation_results[] = compact( 'error', 'sanitized' ); @@ -963,6 +1000,8 @@ public static function print_status_meta_box( $post ) { public static function print_validation_errors_meta_box( $post ) { $validation_errors = self::get_invalid_url_validation_errors( $post ); + $url = self::get_url_from_post( $post ); + $can_serve_amp = 0 === count( array_filter( $validation_errors, function( $validation_error ) { return AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS !== $validation_error['status']; } ) ); @@ -994,7 +1033,7 @@ public static function print_validation_errors_meta_box( $post ) {