Remove ampNodeViewController in favor of amp_entity_view_alter()

Does that seem like a limitation that we cannot alter caching keys in an alter hook? *say Wim Leers three times while spinning around*

It seems like this issue should be blocked on #2688585: Create cache context service for AMP library warnings config. If we can't figure out that one, it makes me wonder if this should go in.

1. +++ b/amp.module
   @@ -34,17 +37,46 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
   +
   +    if ($build['#view_mode'] == 'amp') {
   +      // Otherwise adding 'amp' query parameter in URL will have no effect.
   +      $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], ['url.query_args:amp']);
   +      // Otherwise adding 'warnfix' query parameter in URL will have no effect.
   +      $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], ['url.query_args:warnfix']);
   +      // Otherwise changing 'amp_library_warnings_display' will have no effect.
   +      $build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], ['config:amp.settings.amp_library_warnings_display']);
   +    }
   

   You should move at least the amp context out of this check. Then it applies all the time and should solve your problems.

2. +++ b/src/Element/AmpAnalytics.php
   @@ -36,6 +36,9 @@ class AmpAnalytics extends RenderElement {
          ),
          '#theme' => 'amp_analytics',
   +      '#cache' => [
   +        'contexts' => ['url.query_args:amp']
   +      ]
   

   The used theme is a default cache context, so not sure what this would do additionally?

3. +++ b/src/Element/AmpAnalytics.php
   @@ -36,6 +36,9 @@ class AmpAnalytics extends RenderElement {
   diff --git a/src/Element/AmpIframe.php b/src/Element/AmpIframe.php
   
   diff --git a/src/Element/AmpIframe.php b/src/Element/AmpIframe.php
   index 8838d66..3aa704f 100644
   
   index 8838d66..3aa704f 100644
   --- a/src/Element/AmpIframe.php
   
   --- a/src/Element/AmpIframe.php
   +++ b/src/Element/AmpIframe.php
   
   +++ b/src/Element/AmpIframe.php
   +++ b/src/Element/AmpIframe.php
   @@ -43,6 +43,10 @@ class AmpIframe extends ProcessedText {
   
   @@ -43,6 +43,10 @@ class AmpIframe extends ProcessedText {
            array($class, 'preRenderAmpIframe'),
          ),
          '#theme' => 'amp_iframe',
   +      '#cache' => [
   +        'contexts' => ['url.query_args:amp', 'url.query_args:warnfix'],
   
   +++ b/src/Element/AmpProcessedText.php
   @@ -33,10 +35,8 @@ class AmpProcessedText extends ProcessedText {
   +        'tags' => ['amp.settings.amp_library_warnings_display']
   

   I assume you mean the config? Should be config:amp.settings.

Closed #2725793: AMP node page route is wrong as a duplicate.

Fixed points 1. and 3. mentioned in comment #10 and removed unused import files.

+++ b/amp.module
@@ -64,19 +58,18 @@
     if ($build['#view_mode'] == 'amp') {
-      // Otherwise adding 'amp' query parameter in URL will have no effect.
-      $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], ['url.query_args:amp']);
       // Otherwise adding 'warnfix' query parameter in URL will have no effect.
       $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], ['url.query_args:warnfix']);
       // Otherwise changing 'amp_library_warnings_display' will have no effect.
-      $build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], ['config:amp.settings.amp_library_warnings_display']);
+      $build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], ['config:amp.settings']);
     }

Lets move both cache contexts out, then you can also put them in the same line/array. That will result in fewer cache redirect writes once amp is accessed and the cost of another query args context is pretty small.

Merged these cache contexts. Left that #10.2 there, will be discussed later.

+++ b/amp.module
@@ -34,14 +31,40 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
+    // Otherwise adding 'amp' and 'warnfix' query parameters in URL will have no effect.
+    $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], ['url.query_args:amp', 'url.query_args:warnfix']);
+    if ($build['#view_mode'] == 'amp') {
+      // Otherwise changing 'amp_library_warnings_display' will have no effect.
+      $build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], ['config:amp.settings']);
+    }

We weren't able to reproduce any caching problems here, likely because theme is an enforced cache context, so the different theme enforces that every cache is different for amp pages anyway.

If you do have a way to reproduce a problem, pleas provide steps to do so.

As mentioned above, I don't really understand how the added cache tags and contexts are related to this issue, but other than that, this looks good to me.

