Make Views use render caching and remove Views' own "output caching"

Is there any chance that we can try to make that an opt-in feature?

As long we haven't figured out cache tags yet, we don't have any invalidation yet.

Raising to critical, because while in Drupal 7, it's the site builder's responsibility to think about cache poisoning issues when enabling block or Views caches, that is no longer the case in Drupal 8, except for Views output cache, which I think qualifies this as a real D8 security bug. Tagging for triage though, since the branch maintainers might not see it that way.

fwiw I agree with the reasoning on this.

Good point @effulgentsia

In general I think we should clearly split up work on the result cache from other kind of cache, see #2452317: Let views result cache use cache contexts

Discussed with webchick, alexpott, and xjm, and they also agreed with #12 / #13.

How does this relate to #2450897: Cache Field views row output?

At one level, unrelated. This issue is about caching the entire rendered View (or rather, doing it in a more D8-proper way rather than HEAD's legacy custom approach). While #2450897: Cache Field views row output is about a single row.

At another level, I don't know, but suspect that #2450897: Cache Field views row output will be easier to do once this issue is finished. If that's true, I recommend postponing that issue on this one.

Sounds reasonable to me, the D7 version of views_row_cache is a mighty hack due to the fact that we never get to have full render arrays...

Discussed this with Wim today, and he suggested the opposite of what I said in #18. Specifically, that #2450897: Cache Field views row output can proceed without this issue, because we already have full render arrays where we need to for that issue, and that per #15, this issue is more blocked on #2452317: Let views result cache use cache contexts than on anything rendering related. In fact, Wim suggested that doing #2450897: Cache Field views row output and #2452317: Let views result cache use cache contexts first, independently of each other, should then make this issue simple, since caching a rendered View is just the combination of caching the results plus the rendering of each result. So, postponing this on those two.

putting that info in the summary, and postponing.

Well, either we bubble up cache metadata via cached or uncached rows, I don't see why this should be blocked on the other issue, at least the basic building blocks can be achieved entirely independent.

Just curious what happens :)

We identified which issues are causing these failures:

• #2263569: Bypass form caching by default for forms using #ajax.
• #2433591: Views using pagers should specify a cache context
• #2450897: Cache Field views row output
• #2487099: Set cache contexts for exposed sorts / items_per_page / offset.

#2450897: Cache Field views row output was committed (yay!), updated the test failures count by sending #26 to the bot again.

Updating the issue summary

Discussed with Daniel what role cache plugins should have with respect to render caching at display level and we came up with the Panoramic Views Render Caching strategy. Each cache plugin will be responsible for determining the actual display render cache behavior, while result caching will remain the same:

• The default cache plugin will become the Tag-based one, which will just implement render caching the usual way, at display level. Output caching will be completely replaced by render caching.
• For very complex (uncacheable) displays it will be possible possible to disable render caching via the None plugin.
• Finally, we will retain the Time-based plugin, which will implement regular render caching at display-level, but will override the invalidation strategy by getting rid of cache tags (aside possibly from the rendered one), so that only the cache entry expiration timestamp will matter. The actual expiration time will be configured via the usual settings.

The display renderable array will be provided by a static method on the DisplayPluginBase class, to avoid loading the view altogether. In case of a cache miss the renderable array will be available to the actual display object, that will be able to perform any required modification.

Another one: #2489966: The Views table style plugin does not specify cache contexts for click sorting.

Discussed with plach at the airport:

For 3. we will need a #cache_minage property (that does not bubble and hence is not within #cache), which controls that the RenderCache service should set that special flag for retrieving the cache, so that invalid / expired items are returned.

Will likely need some more thought as we do not want to call the cache plugin at all, but determine on cache retrieval, so we probably should be setting that always and then check if that property is present in the retrieved render array.ù

If it is set and invalid, we compare the timestamp of the minage with the actual age timestamp, else we return a cache miss.

--

- None plugin should just set max-age 0 and we are done.

- Tag based output kinda does nothing on itself. (what we already do in core except that we remove the old and now due to bubbling broken output cache support)

Here are a couple of test fixes.

Once again too many branches ;)

So current work.

More work.

Some more fixes.

A couple of test fixes

Fixed quite some of the failure:

• Kept the block one to not use direct render caching, we could iterate on that later
• Fixes the argument handling to work properly, yeah!
• Realized that #2433591: Views using pagers should specify a cache context is still needed, okay no surprise.
• We need some explicit test coverage for it

I investigated test failures a bit:

• MiniPagerTest, FieldWebTest, FrontPageTest should be fixed by #2433591: Views using pagers should specify a cache context and #2489966: The Views table style plugin does not specify cache contexts for click sorting.
• ContextualDynamicContextTest is caused by contextual links not providing cacheability metadata.
• BulkFormTest (and possibly ExposedFormTest) seems caused by cached form tokens / build ids, we need post render cache placeholders for them.

Applied #2489966: The Views table style plugin does not specify cache contexts for click sorting and #2433591: Views using pagers should specify a cache context and fixed some small points.

Agreed these two subissues would solve a lot of the problems.

Just trying to expand the test coverage and well, the page cache does not take into account the max age bubbling.

Committed one of the blockers and reviewed and bumped status of the other one.

Patch looks really encouraging to me - great that the necessary changes really aren't that much after all the other issues.

noting the blocker in the summary.

Postponed on #2433591: Views using pagers should specify a cache context to keep critical stats properly.

Not really sure why it should be postponed. Its required to make it pass, but its not required to continue on this issue.

