Amp-wp: Default to ?amp=1 query param instead of /amp/ endpoint slug in all scenarios (but persist legacy config and allow customization)

[westonruter]

Feature description

We should consider eliminating the /amp/ in favor of ?amp everywhere. The endpoint slug is unnecessary for SEO, and it improve consistency when switching between the paired modes (transitional and reader). Additionally, there will no longer be a need to flush rewrite rules.

See also #2062 to automatically redirect from /amp/ to ?amp when is_404().

See also #1390 where ?amp=1 should be considered instead of ?amp.

See also #1389 where the AMP plugin should automatically convert all URLs in AMP pages to point to other AMP pages in Transitional mode.

In order to prevent breaking analytics, SEO, and other things… these changes would need to not happen for existing sites. They should only apply to new plugin installs. Furthermore, the specific AMP URL format should be configurable (yes, including the allowing of a site to use /amp/).

Also relates to #2817 where there could be a filter added to is_amp_endpoint() to determine if a request is AMP or not. This would be a necessary corollary to allowing the URL for AMP pages to be arbitrary filterable: there would need to be a way to reverse the process.

Example AMP URLs that should be supported:

Type | Example
-----|---------
Endpoint | https://www.example.com/…/foo/amp/
Root directory | https://www.example.com/amp/…/foo/
File extension | https://www.example.com/…/foo.amp
Bare query var | https://www.example.com/…/foo/?amp
Query var with value | https://www.example.com/…/foo/?amp=1
Subdomain | https://amp.example.com/…/foo/

In each case, the amp corresponds to whatever is returned by amp_get_slug().

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

1. For new plugin installs, replace all instances of /amp/ in favor of ?amp
2. The specific AMP URL format should be configurable (yes, including the allowing of a site to use /amp/).

Implementation brief

• ~Let amp_add_paired_endpoint() allow the $url to be omitted, and when this is done, pass either the current WP_Query (if available) or the WP_Post (if available) for the URL in question. Or else allow for the WP_Query or WP_Post to be added for every occurance of the aforementioned functions?~
• Introduce a new PairedAmpRouting service.
  
  
  
  • Introduce a new AMP option for the AMP permalink_structure.
  
  
  • Move the rewrite endpoint registration from amp_init() to this service.
  
  
• Add a new settings drawer for “Paired URL Structure” which is shown only when Reader mode or Transitional mode is selected. This is similar to the Permalinks admin screen in WordPress core. Four options are available:
  
  
  
  • Query var: ?amp=1 everywhere (the default for new installs).
  
  
  • Endpoint: /amp/ whenever possible (or else ?amp=1).
  
  
  • Legacy Reader: /amp/ for posts and ?amp for everything else (default for sites upgraded when in legacy Reader mode).
  
  
  • Legacy Transitional: ?amp for everything (default for sites upgraded when in Transitional mode or non-legacy Reader mode).
  
  
  • Custom: This will not be an option _per se_, but if the site has a given filter in place to modify the AMP URL structure, then the other options will not be available and this will be the forced option.
  
  
• Update these functions which manipulate AMP URLs to invoke corresponding methods in the service:
  
  
  
  • amp_add_paired_endpoint()
  
  
  • amp_remove_paired_endpoint()
  
  
  • amp_has_paired_endpoint()
  
  

QA testing instructions

Demo

Changelog entry

[westonruter]

Related: #860, #1383, #2062, #314.

[archon810]

For those savvy in Google Analytics, with ?amp instead of /amp, is there a way to not see it as ?amp= in all Analytics urls because that looks... not good?

https://twitter.com/ArtemR/status/1157083403193212928

[westonruter]

@archon810 Probably best to inquire of the Analytics support forum: https://support.google.com/analytics/community?hl=en

See also https://support.google.com/analytics/answer/6343176?hl=en

[jauyong]

Notes from grooming:

• This also came up in support forums: https://wordpress.org/support/topic/customize-the-amp-url/
• Discussed using filters vs URL manipulator. Decided to go with URL manipulator

[westonruter]

