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

Detect /amp/ suffix when determining AMP request #9625

Merged
merged 4 commits into from
May 24, 2018
Merged

Detect /amp/ suffix when determining AMP request #9625

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dda40b0
Detect /amp/ suffix when determining AMP request
gravityrail May 23, 2018
567f897
Use more reliable methods of detecting path and suffix
gravityrail May 23, 2018
8bf1c03
Check for AMP__VERSION and restrict suffix checking to AMP__VERSION <…
gravityrail May 23, 2018
3346f19
Add explanatory comment
gravityrail May 23, 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
7 changes: 6 additions & 1 deletion 3rd-party/class.jetpack-amp-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ static function is_amp_request() {
&&
function_exists( 'amp_is_canonical' ) // this is really just testing if the plugin exists
&&
( amp_is_canonical() || isset( $_GET[ amp_get_slug() ] ) );
( amp_is_canonical() || isset( $_GET[ amp_get_slug() ] ) || self::has_amp_suffix() );
Copy link
Contributor

Choose a reason for hiding this comment

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

The last part in this conditional should be wrapped in a check for AMP__VERSION being less than 1.0 since with ampproject/amp-wp#1148 there will no longer be an /amp/ endpoint, and this will help guard against the case when a user publishes a page about guitar amps at like /guitar/amp/, to avoid ambiguity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


/**
* Returns true if the current request should return valid AMP content.
Expand All @@ -78,6 +78,11 @@ function_exists( 'amp_is_canonical' ) // this is really just testing if the plug
return apply_filters( 'jetpack_is_amp_request', $is_amp_request );
}

static function has_amp_suffix() {
$request_path = parse_url( $_SERVER['REQUEST_URI'], PHP_URL_PATH );
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to use wp_parse_url() instead of parse_url(). Prior to PHP 5.4, parse_url() didn't support relative URLs such as parsing the bare path. Alternatively, it would probably be better to use strtok instead:

$request_path = strtok( $_SERVER['REQUEST_URI'], '?#' );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

return $request_path && substr_compare( $request_path, '/amp/', -4, 5, true );
Copy link
Contributor

@westonruter westonruter May 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This wouldn't handle the case when a user has filtered user_trailingslashit to not use trailing slashes. I think it would be better to do:

return (bool) preg_match( '#/amp/?$#i', $request_path );

This will ensure that the slash is optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}

static function filter_available_widgets( $widgets ) {
if ( self::is_amp_request() ) {
$widgets = array_filter( $widgets, array( 'Jetpack_AMP_Support', 'is_supported_widget' ) );
Expand Down