Did some debugging on the failure in Drupal\contextual\Tests\ContextualDynamicContextTest
The problem is primarily caused by the code in views_page_display_pre_render::

  if ($view = views_get_page_view()) {
    views_add_contextual_links($element, 'page', $view, $view->current_display);
  }

In case we have a cache hit, views_get_page_view() does not exist ... as we have no active view. This is a general problem, which will also hit themers, given that they want to change / add some css classes depending for example on the current result of the view.

At least for this case its enough to check for the route object itself.

Why is the views_page_display_pre_render() called when there is no view?

Can we call it in the #pre_render callback itself and use another hook for being able to tamper with the render array itself (i.e. cache keys, display parameters, cache contexts up-front to optimize experience)?

Why is the views_page_display_pre_render() called when there is no view?

Can we call it in the #pre_render callback itself and use another hook for being able to tamper with the render array itself (i.e. cache keys, display parameters, cache contexts up-front to optimize experience)?

... We want to place the contextual link not on the level of the rendering of the view, but on the page level, so we add that as pre_render function to #type page.

As this is page specific, could we instead add the necessary information to $element when we have constructed the view?

Then we would have all overhead in #pre_render, but the contextual link is still dynamically created ...

Updated issue summary with #36 and #38.

As this is page specific, could we instead add the necessary information to $element when we have constructed the view?

Then we would have all overhead in #pre_render, but the contextual link is still dynamically created ...

Does that allow you to bubble up the contextual links information? We want to place it on the page render array.

No what I mean is to add information so that we can re-create information without having the view object itself:

Lets take a look at the _dependencies_ on $view of the https://api.drupal.org/api/drupal/core%21modules%21views%21views.module/... function:

function views_add_contextual_links(&$render_element, $location, ViewExecutable $view, $display_id) {
  // We use the $view->getShowAdminLinks() function, need to know what that depends on.
  if (\Drupal::moduleHandler()->moduleExists('contextual') && $view->getShowAdminLinks()) {

    // Given the same display_id the plugin_id should not change, so would be safe to pre-compute.
    $plugin_id = $view->displayHandlers->get($display_id)->getPluginId();
    $plugin = Views::pluginManager('display')->getDefinition($plugin_id);

     // ... stuff that only depends on $plugin ...

        // If the link was valid, attach information about it to the renderable
        // array.
        if ($valid) {
          $render_element ['#views_contextual_links'] = TRUE;
          $render_element ['#contextual_links'][$group] = array(
            'route_parameters' => $args,
            'metadata' => array(
              'location' => $location,
// Here we use the $view->storage->id(), which we can also pre-compute easily.
              'name' => $view->storage->id(),
              'display_id' => $display_id,
            ),
          );
          // If we're setting contextual links on a page, for a page view, for a
          // user that may use contextual links, attach Views' contextual links
          // JavaScript.
          if ($location === 'page' && $render_element ['#type'] === 'page' && \Drupal::currentUser()->hasPermission('access contextual links')) {
            $render_element ['#attached']['library'][] = 'views/views.contextual-links';
          }
        }
      }
    }
  }
}

So overall this depends on:

- The location, which we have already anyway.
- The display ID - which we have before #pre_render stage
- The plugin ID - which we have before #pre_render stage
- The view storage ID - which we have before #pre_render stage
- The getShowAdminLinks(), which we need to still analyze:

https://api.drupal.org/api/drupal/core!modules!views!src!ViewExecutable....

only depends on the display option, which we can cache as an extra property.

Overall:

location - have already -- cheap
display ID - have already, part of cache keys -- cheap

plugin ID, view storage ID, getShowAdminLinksResult() can put during #pre_render into the $element like:

#view_plugin_id
#view_storage_id
#view_display_show_admin_links

and then use #cache_properties to store this across cache get / sets and boom we can simplify views_add_contextual_links() to no longer rely on the $view object itself, but just on properties in $element - regardless if it was retrieved via #pre_render or cache get.

Oh and the thing I missed:

No, bubbling is not possible, but as this relies on a global, I would not feel bad to use views_set_page_render_array() / views_get_page_render_array() instead to keep BC.

As its part of the main controller, it would be executed always anyway ... (unless we cache the whole page)

Another possibility is (once placeholders exist) to just use a placeholder in #attached placeholders, but then use the placeholder string in the page pre_render. [ and just share the placeholder string between them as e.g. a CONSTANT ]

That is likely the absolute cleanest solution, but can only be done as follow-up as placeholders should obviously not block this ...

#71: No, that _is_ the strategy of #36 actually (as explained to me by plach), so we are all on the same page.

I'm not entirely convinced that #68 makes things easier than the current state of the patch, anyway, here is a patch for just that.

But for sure I totally get the point in not having to load the view object on cached pages.

Overall nice, some nit-picks on how #68 was implemented.