Note: Use of anything but a query param will make site searching difficult to serve as AMP. Currently if you are on an AMP page and you try doing a search, the search form automatically gets <input name="amp" value="" type="hidden"> added and the resulting URL after searching for “foo” will be https://example.com/?s=foo&amp=. If the site is configured to only recognize an /amp/ endpoint then this will break.

[martn234]

hi, can it be possible to show /amp/ instead of /?amp through amp wordpress plugin

[westonruter]

hi, can it be possible to show /amp/ instead of /?amp through amp wordpress plugin

@martn234 That is what this issue is about making possible. However, this PR is about making it _possible_, not actually implementing it. The default behavior of the AMP plugin will be to create AMP URLs that end in /?amp=1. There is no reason other than for style reasons to use /amp/. In fact, it causes many headaches to use the /amp/ endpoint. It can possibly cause conflicts which prevent being able to serve some site URLs as AMP.

[westonruter]

A support forum use case from @janvitos for having /amp/ as the root directory (_startpoint_?) instead of as an endpoint:

So right now, the AMP plugin adds /amp at the end of URLs. Quite frankly, I have no problem with that. But when using a CloudFlare Worker, I cannot have the Worker bypass that path because CloudFlare does not provide a way to properly use wildcards (*) in the middle of a URL.

Instead, CloudFlare only allows using wildcards at the end of the URL, but not in the middle. So if I wanted a Worker to bypass an AMP URL, it wouldn’t work since the URL would need to be like this: http://www.website.com/*/amp/.

So to make it work, /amp needs to be at the beginning of the URL so I can write my Worker bypass route like this: http://www.website.com/amp/*

So is this something that can be acheieved with this plugin? Any WordPress filter I can use?

[westonruter]

Another case where the /amp/ endpoint causes problems: https://github.com/ampproject/amp-wp/issues/4442

[westonruter]

Related issue: #4466.

When attempting to access /amp/ for a URL and this results in a 404, we should redirect to ?amp=1 automatically.

[westonruter]

As part of this, we need to make sure that query vars from the non-AMP page are persisted on to the linked AMP page. So that would be something like this given the current codebase:

diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php
index dd14486ca..b7f1ecbfd 100644
--- a/includes/amp-helper-functions.php
+++ b/includes/amp-helper-functions.php
@@ -690,7 +690,7 @@ function amp_add_amphtml_link() {
        return;
    }

