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

AMP posts don't work with %category% in permalink structure #314

Closed
nicholasgriffintn opened this issue Mar 5, 2016 · 30 comments
Closed
Labels
Bug Something isn't working

Comments

@nicholasgriffintn
Copy link

Hey,

I posted a support request on the Wordpress.org page, haven't received a response so I figured I'd re-post it here.

It seems as though the plugin does not like permalinks other than the date format one, which is bad news for a lot of publishers like myself, who like to use category and name permalinks. You can read my full post here:

https://wordpress.org/support/topic/permalink-problems-28?replies=4

Figured it's better to post the link than just copy and paste.

@gabrielperezs
Copy link

Hello Nicholas. Maybe you can test my version https://github.com/gabrielperezs/amp-wp , in theory must work in your case.

@nicholasgriffintn
Copy link
Author

Hey,

Your plugin actually works perfectly, whatever you did, you should tell the Automatic guys about it.

An instant fix for me, only problem is that it thinks it's the original plugin, so it wants to be updated.

@dineshitchimes
Copy link

Hi,

I added your plugin in my website. But how I can validate it that it works or not. Earlier I was using the plugin https://wordpress.org/plugins/accelerated-mobile-pages/ but getting lots of error from my webmaster.

So added your plugin but not sure it's working or not.
Can you help me here.
Here is the URL https://www.moability.com where I have installed your plugin.

Thanks
Dinesh

@dineshitchimes
Copy link

I checked that it works with Posts but not pages.
Why it's not working with Pages

@gabrielperezs
Copy link

Hello @nicholasgriffintn Thanks! I sent the pull request to the original plugin, I hope they include my code in some way :)

@gabrielperezs
Copy link

Hey @dineshitchimes you still has my plugin version on your site? because I can't see the rel amphtml , and any /amp/ url works. Tell me if I can check it.

@dineshitchimes
Copy link

Hi,

Here you can see : https://www.moability.com/blog/the-trend-shifts-in-app-development-spectrum-2015/amp
This works only for post. It should be working with any pages as well. How it should be fixed. Please help us out.

Thanks & Regards
Dinesh

@gabrielperezs
Copy link

@dineshitchimes but your are using the amp-wp offical plugin. And this don't has support for pages, archive, tags,... I did a fork ( https://github.com/gabrielperezs/amp-wp ) to develop this part, if you want to try. Disable the official before install my fork.

@dineshitchimes
Copy link

Added your develop plugin
https://www.moability.com/amp
but it shows blog landing page. How to fix it that it should show the default homepage.

Thanks for your help.

@mjangda
Copy link
Contributor

mjangda commented Mar 15, 2016

This might be an issue with add_rewrite_endpoint not working with %category% in permalinks.

@mjangda mjangda added the Bug Something isn't working label Mar 15, 2016
@mjangda mjangda changed the title Permalink problems AMP posts don't work with %category% in permalink structure Mar 15, 2016
@dineshitchimes
Copy link

Hi @gabrielperezs

Also I am getting errors from google for AMP.
Here is the screenshot http://www.awesomescreenshot.com/image/1095605/5017edbefe409eb9573bc471d02c1dc0

How to fix it ? Can you help me out here ?

Thanks
Dinesh

@dineshitchimes
Copy link

Hi @gabrielperezs

Did you get a chance to look in ?

Thanks
Dinesh

@Asif2BD
Copy link

Asif2BD commented Mar 28, 2016

@gabrielperezs why not you submit your code to official repo? Just submit a pull request, because your updates are very essential, and logical improvement for amp-wp. Otherwise when Automattic will push their updates with more feature anybody using your version will be in weird situation.

@gabrielperezs
Copy link

@Asif2BD Yes, you are right. And I did #313

@Asif2BD
Copy link

Asif2BD commented Mar 30, 2016

@gabrielperezs Thank you so much, but it seems to have conflict and never got accepted. Do you think the way you enabled AMP for all pages is possible to do via functions.php or making an addon type small plugin? Otherwise can't really deploy in real site, as when Automattic push update, will be in weird situation.

@gabrielperezs
Copy link

@Asif2BD when I send the pull request was without conflict. I think the authors don't want to go in this way. I don't know :)

I'm working on this option, but the AMP original plugin has no hooks to do this changes from functions.php, at the moment..

@Asif2BD
Copy link

Asif2BD commented Mar 30, 2016

Let me know if you could do that. I was working to change the amp url pattern, and cooking something interesting. If you could have find a way, I could make good use of it. Thanks in advance.

@faquick
Copy link

faquick commented Apr 6, 2016

I simply did not find your bug report before, and I posted a similar support request here:

https://wordpress.org/support/topic/amp-permalink-not-working?replies=6

