Detect /amp/ suffix when determining AMP request #9625
Conversation
gravityrail
added
AMP
[Status] Needs Review
| @@ -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 ); | |||
There was a problem hiding this comment.
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'], '?#' );| @@ -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 ); | |||
| return $request_path && substr_compare( $request_path, '/amp/', -4, 5, true ); | |||
There was a problem hiding this comment.
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.
| @@ -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() ); | |||
There was a problem hiding this comment.
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.
|
Feedback addressed, thanks @westonruter |
jeherve
removed
the
[Status] Needs Review
Most AMP sites are currently configured to use the
/amp/suffix as the canonical path to AMP content.In Jetpack's AMP support code, we need to hook earlier than
parse_queryin order to properly intercept and modify Jetpack's output, however this means we can't use the standard means to detect/amp/from WP_Query and must roll out own solution that assumes that/amp/is truly the AMP path for the site.The downside is that if the site has changed it to something else, e.g.
/mobile/or/lite/, we won't detect it. The upside is that 99% of sites just go with the default, so this provides a workable solution for them.cc @westonruter