Make Url format optional to use query string/prefix/suffix for AMP pages

The patch works well. I think it's an option the module should offer.
I only had to make a small change in hook_uninstall replacing:

variable_del('amp_url_prefix');

With:

variable_del('amp_path_prefix');

I've added and removed some features from #4 in the interest of simplicity and utility.

I've added the option of using the amp url suffix (www.example.com/article/amp), since this seems to be a popular choice, alongside the amp prefix (www.example.com/amp/article) and amp subdomain (amp.example.com/article). For example:

www.newyorker.com/news/john-cassidy/donald-trumps-russian-marginalia-pro...
www.gq.com/story/russia-donald-trump-cnn-report/amp
nypost.com/2017/01/23/how-trump-is-remaking-the-culture-war/amp/

I've also removed the ability to choose the text of the url prefix, limiting the user to just the "www.example.com/amp/article" option. This seems simpler since the user doesn't have an option to choose the text of the query string (www.example.com/article?amp).

I've removed the added helper functions in the interest of simplicity.

I streamlined the logic is amp_is_amp_request() to more closely match the original.

And to convert from amp to non-amp paths for the prefix and suffix options, I've changed the simply cut out the prefix or suffix using substr(), instead of exploding the path on "/".

We still need to change the link rel="amphtml" tag for the new url formats. I will update #5 with this next.

Regarding the comments in #7:

If we're going to allow the user to change amp prefix to arbitrary string, we should allow same for suffix and query string. I don't think it's needed at this point.

1. I added back in the tests, adding a test for suffix case and adjusting the other cases.

2. I added back in the hook_uninstall.

This new patch works with either url alias or node/nid.

For the Context module, for the new prefix and suffix modes, it does not appear to be possible to add a path rule for targeting amp pages. However, this appears to be the case for patch #4 as well (make_url_format_optional-2862218-4.patch).

Other:

I fixed so the link rel="amphtml" tag is correct for the new url formats.

I also fixed the way we set the system path in hook_url_inbound_alter().

One little point found so far on this patch, seems like it will only alter inbound URLs if found in the alias tables by drupal_lookup_path()

