Redirects the old AMP URL to the new AMP URL if post slug is changed. #560 #593

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@rahulsprajapati
Contributor
rahulsprajapati commented Dec 22, 2016 edited

Fixed the old amp url redirect issue after post slug is changed. #560

@rahulsprajapati rahulsprajapati Redirects the old AMP URL to the new AMP URL if post slug is changed.
Signed-off-by: Rahul Prajapati <rahul.prajapati@live.in>
caa6d7a
amp.php
+ *
+ * @return string $link URL to be redirected.
+ */
+function amp_redirect_to_new_url( $link ) {
@mjangda
mjangda Jan 1, 2017 Member

Can we rename this to amp_redirect_old_slug_to_new_url?

@rahulsprajapati
rahulsprajapati Jan 3, 2017 Contributor

yeah sure. :)

amp.php
+function amp_redirect_to_new_url( $link ) {
+
+ if ( is_amp_endpoint() ) {
+ $link = trailingslashit( $link ) . AMP_QUERY_VAR;
@mjangda
mjangda Jan 1, 2017 Member

One possible issue here is if someone is using a different permalink structure for AMP URLs, this will break for them.

I don't see a simple fix for this though because the filter does not pass the post id (so we may just have to accept it as a known issue): https://core.trac.wordpress.org/browser/tags/4.7/src/wp-includes/query.php#L904

@mjangda
mjangda Jan 3, 2017 Member

Core ticket to add $id to the filter: https://core.trac.wordpress.org/ticket/39442

@rahulsprajapati
rahulsprajapati Jan 3, 2017 Contributor

This would be nice if we get id also in filter.

But about changing amp url can you please explain the issue because I can see that we are using the link as per modified permalink itself https://core.trac.wordpress.org/browser/tags/4.7/src/wp-includes/query.php#L896 .

Also we already added amp_query_var filter for AMP_QUERY_VAR so when the url is going to change this can also change. https://github.com/Automattic/amp-wp/blob/master/amp.php#L52

Please let me know if I am missing something.

Thank you:)

@mjangda
mjangda Jan 9, 2017 Member

You are right that using AMP_QUERY_VAR does protect us in case someone changes the value from amp to something else. However, the plugin does provide a filter that let's you change the AMP url so it could look very different from the default, e.g. /amp/2017/01/01/this-post-is-great/

This is a bit of an edge case though, so I think we can merge as-is for now.

@rahulsprajapati rahulsprajapati Rename callback function from "amp_redirect_to_new_url" to "amp_redir…
…ect_old_slug_to_new_url".

Signed-off-by: Rahul Prajapati <rahul.prajapati@live.in>
b9771da
@mjangda
Member
mjangda commented Jan 9, 2017

Merged in db6aed7

@mjangda mjangda closed this Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment