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

With pretty permalinks enabled, adding /amp/ will break rewrite #860

Closed
Airell opened this issue Jan 14, 2018 · 22 comments
Closed

With pretty permalinks enabled, adding /amp/ will break rewrite #860

Airell opened this issue Jan 14, 2018 · 22 comments
Labels
Bug Something isn't working

Comments

@Airell
Copy link

Airell commented Jan 14, 2018

When pretty permalinks enabled, AMP will add /amp/ to the url, but this will break the automatic rewrite rule when you set the site address different from the wordpress address.

My wordpress address is 'blog.oracle48.nl/wordpress', but in WP settings I have set 'blog.oracle48.nl' as site address. This will make 'blog.oracle48.nl/wordpress/post/' goto 'blog.oracle48.nl/post/' and works out of the (wordpress) box.

But whem /amp/ is added (by LinkedIn or other site), this rewrite does not work any more. 'blog.oracle48.nl/wordpress/post/amp/' does not get rewritten to 'blog.oracle48.nl/post/amp/' and visitors will be shown the 404 page.

@Airell Airell changed the title With pretty permalinks enabled, +/amp will break rewrite With pretty permalinks enabled, adding /amp/ will break rewrite Jan 14, 2018
@ThierryA
Copy link
Collaborator

Hi @Airell, I quickly spinned up a test site and cannot reproduce the issue, the redirect works fine on my side even with WordPress installed in a subdirectory. Which method described in this article are you using?

I am closing this for now, it may be reopened if more people are facing the same issue

@Airell
Copy link
Author

Airell commented Jan 16, 2018

Hi @ThierryA,

I'm using method number 2, with URL change. I have copied .htaccess and the index.php and changed the root's index.php.

The next rewrite works:
http://blog.oracle48.nl/wordpress/oracle-linux-patches-for-meltdown-and-spectre-information/
Changes to:
http://blog.oracle48.nl/oracle-linux-patches-for-meltdown-and-spectre-information/

Now with AMP, the URL is:
http://blog.oracle48.nl/oracle-linux-patches-for-meltdown-and-spectre-information/amp/

But the rewrite from the previous example does not work when /amp/ is added to the url:
http://blog.oracle48.nl/wordpress/oracle-linux-patches-for-meltdown-and-spectre-information/amp/

I'm using the lastest Wordpress, my .htaccess is changed by Wordpress (step 11.) to:

# BEGIN WordPress
<IfModule mod_rewrite.c>
RewriteEngine On
RewriteBase /
RewriteRule ^index\.php$ - [L]
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule . /index.php [L]
</IfModule>
# END WordPress

Regards, Airell.

@ThierryA ThierryA reopened this Jan 17, 2018
@ThierryA
Copy link
Collaborator

Thanks for providing more details @Airell, I could reproduce the issue. I did some very quick research and the AMP query var is altering the logic in the redirect_canonical() function here.

@ThierryA ThierryA added the Bug Something isn't working label Jan 17, 2018
@aronmoshe-m
Copy link

Is there a resolution to this issue yet? I saw elsewhere guidance to change /amp/ to ?amp for the endpoint, but this still doesn't seem to resolve the issue for us.

@amedina
Copy link
Member

amedina commented Jan 10, 2019

What version of the plugin are you using? The amp slug is not used anymore. Now in paired mode, the AMP version of a page is accessed via the ?amp query parameter.

@westonruter
Copy link
Member

The /amp/ slug is still used in Classic mode. I assume that is the mode in question here?

@mkormendy
Copy link

mkormendy commented Jul 6, 2019

I am having this issue as well. Visiting the WP > Settings > Permalinks and clicking the [Save Changes] button appears to fix it, as well as running the CLI rewrite flush command. However any time that flush_rewrite_rules(true); is ran (lets say with another plugin like WP Migrate Pro or with an automated process), the Permalinks for AMP pages are broken again.

I am using version (1.1.3). I can't change the slug from /amp/ to ?amp as there's no option to do so in my AMP settings (classic mode).

@westonruter
Copy link
Member

Aside: The latest version of the plugin is 1.2.0, not 1.1.3.

What happens if you force the use of ?amp instead of /amp/? See #1148 (comment)

@westonruter
Copy link
Member

I'm testing on my install in Reader mode and when I do wp rewrite flush the expected rewrite rules are present for the post post type

$ wp rewrite flush && wp rewrite list --source=post | grep amp
Success: Rewrite rules flushed.
| ([0-9]{4})/([0-9]{1,2})/([0-9]{1,2})/([^/]+)/amp(/(.*))?/?$                              | index.php?year=$matches[1]&monthnum=$matches[2]&day=$matches[3]&name=$matches[4]&amp=$matches[6]   | post   |
| ([0-9]{4})/([0-9]{1,2})/([0-9]{1,2})/amp(/(.*))?/?$                                      | index.php?year=$matches[1]&monthnum=$matches[2]&day=$matches[3]&amp=$matches[5]                    | post   |
| ([0-9]{4})/([0-9]{1,2})/amp(/(.*))?/?$                                                   | index.php?year=$matches[1]&monthnum=$matches[2]&amp=$matches[4]                                    | post   |
| ([0-9]{4})/amp(/(.*))?/?$                                                                | index.php?year=$matches[1]&amp=$matches[3]                                                         | post   |

@westonruter
Copy link
Member

westonruter commented Jul 6, 2019

mkendall07 @mkormendy Does the /amp/ URL show a 404 or just the regular post as if the endpoint was not added?

Issue may have been reported here as well: https://wordpress.org/support/topic/plugin-does-not-appear-to-work-2/

@mkormendy
Copy link

mkormendy commented Jul 6, 2019

@westonruter not sure who mkendall07 is .. but maybe you mistyped. Sorry 1.1.3 is considered "latest" by my WordPress installation - it hasn't asked me to update the plugin to a newer version, 1.2.0 or otherwise. Spoke too soon, I've updated, tested, same issue.

I get 404 pages for both ?amp (forced) and /amp/.

Yes .. wp rewrite flush will fix rewrites for AMP as does [Save Changes] on the Permalinks page in the Admin Settings.

@westonruter
Copy link
Member

@mkormendy sorry, yes, autocomplete problem.

Please share the URL to your site.

@mkormendy
Copy link

mkormendy commented Jul 6, 2019

Because I've had this issue for some time, I have created a cron job that runs the wp rewrite flush CLI command on a regular basis so you won't be able to see the issue. I'd have to turn it off. Let me set up an environment to test with and get back to you.

@westonruter
Copy link
Member

I'm confused, though. Why would adding ?amp show a 404? Adding a query param should have no impact on whether or not WordPress is able to successfully perform a query.

@mkormendy
Copy link

mkormendy commented Jul 6, 2019

I lied again. So apparently ?amp does kick to the AMP page after all. Only /amp/ 404's out.

You can test right now for the next 3 minutes (after which point the cron job will flush rewrites via CLI and fix /amp/ rewrites):

https://www.smartstartinc.com/blog/easiest-ignition-interlock-device/?amp

https://www.smartstartinc.com/blog/easiest-ignition-interlock-device/amp/

@westonruter
Copy link
Member

Ok, so for an immediate workaround I suggest adding this code to force the query param as opposed to the endpoint:

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

@mkormendy
Copy link

mkormendy commented Jul 6, 2019

Here's a before and after.

before-flush-rewrites

after-flush-rewrites

@mkormendy
Copy link

I'll try the force, I may have to sit with our SEO Strategist to make a mark in her analytics if she gets changes in her expected attribution paths/urls.

@westonruter
Copy link
Member

What matters is the canonical URL. Whether using the query param or the endpoint for the paired AMP URL, the canonical URL stays the same.

Now, as for the underlying cause for why flushing rewrite rules doesn't work... I'm guessing the cause is a plugin calling flush_rewrite_rules() too early, before AMP has had a chance to register it's rewrite endpoint.

@mkormendy
Copy link

For sure on the canonical, I do know that she has some specific tags/triggers set up for attributing our efforts in adding AMP to the site in the first place. As for the flush_rewrite_rules() firing too early, the process that triggers that and breaks it has to do with WP Migrate Pro .. which I use for automating the migration of specific tables, forms and content from our staging environment to live and vice versa.

@westonruter
Copy link
Member

I'm going to close this in favor of #2204, where we should move to using ?amp query param instead of using the /amp/ endpoint. We already only use ?amp for Transitional mode, and ?amp is used in Reader mode when viewing a page. So using ?amp everywhere will be more consistent and there won't be headaches with rewrite rules.

@mkormendy
Copy link

Thanks @westonruter!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants