_wp_old_slug redirect loses ?amp on posts with changed slugs

[archon810]

Hi,

We're using AMP WP 1.2.2, and I noticed several issues in Search Console's AMP section. The issue is when a post's slug is changed and _wp_old_slug kicks in, the redirect loses ?amp and ends up redirecting to a non-AMP page.

For example:
https://www.androidpolice.com/2019/09/25/google-engineer-web-app-spot-satellites-bare-eyes/?amp
gets redirected to
https://www.androidpolice.com/2019/09/25/google-app-satellites-steet-view/
instead of
https://www.androidpolice.com/2019/09/25/google-app-satellites-steet-view/?amp

I noticed similar fixed issues but seems like there may be a regression - possibly because they were dealing with /amp instead of ?amp: #560, #649.

[westonruter]

I'm not seeing this when testing in my development environment. What I'm doing to test:

1. Set mode to Transitional.
2. Create a post with title: "google-engineer-web-app-spot-satellites-bare-eyes".
3. Verify the AMP version is available at /2019/09/26/google-engineer-web-app-spot-satellites-bare-eyes/?amp.
4. Change post title (and slug) to: “google-app-satellites-steet-view”
5. Try navigating to /2019/09/26/google-engineer-web-app-spot-satellites-bare-eyes/?amp.

I successfully get redirected to the expected URL: /2019/09/26/google-app-satellites-steet-view/?amp

[maciejmackowiak]

@westonruter I was able to reproduce @archon810 bug
I've followed your steps and it worked correctly but the bug occurred after changing "Supported Templates" to this:

and then navigating to old post slug with ?amp I've got redirected first to the old slug without ?amp and then to new slug.

[archon810]

Does enabling Not Found (404) fix the issue?

Are there any adverse effects for us, given that we didn't implement specific support for 404 AMP pages?

[maciejmackowiak]

Yes, enabling Not Found (404) fix the issue.

[schlessera]

The "redirect on 404" is directly tied to the 404 template, as it seems. We should try to hook into the redirect mechanism, it is probably just stripping unknown query vars.

[westonruter]

I think this should do the trick:

diff --git a/amp.php b/amp.php
index b11d3862..a7aa9d2b 100644
--- a/amp.php
+++ b/amp.php
@@ -734,14 +734,13 @@ add_action( 'plugins_loaded', '_amp_bootstrap_customizer', 9 ); // Should be hoo
  * If post slug is updated the amp page with old post slug will be redirected to the updated url.
  *
  * @since 0.5
- * @deprecated This function is irrelevant when 'amp' theme support is added.
  *
  * @param string $link New URL of the post.
  * @return string URL to be redirected.
  */
 function amp_redirect_old_slug_to_new_url( $link ) {
 
-	if ( is_amp_endpoint() && ! amp_is_canonical() ) {
+	if ( ! amp_is_canonical() && false !== get_query_var( amp_get_slug(), false ) ) {
 		if ( current_theme_supports( AMP_Theme_Support::SLUG ) ) {
 			$link = add_query_arg( amp_get_slug(), '', $link );
 		} else {

The problem is that is_amp_endpoint() will return false on the 404 template even though the amp query var is present, when AMP is not enabled for the 404 template.

[schlessera]

Wouldn't you still need is_amp_endpoint() in there? Not every endpoint has the URL argument...

[schlessera]

Ah, nevermind, this is probably not used in that case then.

[archon810]

For now I've enabled the checkbox, though this sort of "broke" our 404s on AMP: https://www.androidpolice.com/2019/09/25/asdf/?amp vs https://www.androidpolice.com/2019/09/25/asdf/.

[archon810]

We rolled out a fixed version of the 404 page for AMP now to mitigate this.

[westonruter]

The broken 404 will be fixed in 1.3 via #3039.

[westonruter]

This needs to be re-checked for whether it is still an issue.

[westonruter]

This needs to be re-checked for whether it is still an issue.

Confirmed it is still an issue. Just pushed up fix (6cae4ee) as part of #5558.

[westonruter]

QA Passed

As noted in #2062 (comment), I tried creating a post with a slug foo and after publishing I changed the slug to bar. Then when attempting to go to /2021/04/25/foo/amp/ I was redirected to /2021/04/25/foo/?amp=1 and then I was redirected to /2021/04/25/bar/?amp=1.

So this is working as expected.