Canonical and AMPHTML must be absolute

Yes, if you have a fix then I think we should use that until core has the same functionality. Thanks!

Just a note that in the latest patch on #2717641: Add support for 'Top Stories with AMP' I'm also changing the canonical URL to be absolute, as I believe that's needed for the JSON schema output.

Slight differences between the two, glad to make modifications as necessary.

This looks great. Code makes sense. Tests fail, as expected. And now the URLs look like this:

Committed to 8.x-1.x. I opened a follow-up for the test: #2741217: Test for absolute URLs

I'm not sure if there's an easier way to alter canonical links, but I think we can do the Drupal 7 version like this.

+++ b/amp.module
@@ -1192,3 +1193,24 @@ function amp_page_delivery_callback_alter(&$deliver_callback) {
+          $relative_url = $tag['#attributes']['href'];
+          // Double check that we are converting relative URLs to absolute URLs.
+          if ($relative_url == drupal_get_normal_path($relative_url)) {
+            $absolute_url = url($relative_url, array('absolute' => TRUE));
+          }
+          $tag['#attributes']['href'] = $absolute_url;

In theory if this URL somehow is already absolute, then $absolute_url might not exist and could cause an error.

Could be worth either using just one variable ($canonical_url or something like that) or moving to:

if ($relative_url == drupal_get_normal_path($relative_url)) {
  $absolute_url = url($relative_url, array('absolute' => TRUE));
  $tag['#attributes']['href'] = $absolute_url;
}
else {
  $tag['#attributes']['href'] = $relative_url;

}

I think using $canonical_url might be the better option, as otherwise it looks like you're setting the tag href to a relative URL, while with the test you would have determined the URL was actually an absolute URL.

Tested this, and it does successfully turn the amphtml link and canonical links on amp pages into absolute URLs.

drupal_get_normal_path() returns an internal path, so we're already checking for relative URLs. But I see how the variable names could be confusing. This should be more clear.

Tested, this still works. Good changes. RTBC.

Committed to 7.x-1.x