-   if ( AMP_Theme_Support::is_paired_available() ) {
+   if ( AMP_Theme_Support::is_paired_available() || ! empty( $_GET ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
        $amp_url = add_query_arg( amp_get_slug(), '', amp_get_current_url() );
    } else {
        $amp_url = amp_get_permalink( get_queried_object_id() );
@@ -1715,7 +1715,7 @@ function amp_add_admin_bar_view_link( $wp_admin_bar ) {

    if ( is_amp_endpoint() ) {
        $href = amp_remove_endpoint( amp_get_current_url() );
-   } elseif ( is_singular() ) {
+   } elseif ( is_singular() && empty( $_GET ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
        $href = amp_get_permalink( get_queried_object_id() ); // For sake of Reader mode.
    } else {
        $href = add_query_arg( amp_get_slug(), '', amp_get_current_url() );

[westonruter]

Another support forum topic about consequences of AMP URLs changing from /amp/ to ?amp: https://wordpress.org/support/topic/chanhe-amp-query-with-subdirectory-amp/

For some reason, the URL change caused Google Discover to stop showing AMP results from a site, even though Google Search did show AMP results.

So we need to minimize URL changes as much as possible when switching modes.

For brand new installs, we can default to ?amp=1. But if someone was previously on /amp/ in Reader mode and they switch to Transitional mode or a Reader theme, then /amp/ should be persisted for singular non-hierarchical post types.

[westonruter]

Now with #4312 merged, we have these 3 functions to build upon:

• amp_has_paired_endpoint()
• amp_add_paired_endpoint()
• amp_remove_paired_endpoint()

Now, in order to implement the legacy URL behavior where /amp/ is used for singular posts but ?amp is used for singular pages and other queries, we may need to pass additional context to figure how how the endpoint should be added. Currently amp_add_paired_endpoint() is only provided the $url. Should it also be provided a WP_Query or a WP_Post for context? This would work for the main query. It wouldn't work when obtaining the AMP URLs for links on a page, however, for AMP-to-AMP linking. However, the most important AMP URL is the one that is generated for the link[rel=amphtml], and this _would_ have that main query for context.

If we wanted to also make sure that the AMP URLs for AMP-to-AMP linking on an AMP page also have the “canonical” paired AMP URLs (/amp/ instead of ?amp), then perhaps url_to_postid() could be used. We did consider using that in the beginning when first adding AMP-to-AMP linking, but decided it wasn't necessary. But if we did, then this could also check amp_skip_post() to see if the AMP link should be added in the first place. (It could be excluded from AMP-to-AMP linking automatically.)

[westonruter]

If that is the case, should amp_add_paired_endpoint() be able to return null if AMP is not available for the provided $url?

[westonruter]

If we do go that route to honor amp_skip_post() then we'd need to do the same for a proposed more generic availability filter in https://github.com/ampproject/amp-wp/issues/2817.

[westonruter]

In order to successfully support users migrating from other plugins, we probably need to register the /amp/ endpoint for EP_ALL rather than just EP_PERMALINK, but only do so if the user specifically selects /amp/. The supported AMP permalink options that the AMP plugin would support would be (when not in Standard mode):

• Query var: ?amp=1 everywhere (the default for new installs).
• Endpoint: /amp/ whenever possible (or else ?amp=1).
• Legacy Reader: /amp/ for posts and ?amp for everything else (default for sites upgraded when in legacy Reader mode).
• Legacy Transitional: ?amp for everything (default for sites upgraded when in Transitional mode or non-legacy Reader mode).
• Custom: This will not be an option _per se_, but if the site has a given filter in place to modify the AMP URL structure, then the other options will not be available and this will be the forced option.

The presentation of the setting, in a new AMP settings drawer, can emulate the Permalink Settings screen:

[mkormendy]

Yay, the latest version of the plugin, installed as of this past Friday has now broken my site for /amp/ suffixed URLs.

[westonruter]

@mkormendy What is your site's URL? We haven't released any changes yet.

[mkormendy]

https://www.smartstartinc.com/locations/ignition-interlock-texas-irving-4848-plaza-dr-75063/amp/ (original not working)

https://www.smartstartinc.com/locations/ignition-interlock-texas-irving-4848-plaza-dr-75063/?amp (test working)

https://www.smartstartinc.com/locations/ignition-interlock-texas-irving-4848-plaza-dr-75063/?amp= (test working)

https://www.smartstartinc.com/locations/ignition-interlock-texas-irving-4848-plaza-dr-75063/?amp=1 (test working)

Flushed permalinks & cache. Still no dice for the not working scenarios.

[mkormendy]

I'll just leave it .. if the AMP project is moving towards the query-string-preferred approach, I'll just make a catchall in my server redirects.

[westonruter]

These three are working for me:

https://www.smartstartinc.com/locations/ignition-interlock-texas-irving-4848-plaza-dr-75063/?amp (test working)

https://www.smartstartinc.com/locations/ignition-interlock-texas-irving-4848-plaza-dr-75063/?amp= (test working)

https://www.smartstartinc.com/locations/ignition-interlock-texas-irving-4848-plaza-dr-75063/?amp=1 (test working)

For each an AMP page is served.

For the non-AMP page, I'm seeing something very strange in the source:

<link rel="amphtml" href="https://www.smartstartinc.com/locations/%location_categories%/%location_categories%/ignition-interlock-texas-irving-4848-plaza-dr-75063/amp/">

It appears that get_permalink() is not returning a correct URL on your site. The logic for constructing the href for the amphtml link can be seen here:

https://github.com/ampproject/amp-wp/blob/7fcdabd53ae0ae7d86a5a93b446dd4bfb5fa4046/includes/amp-helper-functions.php#L729-L769

This is not unique to the AMP plugin. I can see this same problem for other such URLs on your page:

<link rel="canonical" href="https://www.smartstartinc.com/locations/%location_categories%/%location_categories%/ignition-interlock-texas-irving-4848-plaza-dr-75063/" />

<meta property="og:url" content="https://www.smartstartinc.com/locations/%location_categories%/%location_categories%/ignition-interlock-texas-irving-4848-plaza-dr-75063/" />

So there seems to be a problem with how your site is constructing permalinks and perhaps this also is causing problems with the rewrite rules to properly route the requests.

[mkormendy]

@westonruter So I was able to fix the issue with get_permalink() not returning a correct URL for permalinks on my site - that was an issue in my logic, thanks for catching that - it fixed the _canonical_, the _ampurl_ and the _opengraph_ meta link tags on my site for this custom taxonomy structure for our shop locations.

Unfortunately though, the /amp/ directory version still returns 404 Not Found.

Since I've read multiple times that the query string ?amp version is now favored over the directory /amp/ version, I managed to simply use your block of code to force the query string version instead:

add_filter( 'amp_pre_get_permalink', function( $pre, $post_id ) {
    return add_query_arg( amp_get_slug(), '', get_permalink( $post_id ) );
}, 10, 2 );

I'll also add a redirect on the server to catch any indexed URLs that had the directory version and point them to the query string version.

[westonruter]

The default going forward is actually going to be ?amp=1, so of you are switching to use query vars now, then I recommend providing the value of 1 rather than an empty string.

[mkormendy]

Okay, thanks for the heads up. Just made the change in my redirects. Will amp_get_slug() output that in future versions?

[mkormendy]

Oh, and why can't I find the setting in the plugin UI to choose between the two? Somehow my site is forced to use /amp/ (maybe from a previous version where the interface had the choice to choose which URL structure to use), but using the amp_pre_get_permalink now superfluously undoes that? I guess I could go into the options table to see what is set there if the interface doesn't allow new users new to installing this amp plugin to select that (because it's being defaulted to the query string)?

[westonruter]

Will amp_get_slug() output that in future versions?

No, it will output just amp.

Oh, and why can't I find the setting in the plugin UI to choose between the two.

The setting to switch the Paired URL structure is only available on the Settings screen of the 2.1-alpha build that is linked to on the PR: https://github.com/ampproject/amp-wp/pull/5558. It's not yet in a stable release.

[mkormendy]

Will amp_get_slug() output that in future versions?

No, it will output just amp.

So what's the best way to get the output to use ?amp=1?

[westonruter]

Same as you had but add 1 instead of '':

add_filter( 'amp_pre_get_permalink', function( $pre, $post_id ) {
    return add_query_arg( amp_get_slug(), 1, get_permalink( $post_id ) );
}, 10, 2 );

Note: This only works in Reader mode.

[mkormendy]

Great minds think alike! That's what I was going to do, but wanted to make sure it wasn't being added somehow else.
I'm using Reader mode on my site, so that's good to know. Thanks for all of your help.

[dikeii]

Good morning, I needed to use the url format "https://www.example.com/amp/…/foo/" and do the following

add_action( 'request', 'reset_permalinks' );
function reset_permalinks($query_vars) {

    $keys = parse_url($_SERVER['REQUEST_URI']); // parse the url
    $path = explode("/", $keys['path']); // splitting the path
    $last = end($path); // get the value of the last element 

    if(strpos($_SERVER['REQUEST_URI'], "/amp/") !== false || $last == "amp"){
        $query_vars['amp'] = 1;
    } 
    return $query_vars;   
}

It works fine for me, but I wanted to know if it was the best way to do it, thanks for the plugin. Best regards

[westonruter]

@dikeii Please try this plugin code: https://gist.github.com/westonruter/f881658cb1bc5a03cc432c8e2b89102a

Note that this will only work if you are in Reader template mode with the AMP Legacy theme selected, unless you are testing with https://github.com/ampproject/amp-wp/pull/5558 (in which it will work with any variation of Transitional or Reader mode).

[dikeii]

@westonruter thanks it works perfectly