Is there a need for AMP pages to have a nice alias? Possible to switch to query string?

This is also what @dawehner said at #2599164-3: Distinguishing an AMP request from a non-AMP request.

I think this might be a good idea and I'm going to give it a try. I do think it would be better to just have content/nice-alias?_format=amp instead. I'd like to try and get this work and then compare performance with /amp page.

As a little bit of background, we initially wanted to validate the current approach so that we can use it in the D7 version and not have to come up with something different for each version of the module. That being said, if this is better for the D8 version, great! I was under the impression we couldn't do this in D7 though, is that assumption correct? If so does it burden maintainers to have two different approaches?

Turns out I won't have time to work on this today, so unassigning for now in case anyone is feeling adventurous.

The question to ask first is: If there were no system constraints would we go with ?amp=1 or /amp and I think most of us may agree that /amp is better.

The reason is aesthetic but also may have some technical soundness. I have always been given to understand that pages with a query parameter are not *cached* in downstream proxies like varnish and they simply pass the page through to Drupal. In fact on one of our projects in the past we always added a ? if we wanted to skip Akamai and see what Drupal was showing IIRC. (Doing this is quite common and referred to as cache busting as you might know).

Additionally the ? indicates to me that the Drupal needs to regenerate the page and not serve it out of its cache . Of course, this has all changed in Drupal 8. Pages with query parameter can be cached in Drupal 8 (just like a pages without the query parameter). This was different in Drupal 7 I think.

So the real question is -- will downstream proxies (akamai, varnish) be OK with this or will they need special configuration to ensure that ?amp=1 is also cached properly by default.

Proxies are required to honor caching-specific http headers, not just the URL.

I agree with Wim that the D8 version should use querystring, for consistency, code maintenance, etc. Thanks to Dave for showing that similar approach on D7 is easily achieved.

I have always been given to understand that pages with a query parameter are not *cached* in downstream proxies like varnish and they simply pass the page through to Drupal.

Ages ago, and today only on the poorest configured setups.

Query strings don't prevent caching. Not in Drupal (7 nor 8), not in Varnish, not anywhere.

I think perhaps you're confusing query strings that change which is often used for cache-busting, e.g. to ensure an updated CSS aggregate is downloaded again by a browser.

I agree that type negotiation has been handled by core and it would be best to use it. The url may not be as pretty but 1.) that shouldn't matter in this case and 2.) it is a separate issue that could still be addressed in future versions.

There is a middleware for type negotiation.

I tried to give this a shot, but ran into issues.

Specifically, this resulted in a WSOD with the following error message:

The website encountered an unexpected error. Please try again later.

Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException: No route found for the specified format html. in Drupal\Core\Routing\RequestFormatRouteFilter->filter() (line 54 of core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php).

For quick reference, here's the relevant code:

    $format = $request->getRequestFormat('html');
    /** @var \Symfony\Component\Routing\Route $route */
    foreach ($collection as $name => $route) {
      // If the route has no _format specification, we move it to the end. If it
      // does, then no match means the route is removed entirely.
      if ($supported_formats = array_filter(explode('|', $route->getRequirement('_format')))) {
        if (!in_array($format, $supported_formats)) {
          $collection->remove($name);
        }
      }
      else {
        $collection->add($name, $route);
      }
    }

From what I can tell, this checks if a route has a format that is something besides html. If so, discard that from the collection.

What makes me hesitant is not knowing how deep this goes. The _format key for a route appears to be pretty deeply embedded in the routing and rendering system, used to do things like switching over to the JsonRenderer instead of the HtmlRenderer. Feels like going down this road could lead to a lot of overkill and complication, rather than simplifying things.