What should I do to solve this problem? Just install @gabrielperezs version for the time being, and then hope that his fix will be implemented in the official plugin?

@faquick
Copy link

faquick commented Apr 6, 2016

Btw, @gabrielperezs can't you just submit a pull request with only the part of your code that enables more custom permalinks to work? Maybe the authors do not want to (or simply cannot) implement all the features in your fork

@Asif2BD
Copy link

Asif2BD commented Apr 6, 2016

If I am not wrong @gabrielperezs's fix what mainly does it enable amp for all type of page, not limited to singular type. So, it will not solve permalink issue you have. It does not to anything new for single post.

@faquick
Copy link

faquick commented Apr 6, 2016

In fact, I installed it and it does not work. I thought that the original issue in this thread was the same I had :(

@faquick
Copy link

faquick commented Apr 6, 2016

Anyway, I am completely at loss here to understand why they just started working again, a couple minutes after deleting this fork and re-activating the original AMP plugin.

@Asif2BD
Copy link

Asif2BD commented Apr 6, 2016

It should not, if you look at his code, all it does it extends plugin capacity from singular to other page type. You could look at this issue I create #353 But currently 'amp_get_permalink' is limited and wont let you edit it with regular hooks, developer proposed something like 'amp_pre_get_permalink', but still not available. I am personally trying to create something to solve my issue, and looking forward to release as free plugin.

@nicholasgriffintn
Copy link
Author

nicholasgriffintn commented Apr 6, 2016

It used to work, it does not any longer after some changes to the amp coding I think, the issue does still exist in the original plugin also. It would be nice if this issue could be fixed in the original one, as we are unable to use AMP currently, and that's rather annoying.

@madocar
Copy link

madocar commented Apr 27, 2017

Hi @gabrielperezs,
I was hesitating to write you but then it turned out I have to ... Im lost with amp.
The thing is I have a lot of specific pages (taxomies) with no AMP functionality.
Probably I asked everyone why it does not work but no one helped me so far.
Look at these:

http://www.foodfest.sk/jedla/obed/amp/
http://www.foodfest.sk/zdrave-recepty/amp/
http://www.foodfest.sk/ako-na-to/amp/

Can you help me please how to fix it and enable amp?
I already tried disable oficial plugin and enable yours but it did not help.
Thanks a lot

@madocar
Copy link

madocar commented May 15, 2017

Hi @gabrielperezs ,

​I would like to ask you there is any possibility to help me.​
The thing is I have a lot of specific pages (taxomies) with no AMP functionality.

​Look here:
http://www.foodfest.sk/jedla/obed/amp/
http://www.foodfest.sk/ingrediencie/vajcia/amp/

These pages are redirected to URL with no /amp/ at the end.​
I added this code to function.php but still no progress.

function isa_amp_add_cpt() {

add_post_type_support( recipe', AMP_QUERY_VAR );

It looks like It should be working but It does not (post_type=recipe, taxonomy=ingredient)

​/wp-admin/term.php?taxonomy=ingredient&tag_ID=278&post_type=recipe&wp_http_referer=%2Fwp-admin%2Fedit-tags.php%3Ftaxonomy%3Dingredient%26post_type%3Drecipe​

Is there any way to help me enable AMP.

Thanks for any help.

Martin

@native-apps
Copy link

Oh what a bummer. Still an issue today? I just stumbled on this and noticed the permalinks break when using %category% in the custom permalinks field. Has anyone figure a work around yet? Because there is no way we will ever change our permalink structure and destroy healthy SEO.

@4eAgency
Copy link

I've same problem, amp doesnt work for single article if /%category%/%postname%/ is set in permalink structure. Im waiting for response.

@MattGeri
Copy link

MattGeri commented Nov 20, 2017

This issue shows itself when selecting and one of the custom permalink structures on the Permalinks settings page in WordPress. My issue was when using the /%year%/%monthnum%/%day%/%postname%/ structure. The problem is that the attachments rewrite rule gets matched before the AMP rule.

Until this get's fixed, you can use the following code to get your endpoints to work (this is for the /%year%/%monthnum%/%day%/%postname%/ structure, but can be adapted for other structures):

<?php
add_action( 'init', 'mg_reorder_rewrite' );

/**
 * Re-order AMP rewrite rule.
 *
 * This fixes a bug in the Automattic AMP plugin when using custom permalink
 * structures, WordPress sees AMP endpoints as attachments.
 */
public function reorder_rewrite() {
	add_rewrite_rule('([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]', 'top');
}

Basically the above code is just adding the rewrite rule earlier than the attachment rule. Remember to flush rewrite rules (just visit the permalink page in the admin) for this to work.

@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.

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