1. +++ b/amp.module
   @@ -59,10 +59,10 @@ function amp_entity_view_alter(array &$build, EntityInterface $entity, EntityVie
        // Otherwise adding 'amp' and 'warnfix' query parameters in URL will have no effect.
   -    $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], ['url.query_args:amp', 'url.query_args:warnfix']);
   +    $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], ['url.query_args:warnfix']);
        if ($build['#view_mode'] == 'amp') {
   

   Despite it being covered by the theme, I think it might still be useful to keep the amp cache context.

2. +++ b/amp.module
   @@ -59,10 +59,10 @@ function amp_entity_view_alter(array &$build, EntityInterface $entity, EntityVie
   -      $build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], ['config:amp.settings']);
   +      $build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], ['config:amp.settings.amp_library_warnings_display']);
   

   This is a cache *tag*, not a context.

   It doesn't vary, it invalidates. There is only one version, but when you save the config, it invalidates the relevant caches.

   Yes, it possibly invalidates too often, but your cache tag is made up. There are no per-setting cache tags, only per config. This doesn't work. You'd need your own custom cache tag invalidation for this and I think that's overkill.

   You don't change those settings on a daily basis. Once you're site is up, they're not going to change anymore.

As you said yourself, the warn thing seems to be something else. Maybe some part that actually adds those messages is not varied by the cache. I need to look into that more but I doubt it worked before and is broken by this conversion?

I tried to reach you a few times in IRC, please ping me so we can discuss this issue and the module in general.

The problem is simple enough. The query_args cache context doesn't support empty arguments, because it uses the value of it as the context. It varies by its value, not by its existence.

So the easiest way to make this works is to document to use &warnfix=1 or so and check on actually having a value. Otherwise you need your own cache context that checks on the existence of the query argument.

Also

* \Drupal\amp\Element\AmpProcessedText::getInfo() already specifies warnfix and that's where it is actually used. So there's no need to add it in the entity_view_alter()
* Also shows that the current amp cache context is equally meaningless, it just works because of the theme switch.

+++ b/src/Cache/AmpUrlAmpCacheContext.php
@@ -0,0 +1,65 @@
+    if (isset($show_amp)) {
+      return 'url.amp:true';
+    }
+    else {
+      return 'url.amp:false';
+    }

you don't have to include the name of the context, that's done automatically (the cid looks something like [url.amp]=thevalue).

Does that seem like a limitation that we cannot alter caching keys in an alter hook? *say Wim Leers three times while spinning around*

LOL :)

1. +++ b/amp.module
   @@ -34,14 +31,36 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
   +function amp_entity_view_mode_alter(&$view_mode, EntityInterface $entity, $context) {
   +
   +  // Get the AMP Context service.
   

   Nit: unnecessary new line, and pointless comment.

2. +++ b/amp.module
   @@ -34,14 +31,36 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
   +  if ($amp_context->isAmpRoute() && $view_mode == 'full') {
   

   Nit: use strict equality to avoid pain.

3. +++ b/amp.module
   @@ -34,14 +31,36 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
   +  // Get the AMP Context service.
   

   Again pointless comment.

4. +++ b/amp.module
   @@ -34,14 +31,36 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
   +  // Check if entity is a node with an enabled amp view mode.
   

   Nit: s/amp/AMP/

5. +++ b/amp.module
   @@ -34,14 +31,36 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
   +    $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], ['url.amp']);
   

   Just do

   $build['#cache']['contexts'][] = 'url.amp';
   

   Deduplication is handled for you.