When I look through the structure of routes (https://www.drupal.org/node/2092643), I don't see a way to easily check an arbitrary key/value pair in a query string, so we could do something like ?amp=true or something else along those lines.

I'm definitely open to some sort of query string switch, but trying to tell Drupal to use a format that is not html, when this is html just with some restrictions, seems like a problem.

I guess we could just keep this out of the route definition and check for an arbitrary get value within amp context.

Okay, this works. With a big caveat.

We can put ?amp into the query string and show the amp view mode/switch to the amp theme, etc., based on that.

However, the problem is that now there is nothing on the route itself about amp. That means every single node will go through the amp controller rather than the standard node controller. That seems problematic.

It seems strange there is no way to add an arbitrary query string parameter to a route definition. I can't imagine this is the only situation where that might be useful.

This bit in the Symfony route documentation seems highly relevant: http://symfony.com/doc/current/book/routing.html#book-routing-format-param

The Special _format Routing Parameter

This example also highlights the special _format routing parameter. When using this parameter, the matched value becomes the "request format" of the Request object.

Ultimately, the request format is used for such things as setting the Content-Type of the response (e.g. a json request format translates into a Content-Type of application/json). It can also be used in the controller to render a different template for each value of _format. The _format parameter is a very powerful way to render the same content in different formats.

In Symfony versions previous to 3.0, it is possible to override the request format by adding a query parameter named _format (for example: /foo/bar?_format=json). Relying on this behavior not only is considered a bad practice but it will complicate the upgrade of your applications to Symfony 3.

As far as I can tell from several searches, query string values cannot be used in route definitions. So without '/amp' in the URL, the route definition is essentially replacing the default node controller with the amp controller. That seems like a big deal.

I'm feeling a lot better about this patch. This switches the ampPage controller to ampNodeController, which extends off NodeController. That way even if we're passing all nodes through ampNodeController, we can at least send the processing back through NodeController if we're not on an amp route. I think this seems like a reasonable compromise.

Not having parameter queries in route definitions seems like a gap to me after working on this, as it means query strings cannot be used to distinguish between different controllers.

Looking good. Here are a couple initial reactions.

1. +++ b/amp.module
   @@ -329,75 +329,41 @@ function amp_node_form_submit_with_warn(&$form, FormStateInterface $form_state)
   +<<<<<<< HEAD
   

   This should be removed

2. +++ b/amp.module
   @@ -329,75 +329,41 @@ function amp_node_form_submit_with_warn(&$form, FormStateInterface $form_state)
   +* Implements hook_form_BASE_FORM_ID_alter().
   +*/
   +function amp_form_node_type_edit_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   +  $enabled = node_type_get_names();
   +  $node_type = $form['type']['#default_value'];
   +
   +  $form['workflow']['amp'] = array(
   +    '#type' => 'select',
   +    '#title' => t('Enable AMP pages for this content type'),
   +    '#options' => array(
   +      0 => t('No'),
   +      1 => t('Yes'),
   +    ),
   +    '#default_value' => !empty($enabled[$node_type]) ? 1 : 0,
   +  );
   

   Did you mean to add this back in?

Improved logic for setting the query string.

#16
- 1. Already fixed in #17.
- 2. No. Probably good to check the patch closely in case I missed something else like this when fixing merge conflicts. This patch fixes this miss.

+++ b/amp.module
@@ -329,75 +342,25 @@ function amp_node_form_submit_with_warn(&$form, FormStateInterface $form_state)
+function amp_node_settings_submit(&$form, FormStateInterface $form_state) {
+  $amp_enabled = $form_state->getValue('amp');
+  $content_type = $form['type']['#default_value'];
+  $config = \Drupal::service('config.factory')->getEditable('amp.settings');
+  // Like on the configuration screen, use the content type for the value
+  // if it is set to true.
+  $config->set('node_types.' . $content_type, !empty($amp_enabled) ? $content_type : 0);
+  $config->save();
 }

This also should be removed. We are no longer using node_types in amp.settings

Thanks, mtift. New patch with that fix.

This is exciting, because it looks like it's working! The rendered AMP pages are displaying slightly different information with the new route, and I'm not sure why, but I'm sure that can be changed (if necessary). For example, the existing code does not show the node's view/edit/etc links:

But they appear with the patch applied:

This actually seems correct because when I'm not logged in (with the patch applied) they do not appear. So maybe this is an improvement.

Otherwise, this looks great to me. Simple, and so nice to remove all of that alias code. I like it!

I guess we should also figure out if this can be applied to the D7 site.

Turns out we needed to be extending the NodeViewController instead of the NodeController. Because we weren't, the route wasn't applying, and thus the view mode wasn't getting switched.

For what it's worth, the default node route is generated through the NodeRouteProvider class: https://api.drupal.org/api/drupal/core!modules!node!src!Entity!NodeRoute...

From what I can tell, the view mode is properly switching now.

Discussed with mtift, and we're going to move foward with this query string issue. If there are any issues that crop up, we'll take care of them in follow-up issues.

Hello I am trying to unsuccessfully enable url clean aliases for AMP pages. has anyone succeed with Drupal 7? I mean clean urls for AMP pages. There is a patch in another discussion here https://www.drupal.org/node/2862218 but it's not working for the reason described in #37.

Thank you for any help.