In #2688545: Remove ampNodeViewController in favor of amp_entity_view_alter(), we are removing the ampNodeViewController in favor of changing the AMP view mode in amp_entity_view_alter.

However, that means we can no longer set cache keys, as those cannot be changed in alter hooks.

In reviewing caching information at https://www.drupal.org/developing/api/8/cache/contexts, it appears what would be a better option would be to create a cache context service for this configuration setting, and add that context amp_entity_view_alter. We already have a cache context to vary based on whether or not the warnfix query parameter is present. So the service only needs to focus on varying based on something like so (except with dependency injection):

  // Get the Config Factory service.
  /** @var ConfigFactoryInterface $config_factory */
  $config_factory = \Drupal::service('config.factory');

  // First check the config if library warnings are on
  $amp_config = $config_factory->get('amp.settings');
  if ($amp_config->get('amp_library_warnings_display')) {
    return true;
  }

Comments

mdrummond created an issue. See original summary.

dsayswhat’s picture

Issue tags: +1.0 blocker
Berdir’s picture

Need to look more closely, but this seems like a non-issue to me.

cache contexts are cache keys. They bubble up and their value becomes part of the cache key/cid. What you have there right now is not necessary as far as I see.

What you should have is the cache tag for the config instead, then you have that invalidated when changing the config automatically.

mtift’s picture

Status: Active » Closed (works as designed)

Discussed this with @sidharth_k and @mdrummund as well, and we agree that we can close this

Wim Leers’s picture

Issue tags: +D8 cacheability

#3++