6. +++ b/src/Cache/AmpUrlAmpCacheContext.php
   @@ -0,0 +1,65 @@
   +class AmpUrlAmpCacheContext extends ContainerAware implements CacheContextInterface {
   
   +++ b/src/Cache/AmpUrlWarnfixCacheContext.php
   @@ -0,0 +1,65 @@
   +class AmpUrlWarnfixCacheContext extends ContainerAware implements CacheContextInterface {
   

   Let's not do either of these, and wait for #2729439: QueryArgsCacheContext should return a special value for ?arg (without value) to land. It can land very soon. You can then make the AMP module require Drupal 8.1.2, and then you don't need to maintain any of this! :)

7. +++ b/src/EntityTypeInfo.php
   @@ -54,20 +52,19 @@ class EntityTypeInfo {
   +      $configure = t('/admin/structure/types/manage/:bundle/display/amp?destination=/admin/config/content/amp', array(':bundle' => $bundle));
   +      $enable_disable = t('/admin/structure/types/manage/:bundle/display?destination=/admin/config/content/amp', array(':bundle' => $bundle));
   

   This looks … very very very very wrong.

8. +++ b/src/EntityTypeInfo.php
   @@ -54,20 +52,19 @@ class EntityTypeInfo {
   +        $node_status_list[] = t(':label is <em>disabled</em>: <a href=":enable_disable">Enable AMP in Custom Display Settings</a>', array(
   

   This should use $this->t(), not t(). Do use StringTranslationTrait; and you're set.

Nice work, @mdrummund. This looks good to me. Code makes sense. My local still works correctly on routes such as ?amp as well as ?amp&warnfix. +1 from me.

I agree with Wim that we should just let #272943: buddylist2 5.x-1.0-beta1 the bug with empty query args. Should be possible to get it into 8.1.2. That's quite some code that you can avoid then.

1. +++ b/amp.module
   @@ -34,22 +30,38 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
   +  // Get a list of available view modes for the current entity.
   +  $view_modes = \Drupal::service('entity_display.repository')->getViewModeOptionsByBundle('node', $entity->bundle());
    
   -  // Git a list of available view modes for the current entity.
   -  $view_modes = \Drupal::entityManager()->getViewModeOptionsByBundle('node', $entity->bundle());
   -
   +  // Check if entity is a node with an enabled AMP view mode.
      if ($entity->getEntityType()->id() == 'node' && isset($view_modes['amp'])) {
   

   This code should really be wrapped inside the if() statement, so that getViewModeOptionsByBundle() is not called for *every rendered entity*. Might want to take a look at the updated version of this hook in D7, I believe it checks if entity type == node and $build['#view_mode'] == full before doing any more logic.

   Arguably, this could be done in a separate issue.

2. +++ b/amp.module
   @@ -34,22 +30,38 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
   +    // Catches any variations based on 'amp' query parameter in URL.
   +    $build['#cache']['contexts'][] = 'url.amp';
   

   The new cache context should only be added if the view mode is full or amp only. Not for other view modes.

3. +++ b/amp.module
   @@ -34,22 +30,38 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
   -        else if ($build['#view_mode'] == 'full') {
   +        else if ($build['#view_mode'] === 'full') {
   

   Coding standards should be "elseif"

4. +++ b/src/Cache/AmpUrlAmpCacheContext.php
   @@ -0,0 +1,60 @@
   +    $show_amp = $this->requestStack->getCurrentRequest()->get('amp');
   +    return isset($show_amp);
   

   Could be simplified just by doing return (bool) $this->requestStack->getCurrentRequest()->get('amp') instead of having to assign a local variable just to return.

5. +++ b/src/Cache/AmpUrlWarnfixCacheContext.php
   @@ -0,0 +1,60 @@
   +    $show_amp_library_warnings = $this->requestStack->getCurrentRequest()->get('warnfix');
   +    return isset($show_amp_library_warnings);
   

   Same here.

I agree with Wim that we should just let #272943: buddylist2 5.x-1.0-beta1 the bug with empty query args. Should be possible to get it into 8.1.2. That's quite some code that you can avoid then.

Glad to hear I'm not alone.

If and when the fix lands in 8.2, we can revisit this.

Then let's at least have a follow-up issue and a @todo pointing to it, so this is not forgotten?

+++ b/amp.module
@@ -45,39 +45,40 @@ function amp_entity_view_mode_alter(&$view_mode, EntityInterface $entity, $conte
+    // Double-check that the AMP view mode is enabled for this content type.
+    if (isset($view_modes['amp'])) {

How can one enable AMP for a content type? Is this in the entity bundle's configuration's third-party settings? Or is it in some AMP configuration?

If it's the latter, you'll need to add a cache tag.

If it's the former, then this is fine.

Oh, and mark these cache contexts as @deprecated, to prevent 3rd party code from using them, so they're safe to remove at any time.

+++ b/amp.module
@@ -34,38 +30,55 @@ function amp_help($route_name, RouteMatchInterface $route_match) {
+              $build['#attached']['html_head_link'][] = array(
+                array(
+                  'rel' => 'amphtml',
+                  'href' => $amp_href,
+                ),
+                TRUE,
+              );

And try to use the shortened array syntax according to the array standard coding whenever you provide a patch :)

Hmmm. Now I'm feeling a bit uncertain about this one. A patch that adds 4 @todo and 11 @deprecated makes me wonder if this is the correct approach.

It's back to RTBC. :)

Hmmm. Now I'm feeling a bit uncertain about this one. A patch that adds 4 @todo and 11 @deprecated makes me wonder if this is the correct approach.

Well, it's not like any of that is major re-architecting. It's just @todos for removing code! :) Those are the best @todos! The @deprecated is just there to prevent anybody from relying on it. Alternatively, label it @internal, it has the same desired effect.

OK, I discussed this with @mdrummond, and given Wim's comment above, I'm feeling good about this (again). I say +1

+1 for committing now.

Great job here!

Community++