From a4ba86c14bf0fbaf429960b0ead6d9266f8070e3 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 1 Jun 2018 23:26:39 -0700 Subject: [PATCH 1/2] Update amp_get_slug() and amp_get_permalink() to always use 'amp' query var when theme support added (and not endpoint) * Soft-deprecate functions that are for legacy functions. * Add missing since tags. --- amp.php | 19 +++++++- includes/amp-helper-functions.php | 68 ++++++++++++++++++++++++----- tests/test-amp-helper-functions.php | 62 +++++++++++++++++++++++++- 3 files changed, 134 insertions(+), 15 deletions(-) diff --git a/amp.php b/amp.php index 036c368fc18..a3911bb3396 100644 --- a/amp.php +++ b/amp.php @@ -113,6 +113,13 @@ function amp_deactivate() { function amp_after_setup_theme() { amp_get_slug(); // Ensure AMP_QUERY_VAR is set. + /** + * Filters whether AMP is enabled on the current site. + * + * Useful if the plugin is network activated and you want to turn it off on select sites. + * + * @since 0.2 + */ if ( false === apply_filters( 'amp_is_enabled', true ) ) { return; } @@ -183,6 +190,7 @@ function amp_force_query_var_value( $query_vars ) { * If the request is for an AMP page and this is in 'canonical mode,' redirect to the non-AMP page. * It won't need this plugin's template system, nor the frontend actions like the 'rel' link. * + * @deprecated This function is not used when 'amp' theme support is added. * @global WP_Query $wp_query * @since 0.2 * @return void @@ -311,6 +319,7 @@ function amp_add_frontend_actions() { * Add post template actions. * * @since 0.2 + * @deprecated This function is not used when 'amp' theme support is added. */ function amp_add_post_template_actions() { require_once AMP__DIR__ . '/includes/amp-post-template-actions.php'; @@ -322,6 +331,7 @@ function amp_add_post_template_actions() { * Add action to do post template rendering at template_redirect action. * * @since 0.2 + * @deprecated This function is not used when 'amp' theme support is added. */ function amp_prepare_render() { add_action( 'template_redirect', 'amp_render' ); @@ -331,6 +341,7 @@ function amp_prepare_render() { * Render AMP for queried post. * * @since 0.1 + * @deprecated This function is not used when 'amp' theme support is added. */ function amp_render() { // Note that queried object is used instead of the ID so that the_preview for the queried post can apply. @@ -345,6 +356,8 @@ function amp_render() { * Render AMP post template. * * @since 0.5 + * @deprecated This function is not used when 'amp' theme support is added. + * * @param WP_Post|int $post Post. * @global WP_Query $wp_query */ @@ -412,9 +425,11 @@ function _amp_bootstrap_customizer() { * Redirects the old AMP URL to the new AMP URL. * If post slug is updated the amp page with old post slug will be redirected to the updated url. * - * @param string $link New URL of the post. + * @since 0.5 + * @deprecated This function is irrelevant when 'amp' theme support is added. * - * @return string $link URL to be redirected. + * @param string $link New URL of the post. + * @return string URL to be redirected. */ function amp_redirect_old_slug_to_new_url( $link ) { diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 066cf60f7d8..4718918448c 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -8,14 +8,26 @@ /** * Get the slug used in AMP for the query var, endpoint, and post type support. * - * The return value can be overridden by previously defining a AMP_QUERY_VAR + * This function always returns 'amp' when 'amp' theme support is present. Otherwise, + * the return value can be overridden by previously defining a AMP_QUERY_VAR * constant or by adding a 'amp_query_var' filter, but *warning* this ability * may be deprecated in the future. Normally the slug should be just 'amp'. * * @since 0.7 + * @since 1.0 The return value is always 'amp' when 'amp' theme support is present, and the 'amp_query_var' filter no longer applies. + * * @return string Slug used for query var, endpoint, and post type support. */ function amp_get_slug() { + if ( current_theme_supports( 'amp' ) ) { + if ( ! defined( 'AMP_QUERY_VAR' ) ) { + define( 'AMP_QUERY_VAR', 'amp' ); + } elseif ( 'amp' !== AMP_QUERY_VAR ) { + _doing_it_wrong( __FUNCTION__, esc_html__( 'The AMP_QUERY_VAR constant should only be defined as "amp" when "amp" theme support is present.', 'amp' ), '1.0' ); + } + return 'amp'; + } + if ( defined( 'AMP_QUERY_VAR' ) ) { return AMP_QUERY_VAR; } @@ -26,6 +38,8 @@ function amp_get_slug() { * Warning: This filter may become deprecated. * * @since 0.3.2 + * @since 1.0 This filter does not apply when 'amp' theme support is present. + * * @param string $query_var The AMP query variable. */ $query_var = apply_filters( 'amp_query_var', 'amp' ); @@ -39,6 +53,7 @@ function amp_get_slug() { * Retrieves the full AMP-specific permalink for the given post ID. * * @since 0.1 + * @since 1.0 The query var 'amp' is always used exclusively when 'amp' theme support is present; the 'amp_pre_get_permalink' and 'amp_get_permalink' filters do not apply. * * @param int $post_id Post ID. * @@ -46,12 +61,22 @@ function amp_get_slug() { */ function amp_get_permalink( $post_id ) { + // When theme support is present, the plain query var should always be used. + if ( current_theme_supports( 'amp' ) ) { + $permalink = get_permalink( $post_id ); + if ( ! amp_is_canonical() ) { + $permalink = add_query_arg( 'amp', '', $permalink ); + } + return $permalink; + } + /** * Filters the AMP permalink to short-circuit normal generation. * * Returning a non-false value in this filter will cause the `get_permalink()` to get called and the `amp_get_permalink` filter to not apply. * * @since 0.4 + * @since 1.0 This filter does not apply when 'amp' theme support is present. * * @param false $url Short-circuited URL. * @param int $post_id Post ID. @@ -62,15 +87,31 @@ function amp_get_permalink( $post_id ) { return $pre_url; } + $permalink = get_permalink( $post_id ); + if ( amp_is_canonical() ) { - $amp_url = get_permalink( $post_id ); + $amp_url = $permalink; } else { - $parsed_url = wp_parse_url( get_permalink( $post_id ) ); - $structure = get_option( 'permalink_structure' ); - if ( empty( $structure ) || ! empty( $parsed_url['query'] ) || is_post_type_hierarchical( get_post_type( $post_id ) ) ) { - $amp_url = add_query_arg( amp_get_slug(), '', get_permalink( $post_id ) ); + $parsed_url = wp_parse_url( get_permalink( $post_id ) ); + $structure = get_option( 'permalink_structure' ); + $use_query_var = ( + // If pretty permalinks aren't available, then query var must be used. + empty( $structure ) + || + // If there are existing query vars, then always use the amp query var as well. + ! empty( $parsed_url['query'] ) + || + // If the post type is hierarchical then the /amp/ endpoint isn't available. + is_post_type_hierarchical( get_post_type( $post_id ) ) + ); + if ( $use_query_var ) { + $amp_url = add_query_arg( amp_get_slug(), '', $permalink ); } else { - $amp_url = trailingslashit( get_permalink( $post_id ) ) . user_trailingslashit( amp_get_slug(), 'single_amp' ); + $amp_url = preg_replace( '/#.*/', '', $permalink ); + $amp_url = trailingslashit( $amp_url ) . user_trailingslashit( amp_get_slug(), 'single_amp' ); + if ( ! empty( $parsed_url['fragment'] ) ) { + $amp_url .= '#' . $parsed_url['fragment']; + } } } @@ -78,6 +119,7 @@ function amp_get_permalink( $post_id ) { * Filters AMP permalink. * * @since 0.2 + * @since 1.0 This filter does not apply when 'amp' theme support is present. * * @param false $amp_url AMP URL. * @param int $post_id Post ID. @@ -166,7 +208,7 @@ function post_supports_amp( $post ) { /** * Are we currently on an AMP URL? * - * Note: will always return `false` if called before the `parse_query` hook. + * @since 1.0 This function can be called before the `parse_query` action because the 'amp' query var is specifically and exclusively used when 'amp' theme support is added. * * @return bool Whether it is the AMP endpoint. */ @@ -175,15 +217,17 @@ function is_amp_endpoint() { return false; } - if ( amp_is_canonical() ) { + // When 'amp' theme support is (or will be added) then these are the conditions that are key to be checked. + if ( amp_is_canonical() || isset( $_GET[ amp_get_slug() ] ) ) { // WPCS: CSRF OK. return true; } - if ( 0 === did_action( 'parse_query' ) ) { - _doing_it_wrong( __FUNCTION__, sprintf( esc_html__( "is_amp_endpoint() was called before the 'parse_query' hook was called. This function will always return 'false' before the 'parse_query' hook is called.", 'amp' ) ), '0.4.2' ); + // Condition for non-theme support when /amp/ endpoint is used. + if ( false !== get_query_var( amp_get_slug(), false ) ) { + return true; } - return false !== get_query_var( amp_get_slug(), false ); + return false; } /** diff --git a/tests/test-amp-helper-functions.php b/tests/test-amp-helper-functions.php index 6debd01c0fb..06cfdc52e5a 100644 --- a/tests/test-amp-helper-functions.php +++ b/tests/test-amp-helper-functions.php @@ -87,6 +87,17 @@ public function test_amp_get_permalink_without_pretty_permalinks() { remove_filter( 'amp_pre_get_permalink', array( $this, 'return_example_url' ), 10 ); $url = amp_get_permalink( $published_post ); $this->assertContains( 'current_filter=amp_get_permalink', $url ); + remove_filter( 'amp_pre_get_permalink', array( $this, 'return_example_url' ) ); + + // Now check with theme support added (in paired mode). + add_theme_support( 'amp', array( 'template_dir' => './' ) ); + $this->assertStringEndsWith( '&', amp_get_permalink( $published_post ) ); + $this->assertStringEndsWith( '&', amp_get_permalink( $drafted_post ) ); + $this->assertStringEndsWith( '&', amp_get_permalink( $published_page ) ); + add_filter( 'amp_get_permalink', array( $this, 'return_example_url' ), 10, 2 ); + $this->assertNotContains( 'current_filter=amp_get_permalink', amp_get_permalink( $published_post ) ); // Filter does not apply. + add_filter( 'amp_pre_get_permalink', array( $this, 'return_example_url' ), 10, 2 ); + $this->assertNotContains( 'current_filter=amp_pre_get_permalink', amp_get_permalink( $published_post ) ); // Filter does not apply. } /** @@ -101,6 +112,10 @@ public function test_amp_get_permalink_with_pretty_permalinks() { $wp_rewrite->init(); $wp_rewrite->flush_rules(); + $add_anchor_fragment = function( $url ) { + return $url . '#anchor'; + }; + $drafted_post = $this->factory()->post->create( array( 'post_name' => 'draft', 'post_status' => 'draft', @@ -114,11 +129,14 @@ public function test_amp_get_permalink_with_pretty_permalinks() { 'post_status' => 'publish', 'post_type' => 'page', ) ); - $this->assertStringEndsWith( '&', amp_get_permalink( $drafted_post ) ); $this->assertStringEndsWith( '/amp/', amp_get_permalink( $published_post ) ); $this->assertStringEndsWith( '?amp', amp_get_permalink( $published_page ) ); + add_filter( 'post_link', $add_anchor_fragment ); + $this->assertStringEndsWith( '/amp/#anchor', amp_get_permalink( $published_post ) ); + remove_filter( 'post_link', $add_anchor_fragment ); + add_filter( 'amp_pre_get_permalink', array( $this, 'return_example_url' ), 10, 2 ); add_filter( 'amp_get_permalink', array( $this, 'return_example_url' ), 10, 2 ); $url = amp_get_permalink( $published_post ); @@ -128,6 +146,21 @@ public function test_amp_get_permalink_with_pretty_permalinks() { remove_filter( 'amp_pre_get_permalink', array( $this, 'return_example_url' ), 10 ); $url = amp_get_permalink( $published_post ); $this->assertContains( 'current_filter=amp_get_permalink', $url ); + remove_filter( 'amp_get_permalink', array( $this, 'return_example_url' ), 10 ); + + // Now check with theme support added (in paired mode). + add_theme_support( 'amp', array( 'template_dir' => './' ) ); + $this->assertStringEndsWith( '&', amp_get_permalink( $drafted_post ) ); + $this->assertStringEndsWith( '?amp', amp_get_permalink( $published_post ) ); + $this->assertStringEndsWith( '?amp', amp_get_permalink( $published_page ) ); + add_filter( 'amp_get_permalink', array( $this, 'return_example_url' ), 10, 2 ); + $this->assertNotContains( 'current_filter=amp_get_permalink', amp_get_permalink( $published_post ) ); // Filter does not apply. + add_filter( 'amp_pre_get_permalink', array( $this, 'return_example_url' ), 10, 2 ); + $this->assertNotContains( 'current_filter=amp_pre_get_permalink', amp_get_permalink( $published_post ) ); // Filter does not apply. + + // Make sure that if permalink has anchor that it is persists. + add_filter( 'post_link', $add_anchor_fragment ); + $this->assertStringEndsWith( '/?amp#anchor', amp_get_permalink( $published_post ) ); } /** @@ -164,6 +197,33 @@ public function test_amp_remove_endpoint() { $this->assertEquals( 'https://example.com/foo/?blaz', amp_remove_endpoint( 'https://example.com/foo/amp/?blaz' ) ); } + /** + * Test is_amp_endpoint() function. + * + * @covers \is_amp_endpoint() + */ + public function test_is_amp_endpoint() { + $this->assertFalse( is_amp_endpoint() ); + + // Legacy query var. + set_query_var( amp_get_slug(), '' ); + $this->assertTrue( is_amp_endpoint() ); + unset( $GLOBALS['wp_query']->query_vars[ amp_get_slug() ] ); + $this->assertFalse( is_amp_endpoint() ); + + // Paired theme support. + add_theme_support( 'amp', array( 'template_dir' => './' ) ); + $_GET['amp'] = ''; + $this->assertTrue( is_amp_endpoint() ); + unset( $_GET['amp'] ); + $this->assertFalse( is_amp_endpoint() ); + remove_theme_support( 'amp' ); + + // Native theme support. + add_theme_support( 'amp' ); + $this->assertTrue( is_amp_endpoint() ); + } + /** * Filter calls. * From ab99cd7606c00cbeadf01684a9b37685963165b7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 1 Jun 2018 23:33:19 -0700 Subject: [PATCH 2/2] Note pre_amp_render_post action being not called when amp theme support added --- amp.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/amp.php b/amp.php index a3911bb3396..79e32abed81 100644 --- a/amp.php +++ b/amp.php @@ -331,10 +331,11 @@ function amp_add_post_template_actions() { * Add action to do post template rendering at template_redirect action. * * @since 0.2 + * @since 1.0 The amp_render() function is called at template_redirect action priority 11 instead of priority 10. * @deprecated This function is not used when 'amp' theme support is added. */ function amp_prepare_render() { - add_action( 'template_redirect', 'amp_render' ); + add_action( 'template_redirect', 'amp_render', 11 ); } /** @@ -390,6 +391,8 @@ function amp_render_post( $post ) { /** * Fires before rendering a post in AMP. * + * This action is not triggered when 'amp' theme support is present. Instead, you should use 'template_redirect' action and check if `is_amp_endpoint()`. + * * @since 0.2 * * @param int $post_id Post ID.