+++ b/amp.module
@@ -327,6 +327,35 @@ function amp_entity_info_alter(&$entity_info) {
+  else {
+    $path = $non_amp_path;
+  }

A couple of little suggestions:

1. As per #11 previous patch only detects aliases, added backups to make internal system paths recognised.
2. Added checks to ignore urls that are already valid routes.

Ideally all 'amp' paths would be registered as system routes to avoid collisions and avoid other modules defining those paths but not necessary now.

[removed]

[removed]

Please ignore patches 16 and 17. I had not cloned the latest 7.x-1.x dev branch.

Patch 15 works for me, and passes all current tests.

Moved the formatting of output urls to hook_outbound_url_alter to centralising the processing of amp url formats.

This allows redirection on submit to be handled with very little change.

I ran the tests on patch 19, and it results in 4 failures. However when I check the failures manually, the behavior appears to be correct.

The 'failures' relate to finding the expected '

I've created a new patch with the changes in #14

The patch #19 did not need any changes, just re-rolling. Tests needed to be updated.

228 passes, 0 fails, 0 exceptions, and 67 debug messages in 2 min 15 sec.

However this is with amptheme installed, which the CI bot will not do because as far as I can tell there is no way to make the amptheme a dependency for the module.

That's why I'm attaching a second patch excluding the test steps that fail when the amptheme is not enabled.

All these fails are because the amptheme is not installed, we need to find a way to force the amptheme to be installed.

Just wanted to note some history on this one.

There were a fair number of people in the Drupal community who strongly suggested using query strings rather than path aliases. Generating all of the additional aliases was particularly problematic on sites with a lot of nodes, if I remember correctly. Something like that.

Anyhow, here is a link to a past issue where this was discussed and some of the changes were made to move to query string switching. https://www.drupal.org/node/2676922

Giving people the option to use a path may work, it just may also have some drawbacks.

Are there any additional steps needed to get these redirects to work?

If I select prefix (or same behavior with suffix), example.com/amp/my-url seems to just be redirecting me to example.com/my-url

I am looking into this option because I am encountering some odd behaviors with caching. It seems I can consistently trigger an issue with caching on my dev site when I do the following:

1. in mobile, view example.com/my-url?amp - good
2. in mobile, view example.com/my-url - good
3. in mobile view, view #1 again - still good
4. clear drupal cache
5. view #1 again - mistakenly renders as non-amp view (#2)

So, as a work-around, I was looking at this patch to see if it might resolve my caching issue, but I can't get the URL to not redirect to the non-amp version

Has anyone had any luck with this or have any advice?

thanks!

Is it possible to make this part of the module? It's really important to have what we call "clean URLs." I notice that on Google top stories those sites rank more often that have clean urls, like /amp instead of ?amp

Thank you.

Does anyone have any solutions to #37?

I am having the same issues.

Thank you for help.

Thank you Marcelo,

I tried it, but I am geting this.

mysite.com/url looks good
mysite.com/url/amp (or other other aliased version) redirects to mysite.com/url

What to do about this? What is causing the URL? I am using https if that matters.

Thank you

Hello everyone. If the patch doesn't work that means you are using Global Redirect, which redirects it to the original URL. Thank you @marcelovani.

Now the question is, is there a way to make the patch work with Global Redirect? Global Redirect is an important module and running in many sites making vital function. Does anyone have a solution on how to make the patch ignore global redirect?

Thank you.

@erku (& @marcelovani) - Thanks for tracking that down to the Global Redirect module!

It looks like one point of Global Redirect is to avoid having 2 URLs with the same content. However, with AMP, we actually do want this - mostly the same content available via 2 unique URLs; one is just specific to AMP.

In response to @erku's question about how to get around this issue with Global Redirect, I haven't had time to work on this or anything, but looking at the Global Redirect's hook_init function, it looks like it uses hook_url_outbound_alter twice; per the comments on its implementation of hook_url_outbound_alter:

  // Allow Drupal to rewrite the URL
  // Most commonly this will be used to invoke locale_language_url_rewrite_url().
  // Via the locale_url_outbound_alter() implementation of hook_url_outbound_alter().
  // NOTE: We use $request_path here as current_path() is $_GET['q'] which gets
  //       altered by core, whereas $request_path is untouched
  drupal_alter('url_outbound', $request_path, $options, $request_path);

I'm wondering if potentially we could look more into hook_url_outbound_alter - including looking at how locale (in core) handles it for an example?

We have encountered an issue with patch #35 whereby the we are calling amp_is_amp_request() too early in the bootstrap, and as a result after a cache clear, amptheme_html_head_alter() isn't invoked, which means some html head elements aren't being removed. amptheme_html_head_alter() is invoked on subsequent page requests, but it's possible (and likely) the page would have already been cached with the invalid head elements.

This problem only occurs if there are hooks invoked early in the bootstrap that lead to the theme being initialised. In our situation an entity load occured in redirect_url_inbound_alter(), which in turn caused a contrib module to call the l() function in its implementation of hook_filter_info().

We looked into refactoring the patch so that amp_url_inbound_alter() only checks the requested path to determine if an amp page is being requested, as this prevents any risk of entities being loaded too early, but we feel this could lead to other unforeseen issues, such as resolving paths it shouldn't.

After some consideration and discussion with our SEO team and @marcelovani, we decided to phase out the use of this patch and use the ?amp query string instead for the following reasons:
- If a node URL changes, the redirect module can resolve the ?amp path
- Altering the path amp_url_inbound_alter() carries some risk of unpredictable behaviour depending on which modules sites have enabled (as we are currently experiencing)

Hello, is there a way to work /amp URLs in Drupal 9? We are trying to migrate our website from D7 to D9, and would like to know how to make our AMP URLs ending with /amp rather than with a question mark. Can you please help how to do this in Drupal 9?

Thank you.