Determine whether a view is cacheable and its required contexts, store this i/t config entity

1. +++ b/core/modules/views/src/Entity/View.php
   @@ -298,6 +298,37 @@ public function calculateDependencies() {
   +      // Always include at least the language context.
   

   … because every rendered view contains some UI strings, which therefore depend on the language?
   (Perhaps that's the wrong reason?)

   In any case, I think we should document *why* we always include the language cache context.

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2213,6 +2214,45 @@ public function preExecute() {
   +    // Iterate over all handles. Note that at least the argument handler will
   

   s/handles/handlers/

3. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2213,6 +2214,45 @@ public function preExecute() {
   +    return [$cache_contexts, $cacheable];
   

   That's *so* much more readable :)

4. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1457,6 +1458,25 @@ protected static function arrayFilterZero($var) {
   +    if ($this->isExposed()) {
   +      $cache_contexts[] = 'cache.context.url';
   

   Perhaps include a comment here as well? ("An exposed filter allows the user to change a view's filters. They accept input from GET parameters, which are part of the URL. Hence a view with an exposed filter is cacheable per URL.")

5. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1457,6 +1458,25 @@ protected static function arrayFilterZero($var) {
   +    return !$this->isExposed();
   

   Why is it not cacheable at all? It's cacheable just fine, but only per URL, as per the above, right?

ROFL @ #16 :D I had to read that interdiff five times to spot the difference :D

1. +++ b/core/modules/field/src/Plugin/views/field/Field.php
   @@ -944,5 +945,23 @@ public function getDependencies() {
   +   * @return string[]
   

   inheritdoc

2. +++ b/core/modules/history/src/Plugin/views/filter/HistoryUserTimestamp.php
   @@ -99,4 +99,12 @@ public function adminSummary() {
   +    // This filter depends on the current time.
   

   … and is therefore never cacheable.

3. +++ b/core/modules/node/src/Plugin/views/filter/Access.php
   @@ -47,4 +47,16 @@ public function query() {
   +    // Node access is potentially cacheable per user.
   +    $contexts[] = 'cache.context.user';
   
   +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -361,4 +361,16 @@ public function adminSummary() {
   +    // The result potentially depends on term access and so is just cacheable
   +    // per user.
   +    $contexts[] = 'cache.context.user';
   

   Add a todo to update this once https://www.drupal.org/node/2287071 lands :)

4. +++ b/core/modules/node/src/Plugin/views/filter/Status.php
   @@ -30,4 +30,15 @@ public function query() {
   +    $contexts[] = 'cache.context.user';
   

   I was going to question whether this needs to be cacheable per user.

   But, sadly, due to the way this query is currently written, this is indeed necessary:

   $this->query->addWhereExpression($this->options['group'], "$table.status = 1 OR ($table.uid = ***CURRENT_USER*** AND ***CURRENT_USER*** <> 0 AND ***VIEW_OWN_UNPUBLISHED_NODES*** = 1) OR ***BYPASS_NODE_ACCESS*** = 1");
   

   Anyway, that's fine for now — this issue is NOT about improving cacheability of various Views plugins, it's about making that cacheability information for the current implementation available at all.

5. +++ b/core/modules/views/src/Entity/View.php
   @@ -298,6 +298,41 @@ public function calculateDependencies() {
   +   * Fill in all the cache information (cache contexts as cacheablity).
   ...
   +  protected function fillCacheInformation() {
   

   I'd call this "cacheability metadata".

6. +++ b/core/modules/views/src/Plugin/CacheablePluginInterface.php
   @@ -0,0 +1,22 @@
   + * Contains \Drupal\views\Plugin\CacheablePluginInterface.
   

   The docs for this interface are still POC-like. To flesh this out, you might want to look at CacheableInterface for inspiration.

   Which reminds me: why do we want a separate interface for this? We could just reuse the CacheableInterface. It's intended to be generic.

   Views' plugin base classes could provide defaults for getCacheTags() (the empty array), getCacheBin() ('default') and getCacheMaxAge() (Cache::PERMANENT).

   "cache contexts" are just "special cache keys": they're string placeholders that will be replaced with their value for the current request content, at which point they will be just cache keys like any other, hence switching to this interface would mean using getCacheKeys() rather than getCacheContexts().

7. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
   @@ -1163,6 +1173,53 @@ protected function unpackArgumentValue($force_int = FALSE) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function isCacheable() {
   +    $result = TRUE;
   +
   +    if (($plugin = $this->getPlugin('argument_default')) && $plugin instanceof CacheablePluginInterface) {
   +      $result &= $plugin->isCacheable();
   +    }
   +
   +    if (($plugin = $this->getPlugin('argument_validator')) && $plugin instanceof CacheablePluginInterface) {
   +      $result &= $plugin->isCacheable();
   +    }
   +
   +    if (($plugin = $this->getPlugin('style')) && $plugin instanceof CacheablePluginInterface) {
   +      $result &= $plugin->isCacheable();
   +    }
   +
   +    return $result;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCacheContexts() {
   +    $contexts = [];
   +    // By definition arguments depends on the URL.
   +    // @todo Once contexts are properly injected into block views we could pull
   +    //   the information from there.
   +    $contexts[] = 'cache.context.url';
   +
   +    if (($plugin = $this->getPlugin('argument_default')) && $plugin instanceof CacheablePluginInterface) {
   +      $contexts = array_merge($plugin->getCacheContexts(), $contexts);
   +    }
   +
   +    if (($plugin = $this->getPlugin('argument_validator')) && $plugin instanceof CacheablePluginInterface) {
   +      $contexts = array_merge($plugin->getCacheContexts(), $contexts);
   +    }
   +
   +    if (($plugin = $this->getPlugin('style')) && $plugin instanceof CacheablePluginInterface) {
   +      $contexts = array_merge($plugin->getCacheContexts(), $contexts);
   +    }
   +
   +    return $contexts;
   +  }
   

   Some docs to explain what's going on here would be helpful I think.

+++ b/core/modules/node/src/Plugin/views/filter/Status.php
@@ -30,4 +30,15 @@ public function query() {
+    $contexts = parent::getCacheContexts();
+
+    $contexts[] = 'cache.context.user';

+++ b/core/modules/taxonomy/src/Plugin/views/argument_default/Tid.php
@@ -167,4 +168,20 @@ public function getArgument() {
+    $contexts = ['cache.context.url'];

Why not have a parent class here, just to have it?

Otherwise this looks pretty good.

Making Views caching work optimally is Major priority.

So generally I think this is looking pretty good! I think this should go in first, before other cache tag improvements too.

1. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
   @@ -1013,14 +1014,23 @@ public function getPlugin($type = 'argument_default', $name = NULL) {
   +        if (!isset($this->options['default_argument_type'])) {
   ...
   +        if (!isset($this->options['validate']['type'])) {
   ...
   +        if (!isset($this->options['summary']['format'])) {
   

   Yeah, we could really do with fixing our data in some places :/

2. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
   @@ -1163,6 +1173,55 @@ protected function unpackArgumentValue($force_int = FALSE) {
   +    if (($plugin = $this->getPlugin('style')) && $plugin instanceof CacheablePluginInterface) {
   +      $result &= $plugin->isCacheable();
   +    }
   ...
   +    if (($plugin = $this->getPlugin('style')) && $plugin instanceof CacheablePluginInterface) {
   +      $contexts = array_merge($plugin->getCacheContexts(), $contexts);
   +    }
   

   Can you add comments here that style is needed as they are used for summaries please?

3. +++ b/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php
   @@ -83,4 +84,20 @@ public function getArgument() {
   +    $contexts = ['cache.context.url'];
   
   +++ b/core/modules/views/src/Plugin/views/argument_default/Raw.php
   @@ -102,4 +103,20 @@ public function getArgument() {
   +    $contexts = ['cache.context.url'];
   

   Let's just return the array direct here.

4. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2213,6 +2214,45 @@ public function preExecute() {
   +   *     - first value: (boolean) Is the display actually cacheable.
   +   *     - second value: An array of cache contexts the view output varies by.
   

   It is actually returning these the other way round.

5. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1459,6 +1460,28 @@ protected static function arrayFilterZero($var) {
   +   * @return string[]
   

   @inheritdoc

6. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1459,6 +1460,28 @@ protected static function arrayFilterZero($var) {
   +    if ($this->isExposed()) {
   +      $cache_contexts[] = 'cache.context.url';
   ...
   +  public function isCacheable() {
   +    return !$this->isExposed();
   

   This is kind of confusing; we only add the context if it's exposed, but isCacheable() will always be false if it's exposed...

7. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1459,6 +1460,28 @@ protected static function arrayFilterZero($var) {
   +   * @return bool
   

   @inheritdoc

8. +++ b/core/modules/views/src/Plugin/views/sort/Random.php
   @@ -32,4 +33,20 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +    // @todo Is there a cache context which specificies that something is not
   +    //   cacheable at all?
   

   Can remove this I think?

Sorry for not reviewing this sooner :(

1. +++ b/core/modules/user/src/Plugin/views/argument_default/CurrentUser.php
   @@ -19,10 +20,26 @@
   +  public function getCacheContexts() {
   +    $contexts = ['cache.context.user'];
   +
   +    return $contexts;
   +  }
   
   +++ b/core/modules/user/src/Plugin/views/argument_default/User.php
   @@ -74,4 +75,20 @@ public function getArgument() {
   +  public function getCacheContexts() {
   +    $contexts = ['cache.context.url'];
   +
   +    return $contexts;
   +  }
   

   Just like #26.3 said: let's return these directly? Fixed.

2. +++ b/core/modules/views/src/Entity/View.php
   @@ -298,6 +298,41 @@ public function calculateDependencies() {
   +    // @todo Check whether isSyncing is needed.
   

   Who should we talk to to figure out an answer for this?

3. +++ b/core/modules/views/src/Entity/View.php
   @@ -298,6 +298,41 @@ public function calculateDependencies() {
   +   * Fill in all the cache metadata (cache contexts as cacheablity).
   ...
   +   * The cache information is stored per view and per display.
   ...
   +  protected function fillCacheMetadata() {
   ...
   +      list($display['cache_information']['contexts'], $display['cache_information']['cacheable']) = $executable->getDisplay()->getCacheMetadata();
   

   metadata vs information vs metadata vs information. I've cleaned this up to say "metadata" everywhere, but then I noticed the schema actually says cache_information. I think cache_metadata would be better, or even cacheability. So I've changed things around here a bit, I hope you like it.

4. Usually, isCacheable() came first, getCacheContexts() came second. But this was not consistent. I made it consistent.

Oh-so-silly!

#31.1: DisplayPluginBase already has calculateCacheability(). setCacheability() calls that. I thought we'd want to avoid two calculcateCacheability() methods, on different objects, with very different responsibilities. It's your call though! :)

#31.2/3: yep, I did this as part of #28.4.

It was about the inconsistency of "information" vs "metadata", mixed freely. #2340507: Make the new AccessResult API and implementation even better is also renaming protected function mergeCacheabilityMetadata() to public function inheritCacheability(). So "cacheability" is used there, and this is the same concept, so that's why I went with "cacheability" here also.

What about fillInCacheability()?

0 failures, 14,457 exceptions… that must be some kind of record! :D

Done.

Sorry, been quiet for a little while on this issue. I love the concept and what this achieves. Just great.

The only quibbles I have is with the recent name changes since #28. As mentioned previously, I would like to utilise the methods added from this patch in some later patches to add cache tags for a view to the metadata; therefore, changing the names back to Metadata and not Cacheability. That does not suit the more general usage, whereas I think just 'metadata' does. We also do not use 'fillIn' in method names anywhere else? Can we just stick to something simple like add here?

Mh, a couple of new test failures snuck in, in the mean time.

This one looks catch-ish, and also beta safe, since if you don't implement this interface on your Views plugin, the worst you get is just an uncached plugin, not fatal errors, etc. and it only represents a schema addition, not a change.

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -361,4 +361,16 @@ public function adminSummary() {
   +    $contexts[] = 'cache.context.user';
   

   Seems like we ought to be able to figure out whether there's term access implemented or not and set this based on that? Just a @todo but this feels like a similar problem to node access.

2. +++ b/core/modules/views/src/Entity/View.php
   @@ -298,6 +298,48 @@ public function calculateDependencies() {
   +   * Cache metadata is set per view and per display, and ends up being stored in
   

   Happy happy.

Straight reroll.

Committed/pushed to 8.0.x, thanks!