Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let query var always be used and always be 'amp' when theme support is added #1194

Merged
merged 2 commits into from
Jun 3, 2018
Merged

Let query var always be used and always be 'amp' when theme support is added #1194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4ba86c
Update amp_get_slug() and amp_get_permalink() to always use 'amp' que…
westonruter Jun 2, 2018
ab99cd7
Note pre_amp_render_post action being not called when amp theme suppo…
westonruter Jun 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions amp.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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';
Expand All @@ -322,15 +331,18 @@ 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 );
}

/**
* 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.
Expand All @@ -345,6 +357,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
*/
Expand Down Expand Up @@ -377,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.
Expand Down Expand Up @@ -412,9 +428,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 ) {

Expand Down
68 changes: 56 additions & 12 deletions includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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' );
Expand All @@ -39,19 +53,30 @@ 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.
*
* @return string AMP permalink.
*/
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.
Expand All @@ -62,22 +87,39 @@ 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'];
}
}
}

/**
* 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.
Expand Down Expand Up @@ -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.
*/
Expand All @@ -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;
}

/**
Expand Down
62 changes: 61 additions & 1 deletion tests/test-amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', amp_get_permalink( $published_post ) );
$this->assertStringEndsWith( '&amp', amp_get_permalink( $drafted_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.
}

/**
Expand All @@ -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',
Expand All @@ -114,11 +129,14 @@ public function test_amp_get_permalink_with_pretty_permalinks() {
'post_status' => 'publish',
'post_type' => 'page',
) );

$this->assertStringEndsWith( '&amp', 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 );
Expand All @@ -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', 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 ) );
}

/**
Expand Down Expand Up @@ -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.
*
Expand Down