1. +++ b/core/modules/views/views.module
   @@ -309,9 +309,10 @@ function views_page_display_pre_render(array $element) {
      $route = \Drupal::routeMatch()->getRouteObject();
      if ($route && $route->hasDefault('view_id')) {
   -    $view = Views::getView($route->getDefault('view_id'));
   -    $view->setDisplay($route->getDefault('display_id'));
   -    views_add_contextual_links($element, 'page', $view, $view->current_display);
   +    $element['#view_id'] = $route->getDefault('view_id');
   +    $element['#view_display_show_admin_links'] = $route->getOption('_view_display_show_admin_links');
   +    $element['#view_display_plugin_id'] = $route->getDefault('display_id');
   

   The idea was that $element would have that already cached, just need to find the right element on the page.

2. +++ b/core/modules/views/views.module
   @@ -404,15 +405,20 @@ function views_preprocess_html(&$variables) {
   +  $render_element['#cache_properties'] = ['view_id', 'view_display_show_admin_links', 'view_display_plugin_id'];
   

   Needs to use '#view_id', '#view_display_show_admin_links', etc.

   And this is likely not the right place for setting the #cache_properties value.

just need to find the right element on the page.

Well, the question is whether we can assume that the main content is placed in a particular level on the page render array, I guess we can?

It might be easily the case that maybe that regions are rendered before the $page array is assembled together

That is why I voted for simplicity with adding _views_get_page_render_array / _views_set_page_render_array (@internal).

First trying to make blocks lazy, I guess we actually don't need that because block caching already does all we need?

This is work to use render caching for the rest export.

Now also with some basic work to get rid of output caching.

Listed a couple of points.

Some progress.

doh!

Continous work

Drupal Subuser Module
i am not able to assign automatically Parent roles to subuser , while creating sub user .
But same working in D6 but not in D7
Please help me

@nagraj404:

This is a completely unrelated issue, please use the search available at https://www.drupal.org/project/issues/subuser?categories=All to see whether your issue has already been responded, otherwise create a support request for the Subuser project at:

https://www.drupal.org/node/add/project-issue/subuser

Improved issue summary :)

A code review to get familiarity with last patches:

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -306,15 +313,10 @@ public function render() {
   +    $build['#cache-content-type'] = $this->getMimeType();
   

   Why are we using inconsistent separators in the property name? Shouldn't this be #cache_content_type?

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2136,16 +2137,25 @@ public function render() {
   +  protected function renderSetCacheData(array &$element) {
   

   Missing PHP doc

3. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2136,16 +2137,25 @@ public function render() {
   -    // If the output is a render array, add cache tags, regardless of whether
   -    // caching is enabled or not; cache tags must always be set.
   +      // If the output is a render array, add cache tags, regardless of whether
   +      // caching is enabled or not; cache tags must always be set.
   

   Huh?

4. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2314,8 +2324,8 @@ public function execute() { }
   +  public function buildRenderable(array $args = [], $caching = TRUE) {
   

   Mabye just $cache, intended as a verb?

5. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -203,6 +206,10 @@ protected function getRoute($view_id, $display_id) {
   +    $route->setOption('returns_response', !empty($this->getPluginDefinition()['returns_response']));
   

   Would it make sense to use an imperative form: return_response?

6. +++ b/core/modules/views/src/Routing/ViewPageController.php
   @@ -65,31 +66,20 @@ public static function create(ContainerInterface $container) {
          // Allow parameters be pulled from the request.
          // The map stores the actual name of the parameter in the request. Views
          // which override existing controller, use for example 'node' instead of
          // arg_nid as name.
   

   This comment seems to need an update too.

7. +++ b/core/modules/views/src/Routing/ViewPageController.php
   @@ -104,12 +94,12 @@ public function handle($view_id, $display_id, Request $request, RouteMatchInterf
   +    $class = $route->getOption('_view_display_plugin_class');
   +    if ($route->getOption('returns_response')) {
   +      return $class::returnResponse($view_id, $display_id, $args);
        }
        else {
   -      return $view->buildRenderable($display_id, $args);
   +      return $class::buildBasicRenderable($view_id, $display_id, $args);
        }
   

   Given that both methods are static and take the same arguments, do we really need to differentiate them? The actual return value seems an implementation detail to me. Could it be just a static ::handle() method in both cases?

8. +++ b/core/modules/views/src/Tests/AssertViewsCacheTagsTrait.php
   @@ -32,14 +32,20 @@
   +    $copy = $build;
   

   In other places we use $original instead of $copy :)

9. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1492,12 +1481,14 @@ public function getCacheTags() {
   +   * @param bool $caching
   ...
   +  public function buildRenderable($display_id = NULL, $args = array(), $caching = TRUE) {
   

   Same as above

10. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -1492,12 +1481,14 @@ public function getCacheTags() {
    -   * @return array|null
    -   *   A renderable array with #type 'view' or NULL if the display ID was
    -   *   invalid.
    +   * @return array|null A renderable array with #type 'view' or NULL if the display ID was
    +   * A renderable array with #type 'view' or NULL if the display ID was
    +   * invalid.
    

    Oops :)

Ignoring the reviews so far, just posting work on rss/feed related code.

+++ b/core/modules/node/src/Plugin/views/row/Rss.php
@@ -60,12 +62,26 @@ class Rss extends RssPluginBase {
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {

PHP doc

Some intermediate work.

Fixed hopefully most of the feed related failures

mh

+++ b/core/modules/views/src/Plugin/views/cache/Time.php
@@ -177,10 +177,13 @@ protected function cacheExpire($type) {
+  protected function cacheSetMaxAge($type) {

This name is a bit confusing: shouldn't it be ::getCacheMaxAge()?

Just some general work.

More work, now working on getting the contextual links working

Alright, let's fix contextual links.

I'm reclassifying this as a bug because of the security issue mentioned in #10.

Re-titling based on #118

I'm not an expert on views or caching, so I can't give an in-depth review of this, but here's a few things that jumped out at me:

1. +++ b/core/modules/comment/src/Plugin/views/row/Rss.php
   @@ -84,7 +84,7 @@ public function render($row) {
   +    $item_text = [];
   
   @@ -115,14 +115,16 @@ public function render($row) {
   +      $item_text = $build;
   ...
        $item->description = $item_text;
   
   +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -111,7 +130,7 @@ public function render($row) {
   +    $item_text = [];
   
   @@ -154,22 +173,23 @@ public function render($row) {
   +      $item_text = $build;
   ...
   +    $item->description = $item_text;
   

   since $item_text is no longer concatenated text, would it make sense to rename it something more explanatory of what it's storing?

2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -273,28 +273,35 @@ public function collectRoutes(RouteCollection $collection) {
   +    $renderer = \Drupal::service('renderer');
   
   +++ b/core/modules/views/src/Plugin/views/display/Feed.php
   @@ -67,25 +69,38 @@ protected function getType() {
   +    $renderer = \Drupal::service('renderer');
   

   Can this service be injected?

3. +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -45,7 +45,7 @@ protected function setUp() {
   +  public function ptestFieldBasedViewCacheTagsWithCachePluginNone() {
   
   @@ -65,7 +65,7 @@ public function testFieldBasedViewCacheTagsWithCachePluginTag() {
   +  public function ptestFieldBasedViewCacheTagsWithCachePluginTime() {
   
   @@ -135,14 +140,14 @@ protected function assertCacheTagsForFieldBasedView($do_assert_views_caches) {
   +  public function ptestEntityBasedViewCacheTagsWithCachePluginNone() {
   ...
   +  public function ptestEntityBasedViewCacheTagsWithCachePluginTag() {
   
   @@ -155,7 +160,7 @@ public function testEntityBasedViewCacheTagsWithCachePluginTag() {
   +  public function ptestEntityBasedViewCacheTagsWithCachePluginTime() {
   
   @@ -205,7 +210,7 @@ protected function assertCacheTagsForEntityBasedView($do_assert_views_caches) {
   +  public function ptestBuildRenderableWithCacheContexts() {
   
   @@ -218,7 +223,7 @@ public function testBuildRenderableWithCacheContexts() {
   +  public function ptestViewAddCacheMetadata() {
   

   Why are these functions renamed with a prefix 'p'.

1. +++ b/core/modules/system/src/Tests/Cache/AssertPageCacheContextsAndTagsTrait.php
   @@ -76,9 +76,13 @@ protected function assertPageCacheContextsAndTags(Url $url, array $expected_cont
   +  protected function debugCacheTags($actual_tags, $expected_tags) {
   

   Missing PHP doc

2. +++ b/core/modules/views/src/Routing/ViewPageController.php
   @@ -100,7 +100,11 @@ public function handle($view_id, $display_id, RouteMatchInterface $route_match)
   +      views_set_curre
   

   WAT? :)

3. +++ b/core/modules/views/src/Tests/Plugin/CacheTagTest.php
   @@ -88,93 +90,118 @@ protected function setUp() {
   +  protected function getRenderCache(ViewExecutable $view) {
   

   Missing PHP doc. Also, can we name this getRenderCacheEntry? Otherwise it seems we are returning the service.

4. +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -45,7 +45,7 @@ protected function setUp() {
   +  public function ptestFieldBasedViewCacheTagsWithCachePluginNone() {
   
   @@ -65,7 +65,7 @@ public function testFieldBasedViewCacheTagsWithCachePluginTag() {
   +  public function ptestFieldBasedViewCacheTagsWithCachePluginTime() {
   
   @@ -135,14 +140,14 @@ protected function assertCacheTagsForFieldBasedView($do_assert_views_caches) {
   +  public function ptestEntityBasedViewCacheTagsWithCachePluginNone() {
   ...
   +  public function ptestEntityBasedViewCacheTagsWithCachePluginTag() {
   
   @@ -155,7 +160,7 @@ public function testEntityBasedViewCacheTagsWithCachePluginTag() {
   +  public function ptestEntityBasedViewCacheTagsWithCachePluginTime() {
   
   @@ -205,7 +210,7 @@ protected function assertCacheTagsForEntityBasedView($do_assert_views_caches) {
   +  public function ptestBuildRenderableWithCacheContexts() {
   
   @@ -218,7 +223,7 @@ public function testBuildRenderableWithCacheContexts() {
   +  public function ptestViewAddCacheMetadata() {
   

   Cheater :)

5. +++ b/core/modules/views/views.theme.inc
   @@ -917,7 +917,12 @@ function template_preprocess_views_view_row_rss(&$variables) {
   +  $variables['description'] = $renderer->renderPlain($item->description);
   

   Judging from the comment, this seems to require regular metadata bubbling. Should we use ::render() instead?

Thank you for your review, tried to address them.

This should remove quite some of the failures again, puh!
Looking forward to another 100K patch :(

@disasm tests renamed to start with p ... are skipped.

Hopefully one less failure.

@disasm tests renamed to start with p ... are skipped.

Yeah that is a thing I do in order to speed up tests during debugging
failures.

1. +++ b/core/modules/aggregator/src/Tests/AddFeedTest.php
   @@ -28,6 +28,7 @@ public function testAddFeed() {
   +    debug($feed->getWebsiteUrl());
   

   hm

2. +++ b/core/modules/system/src/Tests/Cache/AssertPageCacheContextsAndTagsTrait.php
   @@ -79,6 +79,14 @@ protected function assertPageCacheContextsAndTags(Url $url, array $expected_cont
      protected function debugCacheTags($actual_tags, $expected_tags) {
   

   Sorry for being a PITA, I just realized this is missing type hints.

3. +++ b/core/modules/views/src/Views.php
   @@ -56,6 +56,13 @@ class Views {
   +   * The current page render array.
   +   *
   +   * @var array
   +   */
   +  protected static $pageRenderArray;
   +
   +  /**
       * Returns the views data service.
       *
       * @return \Drupal\views\ViewsData
   @@ -534,4 +541,22 @@ protected static function t($string, array $args = array(), array $options = arr
   
   @@ -534,4 +541,22 @@ protected static function t($string, array $args = array(), array $options = arr
        return static::$translationManager->translate($string, $args, $options);
      }
    
   +  /**
   +   * Sets the current page views render array.
   +   */
   +  public static function &setPageRenderArray(array &$element = NULL) {
   +    if (isset($element)) {
   +      static::$pageRenderArray = &$element;
   +    }
   +
   +    return static::$pageRenderArray;
   +  }
   +
   +  /**
   +   * Gets the current views page render array.
   +   */
   +  public static function &getPageRenderArray() {
   +    return static::$pageRenderArray;
   +  }
   +
   

   Why don't we define these on the Page display plugin?

   Missing @param, btw.

4. +++ b/core/modules/views/tests/src/Unit/Routing/ViewPageControllerTest.php
   @@ -43,6 +43,13 @@ class ViewPageControllerTest extends UnitTestCase {
   +  protected $defaultRenderArray = [
   

   Missing PHP doc :)

5. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2136,16 +2137,25 @@ public function render() {
   +  protected function renderSetCacheData(array &$element) {
   

   Same here :)

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2136,16 +2137,25 @@ public function render() {
   +    $element['#cache']['max-age'] = Cache::mergeMaxAges($element['#cache']['max-age'], $cache->getCacheMaxAge());
   

   This would mean that a display can essentially override the max-age, if it's lower than the cache setting. That doesn't seem like the right behaviour?

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2314,18 +2324,43 @@ public function execute() { }
   +      $build['#cache'] = $this->view->element['#cache'];
   

   Where is the use case of setting ->element['#cache'] ?

3. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2314,18 +2324,43 @@ public function execute() { }
   +    $build['#cache_properties'] =  ['#view_id', '#view_display_show_admin_links', '#view_display_plugin_id'];
   

   Is this a views specific thing? Can it live within the #cache array anyway?

4. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginInterface.php
   @@ -469,10 +469,15 @@ public function execute();
   +  public function buildRenderable(array $args = [], $cache = TRUE);
   

   Where and when do you opt out? Wouldn't the cache plugin control this? i.e. if it's 'none' do nothing.

5. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -113,14 +114,27 @@ protected function defineOptions() {
   +  public static function buildBasicRenderable($view_id, $display_id, array $args = [], Route $route = NULL) {
   ...
   +    if ($route) {
   

   Do we need a route parameter? Shouldn't Page always we using a route, and just assume this behaviour in this overridden method?

6. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -113,14 +114,27 @@ protected function defineOptions() {
   -    views_set_page_view($this->view);
   

   Is there a replacement for this functionality?

7. +++ b/core/modules/views/src/Routing/ViewPageController.php
   @@ -104,12 +94,16 @@ public function handle($view_id, $display_id, Request $request, RouteMatchInterf
   +      Views::setPageRenderArray($build);
   

   Is this the replacement for views_set_page_view?

8. +++ b/core/modules/views/views.theme.inc
   @@ -917,7 +917,12 @@ function template_preprocess_views_view_row_rss(&$variables) {
   +  $variables['description'] = is_array($item->description) ? $renderer->render($item->description) : $item->description;
   

   Don't we create the item anyway? So we could make sure it's always an array?

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -273,28 +273,35 @@ public function collectRoutes(RouteCollection $collection) {
+    $response->headers->set('Content-type', $build['view_build']['#content_type']);

Nice to see that fixed too :D

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -273,28 +273,35 @@ public function collectRoutes(RouteCollection $collection) {
+    $response->headers->set('Content-type', $build['view_build']['#content_type']);

I don't think we need to set this. Symfony does it for us. See Response::prepare(). Maybe it's better to explicitly set it anyway?

Thank you for your reviews!! This is just temporary work in order for damian to join the party.

Thanks! Was just going to start by adding a couple of assertions to help with our confusion over the RestExport headers, they actually work fine, as per my comment in #132

Let's check whether #135 provides more failures

From IRC:

- When someone uses setPage() we need to change the cache keys (injected data)
- When someone does not use setPage(), we set the cache context for the pager (already in place)

Same for items_per_page and offset. (and later also contexts, but other issue as broken atm. anyway)

Also a key sentence by dawehner for anyone still confused:

< dawehner> yeah cache contexts are just needed in case you start with a render array

(and don't have objects to work with)

This is key to everything.

If someone injects data in a view in that way, then the view itself is not affected and cache keys are fine, but if that should e.g. be cached again by e.g. block cache or panels cache or ... THEN the callers that injected the data into the view needs to bubble up that information that this view varies by that.

And the reason it is the callers responsibility for upper levels is:

- It is the caller who accesses e.g. the request and gets the 'page' parameter.

Rule of thumb:

"Whoever uses an external resource to make a decision or command a decision on another class and that decision varies the output, needs to specify the variation criteria, i.e. the cache context".

hm, Sorry for being a PITA, I just realized this is missing type hints.

Fixed

Why don't we define these on the Page display plugin?

Good point, moved it, btw. storm can do it.

Missing PHP doc :)

Same here :)

Fixed.

This would mean that a display can essentially override the max-age, if it's lower than the cache setting. That doesn't seem like the right behaviour?

Yeah that is the central problem, we want to have a min cache age, see issue summary.

Where is the use case of setting ->element['#cache'] ?

For example when you want to vary things via a cache context, when you don't have the view object yet

Is this a views specific thing? Can it live within the #cache array anyway?

#cache_properties is a thing which exists, but we want to store those additional information in order to make contextual links working outside of a loaded view object. Do you think this is okay?

Where and when do you opt out? Wouldn't the cache plugin control this? i.e. if it's 'none' do nothing.

Well, I thought that this method is more of an API function people could use. Cache plugins would opt out on top of that. I see your point thought, mhhh

Do we need a route parameter? Shouldn't Page always we using a route, and just assume this behaviour in this overridden method?

Well, technically its defined on the parent class, so additional parameters can just be optional. For other display types though, it does not make sense to add $route.

Is there a replacement for this functionality?
Is this the replacement for views_set_page_view?

Yes this is the replacement, now on the Page display class.

Don't we create the item anyway? So we could make sure it's always an array?

The aggregator implementation is a little bit odd:

  public function render($row) {
    $entity = $row->_entity;

    $item = new \stdClass();
    foreach ($entity as $name => $field) {
      // views_view_row_rss takes care about the escaping.
      $item->{$name} = $field->value;
    }

    $item->elements = array(

Do you think we should convert $item->description there to be a render array?

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2142,6 +2142,12 @@ public function render() {
+   * Set ups the render cache information onto a view render element

<pita>
"Setups", missing trailing dot :)
</pita>

"Setups", missing trailing dot :)

Thank you a lot for your quick reviews!!!!!!!

Wow, this debugging session needed to much time, but yeah! for IDEs show memory addresses

Working on test failures atm, some cleanup in the meantime.

Work done on #2463959: dawehner testing issue

Nice IS picture. Wish all IS had pix.

"Some" work in the meantime ...

Its green!

This time for real

Nice work!

Awesome it's green.

1. +++ b/core/modules/comment/comment.module
   @@ -211,6 +211,29 @@ function comment_node_links_alter(array &$node_links, NodeInterface $node, array
   +function comment_entity_view(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display, $view_mode, $langcode) {
   

   Any reason for this change and removing it from CommentLinkBuilder CLB?

2. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -60,15 +62,32 @@ class Rss extends RssPluginBase {
   +    $this->renderer = $renderer;
   ...
   +      $container->get('renderer')
   

   Why are we injecting renderer if we are removing calls of drupal_render_root?

3. +++ b/core/modules/views/src/Element/View.php
   @@ -70,7 +75,16 @@ public static function preRenderViewElement($element) {
   -        views_add_contextual_links($element, 'view', $view, $view->current_display);
   +        if (empty($view->display_handler->getPluginDefinition()['returns_response'])) {
   +          $element['#view_id'] = $view->storage->id();
   +          $element['#view_display_show_admin_links'] = $view->getShowAdminLinks();
   +          $element['#view_display_plugin_id'] = $view->display_handler->getPluginId();
   +          views_add_contextual_links($element, 'view', $view->current_display);
   ...
   +      if (empty($view->display_handler->getPluginDefinition()['returns_response'])) {
   +        $element['#attributes']['class'][] = 'views-element-container';
   +        $element['#theme_wrappers'] = array('container');
   
   +++ b/core/modules/views/src/Plugin/Block/ViewsBlockBase.php
   @@ -195,14 +195,19 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   -      views_add_contextual_links($output, $block_type, $this->view, $this->displayID);
   +      $output['#view_id'] = $this->view->storage->id();
   +      $output['#view_display_show_admin_links'] = $this->view->getShowAdminLinks();
   +      $output['#view_display_plugin_id'] = $this->view->display_handler->getPluginId();
   +      views_add_contextual_links($output, $block_type, $this->displayID);
   

   Can we have comments for these changes? :)

4. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -191,20 +191,8 @@ public function cacheSet($type) {
   +        $expire = ($this->cacheSetMaxAge($type) === Cache::PERMANENT) ? Cache::PERMANENT : (int) $this->view->getRequest()->server->get('REQUEST_TIME') + $this->cacheSetMaxAge($type);
   

   Can't we just use REQUEST_TIME instead of (int) $this->view->getRequest()->server->get('REQUEST_TIME')

5. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -490,21 +491,30 @@ public function setArguments(array $args) {
   +    // Calls like ::unserialize() might setup a $page might call this method
   +    // without a proper $page.
   

   Doesn't make sense at all.

Thank you jibran for your super fast review response time!

Any reason for this change and removing it from CommentLinkBuilder CLB?

I'll put that into the issue summary to explain that to others.

Why are we injecting renderer if we are removing calls of drupal_render_root?

Good catch! Fixed that ...

Can't we just use REQUEST_TIME instead of (int) $this->view->getRequest()->server->get('REQUEST_TIME')

Well, I just wanted to do it the right way ... see #2459155: Remove REQUEST_TIME from bootstrap.php

Doesn't make sense at all.

Fair :) Let's update it a bit.

Adapt the test views cache test coverage to test render caching

Done

Here is more test coverage, puh, things aren't trivial always.

Updating the remaining tasks ...

Went through the list of plugins defined in views.

Bugs I realized:

• The Serializer style plugin does not expose the request format as cache context. Added some test coverage for that.
• Various things still lack proper cache contexts, see @todos added by this interdiff. Yes quite some of them are actually security issues like access checking (permissions, roles)!!
• Enabled tag based caching for the frontpage view, let's see what fails based upon that

(Crosspost, reviewed #158)

Additional test coverage looks great. Some minor stuff:

1. +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php
   @@ -54,7 +54,7 @@ protected function setUp() {
   +  public function ptestFrontPage() {
   
   @@ -173,7 +173,7 @@ protected function assertNotInResultSet(ViewExecutable $view, array $not_expecte
   +  public function ptestAdminFrontPage() {
   
   @@ -189,7 +189,7 @@ public function testAdminFrontPage() {
   +  public function ptestCacheTagsWithCachePluginNone() {
   
   @@ -197,7 +197,7 @@ public function testCacheTagsWithCachePluginNone() {
   +  public function ptestCacheTagsWithCachePluginTag() {
   

   Ha!

2. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -178,6 +183,75 @@ public function testSerializerResponses() {
   +  public function testRestRenderCaching() {
   

   PHP doc

3. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -178,6 +183,75 @@ public function testSerializerResponses() {
   +    // Create a new entity and ensure that the cache tags are taken over.
   

   Can we explain that we are removing one item because we display only ten?

4. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -178,6 +183,75 @@ public function testSerializerResponses() {
   +    entity_create('entity_test', array('name' => 'test_11', 'user_id' => $this->adminUser->id()))->save();
   

   EntityTest::create()?

5. +++ b/core/modules/system/tests/modules/entity_test/src/Cache/EntityTestViewGrantsCacheContext.php
   @@ -27,7 +27,8 @@ public static function getLabel() {
   +    // Return a constant value.
   +    return '299792458';
   

   Why? To avoid random failures?

6. +++ b/core/modules/views/src/Tests/AssertViewsCacheTagsTrait.php
   @@ -105,4 +106,44 @@ protected function assertViewsCacheTags(ViewExecutable $view, $expected_results_
   +  protected function assertViewsCacheTagsFromStaticRenderArray(ViewExecutable $view, array $expected_render_array_cache_tags, $views_caching_is_enabled) {
   

   PHP doc

7. +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -84,7 +84,7 @@ public function testFieldBasedViewCacheTagsWithCachePluginTime() {
   -   */
   +  */
   

   Mmh, no

8. +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -126,6 +129,7 @@ protected function assertCacheTagsForFieldBasedView($do_assert_views_caches) {
   +    // $this->assertViewsCacheTagsFromStaticRenderArray($view, $tags_page_2, $do_assert_views_caches);
   

   Hm

9. +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -134,6 +138,8 @@ protected function assertCacheTagsForFieldBasedView($do_assert_views_caches) {
   +    $this->assertViewsCacheTagsFromStaticRenderArray($view, $tags_page_1, $do_assert_views_caches);
   +    $this->assertViewsCacheTagsFromStaticRenderArray($view, $tags_page_1, $do_assert_views_caches);
   

   Double call

1. +++ b/core/modules/user/src/Plugin/views/access/Permission.php
   @@ -132,4 +133,19 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +  public function getCacheContexts() {
   +    // @todo Write explicit test coverage that the variation works.
   +    return ['user.permissions'];
   +  }
   +
   

   Wondering whether #2493033: Make 'user.permissions' a required cache context will make all these unnecessary. It's not critical, though.

2. +++ b/core/modules/views/src/Plugin/views/sort/Random.php
   @@ -37,6 +37,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +    // @todo Write test coverage to ensure that opt out of caching works.
   

   Is this opting out from display-level render caching or whole render caching? I'm asking because row caching should still work.

Blocked on #2498857: Support min-age in render caching, not postponing since we have other areas to focus on.

Thanks again for the quite quick review!

We discussed about min age and moved it first to its own subissue: #2498857: Support min-age in render caching

Can we explain that we are removing one item because we display only ten?

Good point.

Why? To avoid random failures?

Well, to have something which can be used between requests, for example inside the test code and the actual site.
Let me explain that a bit better.

Is this opting out from display-level render caching or whole render caching? I'm asking because row caching should still work.

You are absolutely right, this just takes care about the order.

+++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
@@ -126,6 +129,7 @@ protected function assertCacheTagsForFieldBasedView($do_assert_views_caches) {
+ // $this->assertViewsCacheTagsFromStaticRenderArray($view, $tags_page_2, $do_assert_views_caches);
Hm

Ah, let me manipulate the current request in the test.

Form the interdiff it seems a few items from #164 were not addressed: 1, 7, 9.

Form the interdiff it seems a few items from #164 were not addressed: 1, 7, 9.

Well, 1, 7 got addressed in #163 already and 9 is part of the interdiff:

    $this->assertViewsCacheTagsFromStaticRenderArray($view, $tags_page_1, $do_assert_views_caches);
-    $this->assertViewsCacheTagsFromStaticRenderArray($view, $tags_page_1, $do_assert_views_caches);
     $this->assertTrue(strpos($build['#markup'], $random_name) !== FALSE);

Opened up a follow up around the discussion @berdir, @catch, @msonnabaum, @plach had in IRC: #2498969: Time-based cached views get invalidated too frequently

Sorry, I need a pair of glasses :)

There are signs that you sit too much in front of the computer, see interdiff

Added the test coverage for random sort, role and permission access checking.

Fixed the failures of #167

.

1. +++ b/core/modules/user/src/Tests/Views/AccessPermissionTest.php
   @@ -40,4 +41,32 @@ function testAccessPerm() {
   +    $build = DisplayPluginBase::buildBasicRenderable('test_access_perm', 'default');
   +
   +    // First access as user without access, then with access.
   +    $account_switcher->switchTo($this->normalUser);
   +    $result = $renderer->renderPlain($build);
   +    $this->assertNotEqual($result, '');
   +
   +    $build = DisplayPluginBase::buildBasicRenderable('test_access_perm', 'default');
   +    $account_switcher->switchTo($this->webUser);
   ...
   +    $this->assertEqual($result, '');
   
   +++ b/core/modules/user/src/Tests/Views/AccessRoleTest.php
   @@ -92,4 +93,36 @@ function testAccessRole() {
   +    $build = DisplayPluginBase::buildBasicRenderable('test_access_role', 'default');
   +
   +    // First access as user without access, then with access.
   +    $account_switcher->switchTo($this->normalUser);
   +    $result = $renderer->renderPlain($build);
   +    $this->assertNotEqual($result, '');
   +
   +    $build = DisplayPluginBase::buildBasicRenderable('test_access_role', 'default');
   +    $account_switcher->switchTo($this->webUser);
   +    $result = $renderer->renderPlain($build);
   +    $this->assertEqual($result, '');
   

   Übernitpick (trying to win the "nitpick of the month" context):

   It would be more readable if this line were below the comment so we'd have two symmetric blocks of 4 lines implementing symmetric logic :)

2. +++ b/core/modules/user/src/Tests/Views/AccessRoleTest.php
   @@ -92,4 +93,36 @@ function testAccessRole() {
   +    $build = DisplayPluginBase::buildBasicRenderable('test_access_role', 'default');
   +
   +    // First access as user without access, then with access.
   +    $account_switcher->switchTo($this->normalUser);
   +    $result = $renderer->renderPlain($build);
   +    $this->assertNotEqual($result, '');
   +
   +    $build = DisplayPluginBase::buildBasicRenderable('test_access_role', 'default');
   +    $account_switcher->switchTo($this->webUser);
   +    $result = $renderer->renderPlain($build);
   +    $this->assertEqual($result, '');
   

   Same here

3. +++ b/core/modules/views/src/Tests/Handler/SortRandomTest.php
   @@ -26,10 +28,17 @@ class SortRandomTest extends ViewUnitTestBase {
   +   * In total we have then 60 entries, which makes a probability of a collision
   +   * of 1/60!, which is around 1/1E80, which is higher than the estimated amount
   +   * of protons / electrons in the observable universe, also called the eddington
   +   * number.
   +   *
   +   * @see http://en.wikipedia.org/wiki/Eddington_number
   

   dawehner++

4. +++ b/core/modules/views/src/Tests/Handler/SortRandomTest.php
   @@ -96,4 +105,39 @@ public function testRandomOrdering() {
   +   * The random sorting should opt out of caching by defining a max age 0.
   

   Can we add a note that this is not supposed to opt out from row render caching?

Finally, we will retain the Time-based plugin, which will implement regular render caching at display-level, but will override the invalidation strategy by ignoring invalid cache tags, — where is this happening?

Given that we don't change the behaviour of the time based cache plugin in this issue, we should do that as part of #2498969: Time-based cached views get invalidated too frequently

Could you add to the IS' "Review help" section explanations for the following more meta-ish questions: A) the changes to how Views' contextual links are added to the page, B) the overriding/extending of #cache[keys] in several places. (Several concrete questions in the review below.)

Answered ...

Would you say that at this point it makes more sense to have Views' plugin/handlers interfaces extending CacheablePluginInterface, hence forcing every handler/plugin to consider their cacheability impact? It seems that in this patch all remaining handlers/plugins (that didn't already provide cacheability information) now provide this metadata too.

Not as part of this issue, but still, we need to figure out the different levels, like render vs. result caching ...

This is weird… for every comment field, if there is a 'links' display component in the entity's 'RSS' view mode and the first comment is published, then we a link to basically "http://mysite.com/node/X#comments".

WTF? :P

Would be great to document why it works this way. I know it's pulled out of CommentLinkBuilder, but there's zero documentation right now to explain the if- and for-statements.

No there is in HEAD, see review section, its pretty epic, that it actually works.

Another excellent catch.

Well, that was simply a test failure ...

s/Setups/Sets/

I'll just try it long enough, until its part of the english language.

This looks weird. Perhaps correct, but then the formatting for the added line is definitely off.

Well yeah, there is no reason to for example post_render_cache both on the view element and the view_build twice, you just need it either on one or the other.

I think all of this can be simplified to:

That is pretty cool!

Why unsetting #cache? Why not let cache tags be bubbled, for example?
Needs a comment explaining the rationale.

This is tricky, but well, we want to certainly unset the cache keys, as we don't want the element to be cached. Let's try that.

Why set the #cache_properties separately?

Fixed that

Is this really necessary here? Could use a comment explaining why if "yes".

No idea, I'll just ignore that now :P

Difficult to parse.

Added ...

Let's fix it ...

Addressed the reviews of #179 and #183. Thanks a lot of for the both great and quick feedback

It would be more readable if this line were below the comment so we'd have two symmetric blocks of 4 lines implementing symmetric logic :)

Hei, I think this is a total valid point! Sorry no, award for you ;)

Can we add a note that this is not supposed to opt out from row render caching?

Sure

setCacheMaxAge() would be better IMHO, but would probably be internally inconsistent.

Have a look at the method, it doesn't set the max age, but rather it determines the max age for ::cacheSet()

applyDisplayCacheability()

Went with applyDisplayCachablityMetadata()