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.

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

Consider automatically redirecting /amp/ to ?amp in case of 404 #2062

Closed
westonruter opened this issue Apr 4, 2019 · 1 comment · Fixed by #5558
Closed

Consider automatically redirecting /amp/ to ?amp in case of 404 #2062

westonruter opened this issue Apr 4, 2019 · 1 comment · Fixed by #5558
Assignees
Labels
Groomed P0 High priority Routing Handling URLs WS:Core Work stream for Plugin core
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Apr 4, 2019

As reported in a support forum topic:

I tried to find solution for my problem but unable to.
I want to change the URL format from
https://hinditips.com/best-ayurvedic-tips-for-hair-growth/?amp
to
https://hinditips.com/best-ayurvedic-tips-for-hair-growth/amp/

How can it be done.

Because the previous plugin that I have used has the same structure. Now i switched to this plugin. Search engines may find those old URLs as 404.

The alternative to this I suggested is actually there should just be a redirect from /amp/ to ?amp in the case of 404, such as when migrating to this AMP plugin.

The amp query var works always works for serving AMP regardless of the mode and it always avoids returning a 404 when the plugin gets deactivated. This also ensures that AMP requests don't fail when the rewrite rules have not been flushed. It is overall better to use ?amp instead of /amp/.

Proof of concept plugin: https://gist.github.com/westonruter/6b7d3b168711ee03c4ee29b68c3719b2


When using another plugin that tries to use /amp/ for all URLs, switching to this AMP plugin can result in non-singular URLs 404ing. To deal with that, I created a helper migration plugin: https://gist.github.com/westonruter/6b7d3b168711ee03c4ee29b68c3719b2

Nevertheless, this seems like something that would make sense to be part of the the AMP plugin directly. In particular, it would make sense to add after #2204 is figured out.


A related issue came up in #474 (comment).

Namely, if someone originally had /amp/ for pages, upon switching to the AMP plugin these URLs won't work anymore and they may end up getting routed to WordPress's own canonical redirect system, so in that case /some-page/amp/ will redirect to /some-page/ and not /some-page/?amp as expected. While we don't want to register an amp endpoint for pages, we should perhaps hook into WordPress's canonical redirects to check for /amp/ being present, and make sure that WordPress will redirect to ?amp (or ?amp=1 per #2204) instead of removing it from the URL altogether.

@westonruter
Copy link
Member Author

QA Passed

I set the Template Mode to Transitional and the Paired URL Structure to “Query parameter”.

I then tried going to /sample-page/amp/ and I was redirected to /sample-page/?amp=1.

Also, I tried going to /this-page-does-not-exist/amp/ and I was redirected to /this-page-does-not-exist/?amp=1.

I then 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 all working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groomed P0 High priority Routing Handling URLs WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants