Cache Field views row output

This only affect Field views, not Entity views.

Views row has nothing to do with views field display. Views row can have entity display plugin in it.

Not sure about D8, but on the D7 project where I developed VRC, you got an improvement in the response time of ~25% with warm caches on a page with a single table view with 25 items per page. Another page with a table view with no pager (~400 items), went from ~4500ms to ~400ms.

This obviously depends heavily on the view, but you start seeing some real gain when you have a fair amount of rows. We'll need to do some profiling, but I wouldn't be surprised if it turned out that we have a gain only for views having at least X fields and Y rows in the result.

Wondering how this would play with combined with #1867518: Leverage entityDisplay to provide fast rendering for fields, which should leverage entity cache on views rows ?

While I think having caching per row could dramatically improve stuff (yeah to VCR), I think we should still try out #1867518: Leverage entityDisplay to provide fast rendering for fields and more potential issues in there.

We should absolutely try out #1867518: Leverage entityDisplay to provide fast rendering for fields, because the best time spent is the time you don't spent :-D.

If we can make something 150 ms faster thats absolutely worth it.

Do the numbers we are seeing in #1867518: Leverage entityDisplay to provide fast rendering for fields mean this is critical too?

On criticality I think it depends on how much this helps with performance? Do we have a feeling ahead of time? Looks like #4 and the results of #1867518: Leverage entityDisplay to provide fast rendering for fields make this critical, no?

I think for now this is critical as even a 40% performance regression at this point is problematic ...

Tagging "blocker" per #2381277-20: Make Views use render caching and remove Views' own "output caching".

Discussed with @effulgentsia, @alexpott, and @webchick. Based on #4 and #8 and the critical performance criteria, this issue should definitely be critical. @effulgentsia pointed out that while we also have an issue to cache the full view render, there are many, many operations which will invalidate the cache for a whole view, resulting in cache misses, so it's still worth caching at the row level, especially given that we've introduced a significant performance regression.

Working on this

So after discussing this in IRC the outcome is:

Row based view caching is _very_ difficult to implement properly, because there are no rows per se, but all is rendered via the theme layer.

The best we can do is:

- Use field based render caching, store in StylePluginBase::rendered_fields
- Ensure instead of !isset() for renderedFields() a render-only-what-is-missing approach is used
- Optimize from working field based render caching to load the whole row and pre-populate rendered_fields with a cache key of 'view:name:display:row_1'
- Store at the end when rendering is finished by setting the cache for each newly rendered row.

I have a first proof-of-concept based on #16, but it is really ugly. BUT IT WORKS!

Views really needs #2466585: Decouple cache implementation from the renderer and expose as renderCache service OR at least public cacheSet / cacheGet functions on the renderer.

Trying to use #pre_render works, but is really ugly and need to set cache twice ...

Limitations are:

- All work done in preRender is still done. (Hence with the entity view display patch we need to do the cacheGet way earlier, but the cacheSet later).
- SafeMarkup needs to be set afterwards manually.
- cache->set() is called twice
- Way too much copied code ...
- The cache keys are not including $view name and display id, etc.

Explanation of some points:

- Metadata is pushed to the stack and always in the top entry of $data, this matches the previous behavior of pushing the data to stack when rendering the fields, so is not a problem (that is complicated ...)

Lets still see what breaks :).

Great start!

The attached patch adds some clean-up, implements cache keys and tags handling and refactors the EntityFieldRenderer introduced in #1867518: Leverage entityDisplay to provide fast rendering for fields to lazy render field values instead of pre-rendering them. The performance boost is huge: with warm caches response is ~55% faster with a table view with 50 items.

I think we could make this even faster if we had a way to a multiple cache get, as this would also allow us to inform the system of which rows need actually to be built, instead of building them all every time we have a cache miss.

This patch includes the last one of #1867518: Leverage entityDisplay to provide fast rendering for fields, attaching also a diff against that.

1. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -625,6 +628,41 @@ public function renderGrouping($records, $groupings = array(), $group_rendered =
   +    if (isset($elements['#cache']['keys'])) {
   +      $cid_parts = $elements['#cache']['keys'];
   +      if (!empty($elements['#cache']['contexts'])) {
   +        $contexts = \Drupal::service('cache_contexts')->convertTokensToKeys($elements['#cache']['contexts']);
   +        $cid_parts = array_merge($cid_parts, $contexts);
   +      }
   +      return implode(':', $cid_parts);
   +    }
   

   That sounds something which should have a general API ...

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +675,117 @@ protected function renderFields(array $result) {
   +            $expire = ($element['#cache']['max-age'] === Cache::PERMANENT) ? Cache::PERMANENT : (int) \Drupal::service('request_stack')->getMasterRequest()->server->get('REQUEST_TIME') + $element['#cache']['max-age'];
   

   its sad that we have to figure that out for ourselves.

3. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +675,117 @@ protected function renderFields(array $result) {
   +  protected function getRowCacheTags(ResultRow $row) {
   +    $tags = [];
   +    // FIXME: Handle revisions.
   +    /** @var \Drupal\Core\Entity\EntityInterface $entity */
   +    foreach (array_merge([$row->_entity], $row->_relationship_entities) as $entity) {
   +      $tags[] = $entity->getEntityTypeId() . ':' . $entity->id();
   +    }
   +    return $tags;
   +  }
   

   Alright, so a) we have to check whether there is _entity, its a false assumption that we always render one ... Don't we have to add the langcode of the translation for example as well? We also have to take into account the fact that we might have an aggregated view.

4. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +675,117 @@ protected function renderFields(array $result) {
   +    // Exclude the current index and entity data from the row data. Adding cache
   +    // tags to it ensures we have a unique row identifier.
   +    $row_data = array_diff_key((array) $row, array_flip(['index', '_entity', '_relationship_entities'])) + $this->getRowCacheTags($row);
   

   It is amazing that we can cast random php objects into arrays.

1. / 2. Yes, indeed: #2466585: Decouple cache implementation from the renderer and expose as renderCache service is the issue for that. Once #2469277: Changing #cache keys during #pre_render or anywhere else leads to cache redirect corruption lands I propose to just make the cacheSet / cacheGet methods public on the renderer in this issue as a proof-of-concept and convert to using that ...

(Partially) addressed #22.3, I'm not sure we need special care for aggregation since the related fields will end up in the row data automatically.

Reuploading...

Updated numbers:

• we have an overhead of ~6% on the inclusive wall time with cold caches (50 cache misses, 400 rendered fields, 8 fields per row), ~4% more memory used
• by using field lazy-building instead of pre-building, with warm caches (50 hits) we have ~68% gain on the inclusive wall time and ~8% less memory used

@plach
It would be nice if you could post reviewable patches

+++ b/core/modules/views/src/Entity/Render/EntityTranslationRendererBase.php
@@ -0,0 +1,64 @@
+    $entity_id = $row->_entity->id();
+    return $this->build[$entity_id];

Note: We have an open issue that we don't support relationships here.

@dawehner:

What do you mean? In #25 I posted https://www.drupal.org/files/issues/cache_field_views_row-2450897-25.txt which includes only the changes introduced here.

Note: We have an open issue that we don't support relationships here.

If you are referring to #2446681: Error "Column 'langcode' in field list is ambiguous" thrown due to TranslationLanguageRenderer not rendering a field from a relationship, that seems to be fixed in HEAD now.

Oh, I'm sorry. Great work!

#2457999: Cannot use relationship for rendered entity on Views is the issue I tried to refer to.

1. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +677,134 @@ protected function renderFields(array $result) {
   +          // FIXME: Hack: Use render_cache ::cacheGet / ::cacheSet instead.
   +          $element = [
   +            '#pre_render' => [
   +              [$this, 'preRenderRow'],
   +            ],
   +            '#row' => $row,
   +            '#cache' => [
   +              'tags' => $this->getRowCacheTags($row),
   +              'keys' => $this->getRowCacheKeys($row),
   +            ],
   +          ];
   +          // @todo Should use renderPlain, but that was broken before as fields
   +          //   are rendered to strings, so no need to fix it.
   +          $renderer->render($element);
   +          $data = $renderer->getCacheableRenderArray($element);
   ...
   +            // Unset the big markup.
   +            $data['#markup'] = '';
   

   Here is a naive question: Why can't we call out to $this->preRenderRow() directly? It seems to be a layer of indirection which is not obvious, why we use it.

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +677,134 @@ protected function renderFields(array $result) {
   +      md5(serialize($row_data)),
   

   You are right about the fact that we don't have to care about aggregate itself. Note: Can we use sha256, otherwise some people will complain about the usage of md5 again, I think its just not worth to start some fight here again :(

Just a short note that this is _double_ critical:

We don't have any filter caches anymore in favor of render cache, but that means that views currently do not have any text filter caches at all ...

Here is a naive question: Why can't we call out to $this->preRenderRow() directly? It seems to be a layer of indirection which is not obvious, why we use it.

That way it is not called if render cache is available. However that code will go away once #2466585: Decouple cache implementation from the renderer and expose as renderCache service is done.

Can we use sha256, otherwise some people will complain about the usage of md5 again, I think its just not worth to start some fight here again :(

sha256 will introduce very long cids, would it make sense to make the hashing function configurable? VRC has a variable for that.

#35: I think hash('sha256') is fine. We use automatic cid stripping to a hash also in the cache system now and I think we also allow 255 chars. Let's check.

If someone can RTBC #2469277: Changing #cache keys during #pre_render or anywhere else leads to cache redirect corruption or ping Wim to RTBC it, we can make progress on the renderCache service and hack it here by making the render thing public as a hack instead.

#35
Yeah, that really sounds like quite a micro-optimisation.

I won't work on this until tonight...

Spoke with plach:

- Test failures related to admin links we can ignore and just disable VRC on paths that are admin routes (same as smartcache)

We already have 2 critical issues open for such links:

* https://www.drupal.org/node/2351015
* https://www.drupal.org/node/2335661

but that is a general problem and can be easily created in HEAD by putting any admin content listing into a views block, then caching that block => BOOM

--

Therefore those should not block VRC and a route check should be fine for now.

I'm looking at those test failures in the meantime.

Some test fixes.

While working on it, we had to open #2473873: Views entity operations lack cacheability support, resulting in incorrect dropbuttons

Patch didn't apply. So rerolled.

Some fixes to reduce test failures.

Rerolled on top of the latest code from #1867518: Leverage entityDisplay to provide fast rendering for fields and performed some clean-up.

1. +++ b/core/modules/comment/src/Tests/Views/CommentUserNameTest.php
   @@ -152,6 +152,8 @@ public function testUsername() {
   +    $executable->storage->invalidateCaches();
   
   +++ b/core/modules/dblog/src/Tests/Views/ViewsIntegrationTest.php
   @@ -100,6 +100,7 @@ public function testIntegration() {
   +    $view->storage->invalidateCaches();
   
   +++ b/core/modules/views/src/Entity/View.php
   @@ -333,6 +333,7 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +    $this->invalidateCaches();
   
   +++ b/core/modules/views/src/Tests/Handler/FieldCounterTest.php
   @@ -57,6 +57,7 @@ function testSimple() {
   +    $view->storage->invalidateCaches();
   

   Why is this necessary?

2. +++ b/core/modules/views/src/Entity/View.php
   @@ -448,4 +449,16 @@ public function __sleep() {
   +    \Drupal::service('cache_tags.invalidator')->invalidateTags($tags);
   

   Why not just call Cache::invalidateTags()?

Why is this necessary?

Well, many of them are needed because we manipulate the view and execute/render it again ...

Why not just call Cache::invalidateTags()?

Well, I personally don't like those hiding wrappers

Wow, amazing how small this patch now is when one thinks all the renderer / renderCache overhead removed.

Just a reroll for now

This should fix some more test failures.

Done, for now

I asked Fabian to have a look to ContactLinkTest to check that my fix is correct and render cache is working as expected. Here is my test db, the test view can be found at /test-contact-link, you just need to display the view with the various admin/contact/test users and check render cache entries are the expected one.

1. +++ b/core/modules/contact/src/Plugin/views/field/ContactLink.php
   @@ -114,27 +113,22 @@ protected function renderLink(EntityInterface $entity, ResultRow $values) {
   -    $build = [];
   -    BubbleableMetadata::createFromObject($access_result)->applyTo($build);
   -    if (!$access_result->isAllowed()) {
   -      return $build;
   

   This should be correct ...

   What is wrong about this?

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -783,14 +784,16 @@ protected function getRowCacheTags(ResultRow $row) {
   -  protected function getRowCacheKeys(ResultRow $row) {
   +  protected function getRowCacheKeys(ResultRow $row, $tags) {
        // Exclude the current index and entity data from the row data. Adding cache
        // tags to it ensures we have a unique row identifier.
   -    $row_data = array_diff_key((array) $row, array_flip(['index', '_entity', '_relationship_entities'])) + $this->getRowCacheTags($row);
   +    $row_data = array_diff_key((array) $row, array_flip(['index', '_entity', '_relationship_entities'])) + $tags;
   

   This is probably wrong.

   Cache tags are for invalidation and not for variation.

   Cache contexts need to be used for more invalidation and is probably what is missing here ...

   What I meant here was:

   "Cache contexts need to be used for more variations and is probably what is missing here ..."

This should be correct ...

What is wrong about this?

I just reverted those changes, they were breaking tests even more. Now we have what's in HEAD.

This is probably wrong.

Cache tags are for invalidation and not for variation.

Cache contexts need to be used for more invalidation and is probably what is missing here ...

It was just a quick and dirty way to add entity ids in the row data, I will find a cleaner way.

1. +++ b/core/modules/user/src/UserData.php
   @@ -99,6 +101,7 @@ public function set($module, $uid, $name, $value) {
   +    Cache::invalidateTags(User::load($uid)->getCacheTags());
   

   Woah. Why is this only necessary for the set() case? Why not also for the delete() case?

   Also: WTF! I had no idea this existed. Looks like this is the last remnant from before Users were fieldable Entities.

   We're definitely missing test coverage in this area then.

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +677,138 @@ protected function renderFields(array $result) {
   +          // FIXME: Hack: Use render_cache ::cacheGet / ::cacheSet instead.
   +          $element = [
   +            '#pre_render' => [
   +              [$this, 'preRenderRow'],
   +            ],
   +            '#row' => $row,
   +            '#cache' => [
   +              'tags' => $tags,
   +              'keys' => $this->getRowCacheKeys($row, $tags),
   +            ],
   +          ];
   +          // @todo Should use renderPlain, but that was broken before as fields
   +          //   are rendered to strings, so no need to fix it.
   +          $renderer->render($element);
   +          $data = $renderer->getCacheableRenderArray($element);
   +
   +          // This will set the row into cache, but as raw #markup, but we need
   +          // the individual fields. Fortunately we can detect that case and
   +          // update the cache entry :).
   +          if (!empty($element['#pre_rendered_row'])) {
   +            foreach ($keys as $id) {
   +              $data[$id] = $element[$id]['#markup'];
   +            }
   +
   +            // RendererInterface::getCacheableRenderArray() does not preserve
   +            // cache keys.
   +            $data['#cache']['keys'] = $element['#cache']['keys'];
   +
   +            // Unset the big markup.
   +            $data['#markup'] = '';
   +
   +            // FIXME: Copied from renderer::cacheSet.
   +            $cid = $this->createCacheID($data);
   +            $bin = isset($element['#cache']['bin']) ? $element['#cache']['bin'] : 'render';
   +            $expire = ($element['#cache']['max-age'] === Cache::PERMANENT) ? Cache::PERMANENT : (int) \Drupal::service('request_stack')->getMasterRequest()->server->get('REQUEST_TIME') + $element['#cache']['max-age'];
   +            $cache = \Drupal::service('cache_factory')->get($bin);
   +
   +            $view_cache_tag = 'config:' . $this->view->storage->getConfigDependencyName();
   +            $cache->set($cid, $data, $expire, Cache::mergeTags($data['#cache']['tags'], ['rendered', $view_cache_tag, $view_cache_tag . ':' . $this->displayHandler->display['id']]));
              }
   +          else {
   +            // This is exactly what we stored into cache earlier.
   +            $data = $element;
   +
   +            // Restore SafeMarkup.
   +            foreach ($keys as $id) {
   +              $data[$id] = SafeMarkup::set($data[$id]);
   +            }
   +          }
   

   I think we need a very thorough, as-clear-as-possible explanation for why this needs to implement its own render caching.

#62

1: Yep, looks like it, good catch by plach.

2)

It is pretty simple.

Because how views works, there is never something that is operating on a whole row, but on fields.

What we do is:

We store the whole row with the whole row cache metadata, but keep the output of the individual fields, which are rendered to a string.

A cached row render array looks for example like this:

  $cached = [
    '#cache' =>[
       'keys' => [...],
       'tags' => [ 'entity:1', 'view_mode:full', ... ],
       'contexts' => ['user.permissions', 'languages:language_content'],
    ],
   '#attached' => [...],
    '#post_render_cache' => [...],
    '#markup' => '', // Yes, that is empty!
    'field_1' => 'foo',
    'field_2' => 'bar',
    // ...
  ];

Therefore we create the exact array again that is needed by views in the next steps.

This is 100% comparable to how entity render caching works (it also caches the whole entity with all fields output).

We cache the whole build here as well, just that in this case we need the individual fields output for future usage, by e.g. the table plugin.

This should address the reviews above and fix the last failures. Now we just need to clean-up our code once #2466585: Decouple cache implementation from the renderer and expose as renderCache service lands and provide some additional test coverage since most Views tests are running with cold caches. Manual testing results are encouraging though, and the ContactLinkTest is actually testing the new code and is green. The bot patch includes #1867518: Leverage entityDisplay to provide fast rendering for fields as usual.

Not sure why we lost the first two hunk in the interdiff, probably in #47. This demonstrates we need specific test coverage.

As a bonus performing some clean-up (last two hunks).

This reverts the UserData fix: the UserData service is normally used by User::postSave() / User::postDelete(), hence we would be invalidating tags twice for each user write. This is in internal implementation that should not be used as a public API, hence I think it's fine to just fix the test instead.

+++ b/core/modules/contact/src/Tests/Views/ContactLinkTest.php
@@ -83,6 +84,7 @@ public function testContactLink() {
     $this->userData->set('contact', $no_contact_account->id(), 'enabled', FALSE);
+    Cache::invalidateTags($no_contact_account->getCacheTags());

So you're saying that this first line is abusing an internal API, in a way that no contrib module ever would (they would modify the data on the User entity and call ->save() on that User entity)?

If so, then I agree that the added invalidation line there is acceptable.

This review was for an earlier patch (#65), but I did not see that it conflicted, hence re-posting now.

1. +++ b/core/modules/views/src/Entity/View.php
   @@ -448,4 +449,16 @@ public function __sleep() {
   +    foreach (array_keys($this->display) as $display_id) {
   +      $tags[] = 'config:' . $this->getConfigDependencyName() . ':' . $display_id;
   +    }
   

   Why do we need to calculate that separately?

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1095,6 +1095,17 @@ public function adminSummary() {
   +  public function processRowData(array& $data, ResultRow $row) {
   +  }
   

   Lets call that something like:

   alterRowCacheHash/Keys/Data?

   That is else really confusing ...

Besides those two remarks, this looks great as usual!

This should address #69 and #70.

I also separated the code computing row identifiers to its own method, as I think that will be useful to compute cache keys for the full view render cache.

So you're saying that this first line is abusing an internal API, in a way that no contrib module ever would (they would modify the data on the User entity and call ->save() on that User entity)?
If so, then I agree that the added invalidation line there is acceptable.

Actually I realized only a couple of user data properties are handled in User::preSave(), so this service is supposed to be used as a public API. Created #2477903: User data service does not take cache tag invalidation into account to work on a proper fix.

Let's try to remove displays altogether

I tried to go ahead and apply #2466585: Decouple cache implementation from the renderer and expose as renderCache service but I have some trouble using the new API and make things work. I had to perform some adjustments. Attaching my current code, it would be good to have some feedback before doing more work.

Okay, you will still need to use the #pre_render pattern, but that is only due to how views work.

The code needs to look something like:

$build = [...]; // Like before, but omit the cache keys.

$cache_array = '[
  '#cache' => [
    'keys' => $this->getCacheKeys(),
  ],
]';

$data = $render_cache->get($cache_array);

if (!$data) {
  $renderer->renderPlain($build)
  $data = $render_cache->getCacheableRenderArray($build);
  // Do the same as before to set the individual values from $build, but use '#markup' => ..., for each child key.
  $data['#cache']['keys'] = $cache_array['#cache']['keys'];
  $render_cache->set($data, $cache_array);
} else {
  // Render the data to mark strings as safe and push cacheable metadata to the stack.
  $markup = $renderer->render($data);
}

// Now copy just the individual strings out like before from '#markup'.

I can provide a working implementation without problems - if you want.

#2466585: Decouple cache implementation from the renderer and expose as renderCache service landed, this can now be rerolled on top of that.

This should address #75-#76 plus some clean-up to make it easier to get row identifiers, which may turn useful in #2381277: Make Views use render caching and remove Views' own "output caching".

There is still some fix needed as RendererCacheInterface::set() does not preserve any actual render array value, hence there is no way to store field markup. For now I used an ugly hack, but I hope there is a cleaner solution.

Finally, yesterday I started implementing test coverage. The bad news is that we still have some broken stuff, the good one is that if cacheability metadata is correctly specified things seem to be working properly also in fairly complex use cases.

I restored the changes Daniel did to the ContactLink field handler and it seems to work more or less correctly. There is still one problem, although the current contact link test is not exposing it: ContactPageAccess specifies different cache granularity depending on the execution context, for instance users are not allowed to access their own contact page. If you list all users with a view displaying only a contact link (not very sensible) every user should see her link missing and all the other available (provided she has access to contact forms). However this not always work because an item may be cached with a user.permission cache context, even if the current user would require user cache context.

Probably it would be better to skip all this mess and just use placeholders, like we re doing with node links, for all field handlers requiring per-user granularity.

1. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -694,60 +705,57 @@ protected function renderFields(array $result) {
   +            // We do not need the full markup, just individual fields.
   +            // FIXME RendererCacheInterface::set() does not preserve any actual
   +            //   render array value so we need to store field markup also in the
   +            //   '#markup' property to have it available successively.
   +            $data['#markup'] = [];
                foreach ($keys as $id) {
   -              $data[$id] = $element[$id]['#markup'];
   +              $data[$id]['#markup'] = $build[$id]['#markup'];
   +              $data['#markup'][$id]['#markup'] = $build[$id]['#markup'];
                }
   

   So this is why you said:

   There still some fix needed as RendererCacheInterface::set() does not preserve any actual render array value, hence there is now way to store field markup. For now I used an ugly hack, but I hope there is a cleaner solution.

   But RenderCache::set() doesn't modify the $elements it is given anywhere. So… I don't understand what is being lost/not preserved.

   Can you explain this a bit more?

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -694,60 +705,57 @@ protected function renderFields(array $result) {
   +            // RendererCacheInterface::getCacheableRenderArray() does not
   

   Here and elsewhere: s/RendererCacheInterface/RenderCacheInterface/

3. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -694,60 +705,57 @@ protected function renderFields(array $result) {
   +            $data += $data['#markup'];
   

   Typewise, this is:

   array += string;
   

   How can this work? :)

4. . There is still one problem, although the current contact link test is not exposing it: ContactPageAccess specifies different cache granularity depending on the execution context, for instance users are not allowed to access their own contact page. If you list all users with a view displaying only a contact link (not very sensible) every user should see her link missing and all the other available (provided she has access to contact forms). However this not always work because an item may be cached with a user.permission cache context, even if the current user would require user cache context.

   That sounds like a bug in ContactPageAccess, or this patch not correctly supporting cache redirection (see the super long comment in RenderCache::set() regarding cache redirection).

   Upon further thought, I think I see what you mean. Let me describe the situation that I think more clearly explains it, then you can say this is indeed the problem you're describing or not:

   • We have 6 registered users: A, B, C, D, E, F.
   • We log in as user F.
   • We visit a view that lists users, along with links to their contact page.
   • That view lists 5 items per page, and we're on the first page. Thus we (as user F) see all users except F: users A, B, C, D, E are listed.
   • All 5 "contact" links have the cache context user.permissions. They are render cached (row cached, thanks to this patch).
   • Now log in as user A, B, C, D or E. Due to render caching, we'll be able to see the contact link to ourselves, because all links have user.permissions as their cache context, not user.

   In other words:

       // Users may not contact themselves.
       if ($account->id() == $contact_account->id()) {
         return AccessResult::forbidden()->cachePerUser();
       }
   

   actually needs to be executed for every user, but is not in this case.

   That is a great, great, great find!

   I wonder how big of a general problem this is. At first sight, it looks like in cases where an access result may be per-user, it always needs to be per-user, because otherwise this exact problem can always happen. Fortunately, there are only about a dozen ::cachePerUser() calls.

1/3: RenderCache::set() does:

$data = $this->getCacheableRenderArray($elements);

and caches $data (after some massaging), which means only $data['#markup'] is preserved between different requests (aside from cacheable metadata). The current code is storing and array instead of a string in $data['#markup'] (aw) in order to preserve individual field markup.

4: You are right, with the exception that since we are talking about fields, we just have an empty field and not an entire row go missing. Hence you can simplify the example and not introduce paging.

I wonder how big of a general problem this is. At first sight, it looks like in cases where an access result may be per-user, it always needs to be per-user, because otherwise this exact problem can always happen. Fortunately, there are only about a dozen ::cachePerUser() calls.

Yep, I came to the same conclusion. If this is true it needs to be documented very clearly.

As I said, for the views field case I think using post_render_cache placeholders as suggested by Fabian would be easier, and would also keep down the number of cache entries.

1. Hmmm… this is unchanged compared to HEAD prior to #246658: link to bulk delete UI, so it must've been a pre-existing problem, that is not caught by any of our tests.
   But I'm still not quite sure what you mean. Yes, only the following keys are preserved in the render cache: '#markup', '#cache', '#attached' and '#post_render_cache'. This is very much intentional:
   1. it is the final rendered markup that we care about ('#markup'), plus its bubbleable metadata (everything except '#markup')
   2. we don't need anything else, so we omit all other keys. This ensures we don't serialize huge objects, and thus ensures we only cache the truly necessary data: the rendered mark-up (that we don't have to re-render) plus the bubbleable metadata (that we still have to bubble).

   Having said all that and re-read the relevant part of the patch/interdiff, I think I now understand your problem: you want to cache "per row", but you want to be able to pull out individual fields within a row. The Render API only caches at the levels (the subtrees) where you specify [#cache][keys]. Since you only specify it at the row level, it only caches at the row level, and indeed the fields within the row won't be individually retrievable. That's why you apply that … interesting hack :) (i.e. $data['#markup'][$id]['#markup'] = $build[$id]['#markup'];) If you'd instead render cache per field per row, then the problem goes away.
   Is there a reason you cannot do that?

   P.S.: AFAICT you only replied to point 1, not to points 2/3. I'm particularly interested in your explanation for point 3. :)

4. As I said, for the views field case I think using post_render_cache placeholders as suggested by Fabian would be easier, and would also keep down the number of cache entries.

   This is where the "auto-placeholdering" mentioned in #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts comes in: as a developer, you wouldn't have to deal with the burden of creating a #post_render_cache callback, generating a placeholder, etc. anymore. The Render API would do that for you. The Render API would see that you're using a "high-cardinality" cache context (i.e. "per user" caching causes many variations to be created — see #2469431-49: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts and the IS there) and automatically determine do the placeholder/#post_render_cache stuff for you.

1: We cannot cache per-row markup because there is no row markup: to preserve the current rendering pipeline we need to store individual field markup, since most style templates use that to assemble the row output.
2: Will fix it
3: $data['#markup'] is an array in that case due to the ugly hack explained above
4: Sounds great, so we just need to ensure we always specify the most granular cache context

If you'd instead render cache per field per row, then the problem goes away. Is there a reason you cannot do that?

Sorry, I misread your comment. We agreed to go this way before the first patch, my personal motivations were: reducing the number of cache entries and make sure we cache fields after they have gone through advanced rendering. Not sure whether Fabian or Daniel had additional motivations.

I see. Would be good to hear from Fabian & Daniel then :) I'd think #2453945: Use multiple get for #pre_render / #cache where possible would solve the I/O problem.

Yes, I missed that we only preserve #markup, not sure we should be stuffing an array in there. I usually mis-used [#attached]['my-extra-data'] for that :p.

- A cache get multiple is still slower than a single call, we also want to do a cache_get_multiple() of all fields for all rows in the end.

Also:

- To do this properly we need to add a #pre_render per views ->theme call, which is possible but it gets way more complicated then.

I would vote to make the 'hack' as clean as possible instead. Caching on the field level does not feel like the right granularity for me.

A review:

1. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +681,111 @@ protected function renderFields(array $result) {
   +            $build = $cache_array + [
   +              '#pre_render' => [[$this, 'preRenderRow']],
   +              '#row' => $row,
   +            ];
   

   The adding of $cache_array is wrong here as that will have renderPlain() call the cache_set already.

   '#keys' should not be set here.

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +681,111 @@ protected function renderFields(array $result) {
   +            $renderer->renderPlain($build);
   

   I know I wrote that, but likely renderPlain() with how it is structured now is wrong.

   renderPlain means:

   render in isolation and do not update the stack, but in this case the meta data is never rendered to the stack later.

   I thought the else block afterwards was always executed, in which case using $data = $render_cache->getCacheableRenderArray() is right.

   Still thinking ...

   Can use renderer->render() for now.

3. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +681,111 @@ protected function renderFields(array $result) {
   +            $data = $render_cache->getCacheableRenderArray($build);
   

   This is not needed and hte code can be simplified, you can just override $build['#markup'] or (mis-)use $build['#attached']['vrc-row-data'].

Sorry for the confusion ... still thinking of a good API to support that.

This should address #78.2 and #87. We still have a couple of issues, both caused by the fact that now we don't set cache keys in the render array on cache misses, to avoid storing cache items twice:

• Required cache contexts are not initialized, hence the CID would not include them, which is bad. Atm, the patch just retrieves them by the container directly, but the proper fix would be adding an helper method to the Renderer (or render cache), something like ::createCacheableElement().
• Pre-bubbling CIDs are not computed, which prevents cache redirects from working. In this specific case this seems to have only a positive side-effect, because this way the contact test view issue described in #77/#78 goes away. In fact the "primary" CID already contains the additional cache context, so no "collision" happens when rendering the view with different users.
  We get only these CIDs:

  views:fields:test_contact_link:page_1:1edd69733a7a9db4d734b2ed1e9db043149dc5f21201cb840d99d3b36a3ebc24:en:bartik:ph.6d5a93fc5d9be38c332d74128f4b216744f93d07a4235c3b631c8f847375deed
  views:fields:test_contact_link:page_1:1edd69733a7a9db4d734b2ed1e9db043149dc5f21201cb840d99d3b36a3ebc24:en:bartik:u.1
  

  while formerly we had:

  views:fields:test_contact_link:page_1:1edd69733a7a9db4d734b2ed1e9db043149dc5f21201cb840d99d3b36a3ebc24:en:bartik ->
  views:fields:test_contact_link:page_1:1edd69733a7a9db4d734b2ed1e9db043149dc5f21201cb840d99d3b36a3ebc24:en:bartik:ph.6d5a93fc5d9be38c332d74128f4b216744f93d07a4235c3b631c8f847375deed (or)
  views:fields:test_contact_link:page_1:1edd69733a7a9db4d734b2ed1e9db043149dc5f21201cb840d99d3b36a3ebc24:en:bartik:u.1
  

Working on some additional coverage now, it would be good to have some feedback on this meanwhile.

1. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +665,113 @@ protected function renderFields(array $result) {
   +            $data += $data['#attached']['views_rendered_fields'];
   +            unset($data['#attached']['views_rendered_fields']);
   

   Regarding the unsetting which existed now and in previous versions ... Its fine to add it, given that $this->rendered_fields itself will have the stored values still.

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +665,113 @@ protected function renderFields(array $result) {
   +          $id = end($keys);
   +          $this->rowTokens[$index] = $this->view->field[$id]->getRenderTokens(array());
   

   That bit is a bit odd, can we at least explain why we are doing the end() call here?

3. +++ b/core/modules/views/src/ResultRow.php
   @@ -53,4 +55,41 @@ public function resetEntityData() {
   +  /**
   +   * Returns the row cache tags.
   +   *
   +   * @return string[]
   +   *   The row cache tags.
   +   */
   +  public function getCacheTags() {
   +    $tags = !empty($this->_entity) ? $this->_entity->getCacheTags() : [];
   +
   +    if (!empty($this->_relationship_entities)) {
   +      foreach ($this->_relationship_entities as $entity) {
   +        $tags = Cache::mergeTags($tags, $entity->getCacheTags());
   +      }
   +    }
   +
   +    return $tags;
   +  }
   

   Why do we need this logic again? Note: \Drupal\views\Plugin\views\query\Sql::getCacheTags as part of that logic already, but it will be adapted in #2477157: rest_export Views display plugin does not set necessary cache metadata. I am not sure whether adding actual logic to that object is the best idea.

4. +++ b/core/modules/views/src/ResultRow.php
   @@ -53,4 +55,41 @@ public function resetEntityData() {
   +  public function getId() {
   

   Yeah especially this method on the ResultRow feels a bit icky, but we have helper methods which produce those values? Let's ask damiankloip for an opinion.

1. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -689,47 +691,49 @@ protected function renderFields(array $result) {
   +              // FIXME This logic should be encapsulated in the render cache
   +              //   service.
   +              'contexts' => \Drupal::getContainer()->getParameter('renderer.config')['required_cache_contexts'],
   

   Great catch!

   Note that it must continue to live in Renderer where it does live today; it just should *also* be done in RenderCache.

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -689,47 +691,49 @@ protected function renderFields(array $result) {
   +            $data['#cache'] = [
   +              'keys' => $cache_array['#cache']['keys'],
   +              'contexts' => Cache::mergeContexts($data['#cache']['contexts'], $cache_array['#cache']['contexts']),
   +              'tags' => Cache::mergeTags($data['#cache']['tags'], $row->getCacheTags()),
   +            ];
   

   Why not let
   ResultRow implement CacheableDependencyInterface, then rename ::getId() to ::getCacheKeys() and merge the logic of StylePluginBase:: getRowCacheKeys() into ResultRow::getCacheKeys() too?

   Then all this can be simplified to:

   $data['#cache'] += #cache_array['#cache'];
   $renderer->addCacheableDependency($data, $row);
   

   Probably I'm missing something though.

@dawehner, #90:

1: It's not only fine, it's required otherwise we get a logic exception because the system tries to process that data as <head> markup. Added a comment.
2: It's the same in HEAD, added a comment here too. Actually I realized we can/need to cache also render tokens, because they rely on field handler data which is populated only when actually rendering field markup.
3: It's not the same logic as in HEAD: here we collect only the entities for a single row. We could certainly refactor it to allow it to be used by the Sql class.
4: The reason why I moved this code on the ResultRow class is that we might need it to compute the full view CID in #2381277: Make Views use render caching and remove Views' own "output caching". This would allow us to avoid instantiating/depending on the style plugin for a logic that actually concerns only the row data. This looks like a way cleaner approach to me, actually.

@Wim Leers, #91:

1: Thanks, done :)
2: Well, as explained above row IDs are a more generic concept than cache keys, although they are close. They can be used to compute cache keys for a single row cache entry, but also for the whole displayed result set, which should allow us to cache the full view. Well, it's not that simple, but this is the core idea.

I started writing some test coverage (not part of this yet), however it seems the most meaningful tests would involve link fields, which are broken currently (see #38). I spoke with @catch in IRC and we agreed that, if it's not a huge effort, we should try to fix them here, as we did for the contact link handler, to avoid introducing regressions.

Re-uploading patch #92 for the bot, sorry for the noise.

It's not the same logic as in HEAD: here we collect only the entities for a single row.

Sure, but still, you know what I meant and what I did not meant :)

4: The reason why I moved this code on the ResultRow class is that we might need it to compute the full view CID in #2381277: [PP-1] Make Views use render caching. This would allow us to avoid instantiating/depending on the style plugin for a logic that actually concerns only the row data. This looks like a way cleaner approach to me, actually.

While getCacheTags is probably fine on there, I think the ID should potentially really be somewhere else, why, because its a detail for caching views rows IMHO,
who knows whether someone else comes up with some other idea in contrib. The style plugin at least now is swappable, but the row plugin by design is made as a simple object,
adding sort of logic onto it, removes that capability. You know, the cache plugin would be a great place for that, given that this method is certainly in the caching domain.

1. +++ b/core/modules/views/src/Entity/View.php
   @@ -448,4 +449,13 @@ public function __sleep() {
   +    // Invalidate cache tags for cached rows.
   +    $tags = $this->getCacheTags();
   

   I'm curious whether we should fetch the information from the cache plugin instead?

2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -112,6 +112,13 @@
      /**
   +   * Stores the render API render cache.
   +   *
   +   * @var \Drupal\Core\Render\RenderCacheInterface
   +   */
   +  protected $renderCache;
   

   How many plugins need it? Style and Field? which are kinda 20% or something like that, well, I guess being pragmatic here is okay.

3. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -626,6 +629,29 @@ public function renderGrouping($records, $groupings = array(), $group_rendered =
   +  public function preRenderRow(array $build) {
   

   That method name is a bit confusing, given that views already has a concept of preRender() but preRenderRow comes way after that, well

4. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -626,6 +629,29 @@ public function renderGrouping($records, $groupings = array(), $group_rendered =
   @@ -637,27 +663,116 @@ protected function renderFields(array $result) {
   
   @@ -637,27 +663,116 @@ protected function renderFields(array $result) {
        }
   

   Its odd that there is no way to opt out of that row caching, is that really what we want?

5. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +663,116 @@ protected function renderFields(array $result) {
   +          array_walk($keys, function($id) use (&$rendered_fields, $data) {
   

   quick style question, do we typehint against arrays in anonymous functions?

This should address #95, except for:

4: Discussed this with Daniel: although normally we don't let people opt-out from render caching, since with Views it's very easy to expose sensible data, a possible solution could be to make the Tags cache plugin the default one, and make None skip render caching. I think we still need to clear up what's the relationship between render caching and cache plugins: at least for output there is potential for confusion, especially when we'll start working on #2381277: Make Views use render caching and remove Views' own "output caching".

5: I believe we cannot type-hint used variables, they are actual variables not parameters.

5: I believe we cannot type-hint used variables, they are actual variables not parameters.

OH you are absolutely right.

@Fabian, Wim:

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -596,4 +603,17 @@ protected function getRenderer() {
+  protected function getRenderCache() {
+    if (!isset($this->renderCache)) {
+      $this->renderCache = \Drupal::service('render_cache');
+    }
+    return $this->renderCache;
+  }
+

I'm wondering whether it would make sense to move this method on the Renderer itself.

Talked with @damiankloip and he agreed that putting stuff onto ResultRow just doesn't make sense.
Please always keep in mind that the code has to be sort of maintainable so putting cache related stuff into everything makes things worse.

This fixes cache redirects as items were actually not picked form cache. Contact link access has been fixed to always return user cacheability for the access result, as suggested by Wim.

This also adds cacheable properties preservation, as suggested by Fabian, and starts fixing other field link handlers, as discussed with Daniel and Nat.

Re-uploading, bot seems to be stuck

1. +++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
   index 78057cc..6a64dc8 100644
   --- a/core/modules/user/src/Plugin/views/field/Link.php
   
   +++ b/core/modules/user/src/Plugin/views/field/Link.php
   index b29c426..8cb214a 100644
   --- a/core/modules/user/src/Plugin/views/field/LinkCancel.php
   

   I'm curious whether we could not change those handlers here, given that they will be generalised in #2322949: Implement generic entity link view field handlers ?

2. +++ b/core/modules/views/tests/modules/views_test_render_cache/views_test_render_cache.info.yml
   index 0000000..f6b99f0
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/views/tests/modules/views_test_render_cache/views_test_render_cache.module
   
   +++ b/core/modules/views/tests/modules/views_test_render_cache/views_test_render_cache.module
   +++ b/core/modules/views/tests/modules/views_test_render_cache/views_test_render_cache.module
   @@ -0,0 +1,7 @@
   
   @@ -0,0 +1,7 @@
   +<?php
   +
   +/**
   + * @file
   + * Contains the Views Test Render Cache module.
   + */
   +
   

   I thought we don't need empty module files any longer.

1: Well, that would certainly make sense, but it would mean postponing this once again, unless we commit this and break HEAD.
2: It wasn't meant to be in the patch: they are part of another branch where I started to work on test coverage.

Spoke with @dawehner and @catch: this is now postponed on #2322949: Implement generic entity link view field handlers (besides #1867518: Leverage entityDisplay to provide fast rendering for fields), as it will make providing cacheability metadata for links way easier.

#1867518: Leverage entityDisplay to provide fast rendering for fields is in, so just one issue remaining.

Rerolled this on top of #2322949: Implement generic entity link view field handlers. The test coverage added there in FieldEntityLinkTest is exactly the test coverage we were missing here, thus we should be good now.

The only issue that might use some more discussion is the one exposed by the Contact link tests and outlined in #77 / #78 / #79. If that's a real concern and the fix introduced here on ContactPageAccess is correct, we need at very least a follow-up to make sure access cacheability is defined correctly everywhere.

#2322949: Implement generic entity link view field handlers is in now!

Rerolled.

Working on the Views counter field handler, which should be uncacheable (+ test coverage).

Went through all of the field handers.
Here is a list of handlers which are potentially problematic:

• \Drupal\system\Plugin\views\field\BulkForm::render ... isn't it a similar problem as the views counter one, ... we can't cache when the row index is there? This sounds like a problem we have to adress
• \Drupal\contextual\Plugin\views\field\ContextualLinks::render ... don't we have to somehow take access checking, which is part of \Drupal\views\Plugin\views\field\LinkBase::render into account? Isn't that part of the cacheablity metadata. Maybe there is magic involved I don't get yet.
• \Drupal\history\Plugin\views\field\HistoryUserTimestamp::render is problematic, as its per user, right? I guess we could come up with something based upon javascript rendering?

PS: Its pretty epic how many handlers we have been able to remove due to all this effort in the context of views <-> entity integration.

1. +++ b/core/core.services.yml
   @@ -1358,7 +1358,7 @@ services:
   +    arguments: ['@request_stack', '@cache_factory', '@cache_contexts_manager', '%renderer.config%']
   

   Yeah, container parameters! Can we add it to the top of the container.services.yml file?

2. +++ b/core/core.services.yml
   @@ -1358,7 +1358,7 @@ services:
   +    arguments: ['@request_stack', '@cache_factory', '@cache_contexts_manager', '%renderer.config%']
   
   +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -39,6 +40,13 @@ class RenderCache implements RenderCacheInterface {
      /**
   +   * The renderer configuration array.
   +   *
   +   * @var array
   +   */
   +  protected $rendererConfig;
   +
   

   Can we document what is in there? It would be also great to document why we have required_cache_contexts being configurable. That seems not obvious, to be honest.

3. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -47,11 +55,31 @@ class RenderCache implements RenderCacheInterface {
   +    CacheableMetadata::createFromRenderArray($build)
   +      ->applyTo($build);
   

   So wait, can we document why we have this call here? It seems to be all we do is to fill out the default values of $build?

4. +++ b/core/modules/views/src/ResultRow.php
   @@ -7,6 +7,8 @@
   +use Drupal\Core\Cache\Cache;
   +
    /**
   

   Not needed anymore

+++ b/core/modules/comment/comment.routing.yml
@@ -57,7 +57,7 @@ comment.reply:
-    _access: 'TRUE'
+    _permission: 'post comments'

+++ b/core/modules/comment/src/Tests/CommentAnonymousTest.php
@@ -138,9 +138,7 @@ function testAnonymous() {
-    $this->assertText('You are not authorized to post comments', 'Error attempting to post comment.');
-    $this->assertNoFieldByName('subject[0][value]', '', 'Subject field not found.');
-    $this->assertNoFieldByName('comment_body[0][value]', '', 'Comment field not found.');
+    $this->assertResponse(403, 'Error attempting to post comment.');

+++ b/core/modules/comment/src/Tests/CommentNonNodeTest.php
@@ -344,9 +344,7 @@ function testCommentFunctionality() {
-    $this->assertText('You are not authorized to post comments', 'Error attempting to post comment.');
-    $this->assertNoFieldByName('subject[0][value]', '', 'Subject field not found.');
-    $this->assertNoFieldByName('comment_body[0][value]', '', 'Comment field not found.');
+    $this->assertResponse(403, 'Error attempting to post comment.');

These are leftovers of a previous version of #2322949: Implement generic entity link view field handlers.

PS: Its pretty epic how many handlers we have been able to remove due to all this effort in the context of views <-> entity integration.

+1

We have issues open already for the history markers - see [#2086767] and #2082319: Comment's node_new_comments View field history markers ("new" comment marker) forces render caching to be per user

Yes, but this issue should then at least update \Drupal\history\Plugin\views\field\HistoryUserTimestamp::render() to set the user cache context.
Great find, @dawehner!

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -47,11 +55,31 @@ class RenderCache implements RenderCacheInterface {
   +    CacheableMetadata::createFromRenderArray($build)
   +      ->applyTo($build);
   

   Like @dawehner, this doesn't make any sense to me. It should effectively have no effect. Well, except for it adding ['#cache']['tags'] = [].

2. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -305,6 +333,12 @@ public function getCacheableRenderArray(array $elements) {
   +    if (!empty($elements['#cache']['properties']) && is_array($elements['#cache']['properties'])) {
   +      $data += array_intersect_key($elements, array_flip($elements['#cache']['properties']));
   +    }
   
   +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
   +            // We do not need the full markup, just individual fields.
   +            $data['#markup'] = '';
   +            foreach ($field_ids as $id) {
   +              $data[$id] = ['#markup' => $data[$id]['#markup']];
   +            }
   ...
   +              'properties' => array_merge($field_ids, ['#views_render_tokens']),
   

   Woah, mind blown! :D

   At the very least, this is missing documentation.

3. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -17,6 +17,17 @@
      /**
   +   * Creates a new cacheable render array with required metadata pre-filled.
   +   *
   +   * @param string[] $keys
   +   *   The cache keys.
   +   *
   +   * @return array
   +   *   The cacheable render array.
   +   */
   +  public function createCacheableRenderArray(array $keys);
   
   +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
   +          $cache_array = $render_cache->createCacheableRenderArray($keys);
   

   I don't understand the value of this. AFAICT the only value is to get the required cache contexts set on a render array.

   But… then why not let RenderCache::(get|set)() do this? Because it'd mean additional function calls (think array_diff( $required_cache_contexts, $contexts)) and we don't want to incur those?

   IMHO this super advanced case — which will likely never be paralleled by anybody in contrib, and if so, affect 0.0001% of developers — should just retrieve the required cache contexts and apply it to the render array. It could then do it once, outside of this loop, and not have to call this method for every row.

4. +++ b/core/modules/comment/src/Tests/Views/CommentUserNameTest.php
   --- a/core/modules/contact/src/Access/ContactPageAccess.php
   +++ b/core/modules/contact/src/Access/ContactPageAccess.php
   

   These changes confuse me. I'll reroll to change it to how I think it should work, and see if that passes tests.

   For example, if the first return statement is used, then we're caching that per user, until the contacted account entity changes and until the configuration changes. For no reason at all, because the anonymous user always remains the anonymous user.

   In my suggested-interdiff.txt:

   1. The first case (contacted account is anonymous) just returns AccessResult::forbidden(). HEAD was varying this per permissions, which was indeed nonsensical.
   2. The second case (prevent self-contacting) is where we set up a $access variable that we keep reusing as we go further in the function body. It only needs to vary per user, just like HEAD was doing. The difference with HEAD is that we don't return "forbidden" — which is fine, because it *is* possible that a contrib module wants to allow a user to contact themselves to leave notes. If it's already forbidden, then such a contrib module would be impossible.
   3. The third case (admins can use any personal contact form) fixes HEAD in that it no longer overwrites $access, but adds to it, and instead of manually saying "per permissions", we make it inherit the cacheability of $permission_access.

   That's it. I also included suggested-interdiff-against-HEAD.txt to show the changes relative to HEAD.

   ContactLinkTest and ContactPersonalTest still pass with these changes.

5. +++ b/core/modules/contact/src/Tests/Views/ContactLinkTest.php
   @@ -84,6 +85,8 @@ public function testContactLink() {
   +    // @todo Remove cache invalidation in https://www.drupal.org/node/2477903.
   +    Cache::invalidateTags($no_contact_account->getCacheTags());
   

   Manually confirmed that this is necessary. The only reason it works in HEAD, is that if you modify your profile, changes to this are saved in tandem with changes to the User entity, which means that the User entity's cache tag is invalidated, and hence it all works.

   But, as you can see in contact_user_profile_form_submit(), no cache tag invalidation happens there.

   So it currently only works by accident. +1 to that issue this links to.

6. +++ b/core/modules/views/src/Plugin/views/field/LinkBase.php
   @@ -124,8 +126,17 @@ public function query() {
   +    $build = [];
   +
   +    $access = $this->checkUrlAccess($row);
   +    if ($access instanceof AccessResultInterface) {
   +      BubbleableMetadata::createFromObject($access)->applyTo($build);
   +      $access = $access->isAllowed();
   +    }
   +    $build['#markup'] = $access ? $this->renderLink($row) : '';
   

   There are several problems here:

   1. I think you actually meant instanceof CacheableDependencyInterface? :)
   2. Access results that don't implement CacheableDependencyInterface must be considered to have max-age = 0, i.e. must be considered to be uncacheable. This is actually taken care of by ::createFromObject() already! So, no need for the if-statement.
   3. I can't imagine $access = $access->isAllowed() being anything else but a security leak?

   If point one was incorrect, then I'm even more confused, because ::checkUrlAccess() already promises to return an AccessResultInterface object.

   AFAICT this can be simplified to:

   $access = $this->checkUrlAccess($row);
   $build = [
     '#markup' => $access->isAllowed() ? $this->renderLink() : '',
   ];
   $access->applyTo($build);
   

7. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
   +          // after manually pre-rendering it and discarding the global markup.
   

   What is "global markup"?

8. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
   +          // This allow us to cache the markup of the various children, that is
   

   s/allow/allows/

9. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
   +            // We need to cache also row render tokens, since they rely on data
   +            // saved in the field handler objects, which is populated only when
   +            // actually rendering field values.
   +            $data['#views_render_tokens'] = $this->view->field[$render_tokens_field_id]->getRenderTokens([]);
   

   Woah, this is getting all render tokens and caching them!

   1. Isn't there a way to only generate the ones we actually need? Probably not.
   2. More importantly, we don't know the cacheability of these tokens… so is it possible that some of these tokens are actually varying per user, or maybe even are uncacheable? I can imagine that being super edge casey, but I'm kinda hoping that this is impossible. Then we wouldn't need to worry about that.

10. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
    +   * #pre_render callback for view display rendering.
    ...
    +  public function elementPreRenderRow(array $element) {
    

    Not view display rendering, but view row rendering.

11. +++ b/core/modules/views/src/ResultRow.php
    @@ -7,6 +7,8 @@
    +use Drupal\Core\Cache\Cache;
    

    Unused. :)

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -47,11 +55,31 @@ class RenderCache implements RenderCacheInterface {
   +  public function createCacheableRenderArray(array $keys) {
   +    $build = [
   +      '#cache' => [
   +        'keys' => $keys,
   +        'contexts' => $this->rendererConfig['required_cache_contexts'],
   +      ],
   +    ];
   +
   +    CacheableMetadata::createFromRenderArray($build)
   +      ->applyTo($build);
   +
   +    return $build;
   

   Lets not do this, this is a bug in the Renderer -> RenderCache split.

   ->set() should always merge in the required cache contexts before saving to the cache.

   Renderer could instead only do it for $is_root_call = TRUE.

   Both profit.

2. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -305,6 +333,12 @@ public function getCacheableRenderArray(array $elements) {
   +    if (!empty($elements['#cache']['properties']) && is_array($elements['#cache']['properties'])) {
   +      $data += array_intersect_key($elements, array_flip($elements['#cache']['properties']));
   +    }
   

   I don't think we want #cache properties, but an option to ->set().

   If we want #cache properties, we btw. could use the normal #pre_render pattern again and would not need to use the RenderCache API.

   #cache is for cacheability metadata, not for cache control of what to cache - that is at least Wim's and mine current understanding of it.

3. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -17,6 +17,17 @@
      /**
   +   * Creates a new cacheable render array with required metadata pre-filled.
   ...
   +  public function createCacheableRenderArray(array $keys);
   

   Should not be necessary as explained.

4. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
   +            $original = $cache_array + $data;
   +            $renderer->render($data);
   

   If you use that then you essentially still cache set twice and also attempt another cache get.

   'keys' should not be set here.

5. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
   +            foreach ($field_ids as $id) {
   +              $data[$id] = ['#markup' => $data[$id]['#markup']];
   +            }
   

   Is there other data in $data[$id]?

   I thought this would not be necessary?

6. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
   +            $data['#cache'] = [
   +              'keys' => $cache_array['#cache']['keys'],
   +              'contexts' => Cache::mergeContexts($data['#cache']['contexts'], $cache_array['#cache']['contexts']),
   +              'tags' => Cache::mergeTags($data['#cache']['tags'], $cache_plugin->getRowCacheTags($row)),
   +              'properties' => array_merge($field_ids, ['#views_render_tokens']),
   +            ];
   +            $renderer->addCacheableDependency($data, $this->view->storage);
   

   RenderCache::set is responsible for merging default data.

7. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +640,120 @@ protected function renderFields(array $result) {
   +            $render_cache->set($data, $original);
   

   Here the cache keys,tags,contexts need to be added to both original and $data, that is right for sure.

Opened #2483887: Mark RenderCache as internal for the bug, but this bug is not _blocking_ this issue as we found another approach here where we can get back to the nicer #pre_render pattern.

#117:

->set() should always merge in the required cache contexts before saving to the cache.

Then so should ::get(), to avoid cache misses when it'd actually be a cache hit.

Outcome of IRC was:

- We use #cache_properties for now to specify additional properties to cache (there is quite a few use cases independent of this when working on real sites, I encountered lots of them for render_cache in D7).
- We go back to the #pre_render + #cache + #cache_properties pattern, now that we can.

That will give use multiple cache get also for free (once renderer supports it) with a very little change in the future (two loops instead of one).

This should address the reviews above, except for #116.9: I was assuming only field data would be displayed, so cache contexts would automatically fix that, but I realized there are also other tokens. Anyway, I will try to fix that together with uncacheable field handlers, I have an idea that might work, I'll try it later.

Please pay special care to the changes to Renderer/Render Cache, as I was not able to save an empty markup for the parent element without them. Feedback/suggestions welcome.

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -334,8 +306,16 @@ public function getCacheableRenderArray(array $elements) {
   +    if (!empty($elements['#cache_properties']) && is_array($elements['#cache_properties'])) {
   +      // Preserve cacheable items if specified. If we are preserving element
   +      // children we assume we are only interested in their individual markup
   +      // not the parent one, thus we empty it to minimize the cache entry size.
   +      $data['#cache_properties'] = $elements['#cache_properties'];
   +      $cacheable_items = array_intersect_key($elements, array_flip($elements['#cache_properties']));
   +      if (Element::children($cacheable_items)) {
   +        $data['#markup'] = '';
   +      }
   +      $data += $cacheable_items;
        }
   
   +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -180,7 +179,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +        // Mark the element markup as safe. If we have cached children, we need
   +        // to mark them as safe too.
            $elements['#markup'] = SafeMarkup::set($elements['#markup']);
   +        if (!empty($elements['#cache_properties'])) {
   +          foreach (Element::children($cached_element) as $key) {
   +            SafeMarkup::set($cached_element[$key]['#markup']);
   +          }
   +        }
   

   *Much* clearer, thank you! Now all that's missing for this, is test coverage.

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -663,58 +660,23 @@ protected function renderFields(array $result) {
   -          $keys = $cache_plugin->getRowCacheKeys($row);
   -          $cache_array = $render_cache->createCacheableRenderArray($keys);
   -          $data = $render_cache->get($cache_array);
   -
   -          if (!$data) {
   -            // If we have a cache miss, render field markup via the renderer so
   -            // bubbleable metadata can be preserved before caching the result.
   -            $data = [
   -              '#pre_render' => [[$this, 'elementPreRenderRow']],
   -              '#row' => $row,
   -            ];
   -            // We add pre-bubbling cacheable metadata to the original array to
   -            // make cache redirects work when storing the cache entry.
   -            $original = $cache_array + $data;
   -            $renderer->render($data);
   -
   -            // We do not need the full markup, just individual fields.
   -            $data['#markup'] = '';
   -            foreach ($field_ids as $id) {
   -              $data[$id] = ['#markup' => $data[$id]['#markup']];
   -            }
   -
   -            // We need to cache also row render tokens, since they rely on data
   -            // saved in the field handler objects, which is populated only when
   -            // actually rendering field values.
   -            $data['#views_render_tokens'] = $this->view->field[$render_tokens_field_id]->getRenderTokens([]);
   -
   -            // Set cacheable metadata.
   -            $data['#cache'] = [
   -              'keys' => $cache_array['#cache']['keys'],
   -              'contexts' => Cache::mergeContexts($data['#cache']['contexts'], $cache_array['#cache']['contexts']),
   -              'tags' => Cache::mergeTags($data['#cache']['tags'], $cache_plugin->getRowCacheTags($row)),
   -              'properties' => array_merge($field_ids, ['#views_render_tokens']),
   -            ];
   -            $renderer->addCacheableDependency($data, $this->view->storage);
   -
   -            // Manually set the render cache item so we can keep markup for
   -            // individual fields.
   -            $render_cache->set($data, $original);
   -          }
   -          else {
   -            // Render the data to mark strings as safe and push cacheable
   -            // metadata to the stack if we are dealing with a cached item.
   -            $renderer->render($data);
   -          }
   +          $data = [
   +            '#pre_render' => [[$this, 'elementPreRenderRow']],
   +            '#row' => $row,
   +            '#cache' => [
   +              'keys' => $cache_plugin->getRowCacheKeys($row),
   +              'tags' => $cache_plugin->getRowCacheTags($row),
   +            ],
   +            '#cache_properties' => $cache_properties,
   +          ];
   +          $renderer->addCacheableDependency($data, $this->view->storage);
   +          $renderer->render($data);
   

   Holy moly, this is night and day! Fifty times more understandable :D

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -305,6 +305,20 @@ public function getCacheableRenderArray(array $elements) {
   +      $data['#cache_properties'] = $elements['#cache_properties'];
   +      $cacheable_items = array_intersect_key($elements, array_flip($elements['#cache_properties']));
   +      if (Element::children($cacheable_items)) {
   +        $data['#markup'] = '';
   +      }
   +      $data += $cacheable_items;
   

   That is very clever. I like the thinking that if we have the children we don't need the '#markup' and it will save quite some space.

   In case someone really needs the markup, they could use a #post_render callback to put it in another property.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -180,7 +179,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +        if (!empty($elements['#cache_properties'])) {
   +          foreach (Element::children($cached_element) as $key) {
   +            SafeMarkup::set($cached_element[$key]['#markup']);
   +          }
   +        }
   

   If we support that as an API that also makes lots of sense.

3. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -112,6 +112,13 @@
      /**
   +   * Stores the render API render cache.
   +   *
   +   * @var \Drupal\Core\Render\RenderCacheInterface
   +   */
   +  protected $renderCache;
   

   No longer needed!

4. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -596,4 +603,17 @@ protected function getRenderer() {
   +  /**
   +   * Returns the render API render cache.
   +   *
   +   * @return \Drupal\Core\Render\RenderCacheInterface
   +   *   The render cache.
   +   */
   +  protected function getRenderCache() {
   

   No longer needed!

5. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +641,84 @@ protected function renderFields(array $result) {
   +          $data = [
   +            '#pre_render' => [[$this, 'elementPreRenderRow']],
   +            '#row' => $row,
   ...
   +          $renderer->addCacheableDependency($data, $this->view->storage);
   +          $renderer->render($data);
   

   Love how easy that now again is!

6. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -637,27 +641,84 @@ protected function renderFields(array $result) {
   +            '#cache_properties' => $cache_properties,
   

   It turns out (sigh), that we could also support that manually by doing:

   $build = [
     '#views_cache_properties' => $cache_properties,
     '#post_render' => [
       function($markup, &$elements) {
         $elements['#attached']['views_cache_properties'] = array_intersect_key($elements, $elements['#views_cache_properties']);
         return '';
     })]
   ];
   

   and then checking for CacheGet and rendering children manually in that case. (as before) ...

   I defer to core committers however if #cache_properties is acceptable as I think its generally useful.

I also still need to check #116.4.

NW per #122, #123 and #124. Patch is green :)

This takes care of #116.4, #122, and #123. While adding test coverage I found a glitch with cache properties handling: we were caching lots of additional properties for children, aside from #markup. Fixed that.

@Fabian:

It turns out (sigh), that we could also support that manually by doing: [...]
and then checking for CacheGet and rendering children manually in that case. (as before) ...
I defer to core committers however if #cache_properties is acceptable as I think its generally useful.

That's exactly what I originally did in one the patches above, but I sincerely doubt committers will argue against an addition that lets us simplify things like this.

Back to trying to make uncacheable stuff and render tokens work.

Just a few nitpicks. The test coverage looks great. This patch looks great. :)

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -312,8 +312,13 @@ public function getCacheableRenderArray(array $elements) {
   +        // Cache only children markup.
   
   +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -247,12 +247,12 @@ public function renderPlain(&$elements);
   +   *     assumes only children individual markup is relevant and ignores the
   
   +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -622,6 +622,75 @@ public function providerTestRenderCacheMaxAge() {
   +    // Check that parent markup is ignored when caching children markup.
   

   s/children/children's/

2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -622,6 +622,75 @@ public function providerTestRenderCacheMaxAge() {
   +    $this->assertEquals(empty($data['#markup']), (bool) Element::children($data));
   

   Hrm, empty($data['#markup'] could also mean !isset($data['#markup'])?

   I think we want isset($data['#markup']) && $data['#markup'] === '' here, to remove all ambiguity.

3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -622,6 +622,75 @@ public function providerTestRenderCacheMaxAge() {
   +    // Check the element properties are cached as specified.
   

   s/Check the/Check that the/

4. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -622,6 +622,75 @@ public function providerTestRenderCacheMaxAge() {
   +      [['child1' => 0, 'child2' => 0, '#custom_property' => 0]],
   +      [['child1' => 0, 'child2' => 0, '#custom_property' => 1]],
   +      [['child1' => 0, 'child2' => 1, '#custom_property' => 0]],
   +      [['child1' => 0, 'child2' => 1, '#custom_property' => 1]],
   +      [['child1' => 1, 'child2' => 0, '#custom_property' => 0]],
   +      [['child1' => 1, 'child2' => 0, '#custom_property' => 1]],
   +      [['child1' => 1, 'child2' => 1, '#custom_property' => 0]],
   +      [['child1' => 1, 'child2' => 1, '#custom_property' => 1]],
   

   These 0s and 1s are actually FALSEs and TRUEs. I think plach went with integers instead of booleans because it'd yield a more legible end result? :)

2: Great point, although I think we don't need to care about notices, on the contrary, so I simplified the check to $data['#markup'] === ''.
4: Exactly :)

Still working on the rest...

Patch looks really great now!

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -180,7 +179,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+        // Mark the element markup as safe. If we have cached children, we need
+        // to mark them as safe too.
...
+        if (!empty($elements['#cache_properties'])) {
+          foreach (Element::children($cached_element) as $key) {
+            SafeMarkup::set($cached_element[$key]['#markup']);
+          }

It would be nice to explain why its okay to mark them as safe.

Nice work @plach

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -305,6 +305,25 @@ public function getCacheableRenderArray(array $elements) {
   +    // Preserve cacheable items if specified. If we are preserving element
   +    // children we assume we are only interested in their individual markup not
   +    // the parent one, thus we empty it to minimize the cache entry size.
   +    if (!empty($elements['#cache_properties']) && is_array($elements['#cache_properties'])) {
   +      $data['#cache_properties'] = $elements['#cache_properties'];
   +      $cacheable_items = array_intersect_key($elements, array_flip($elements['#cache_properties']));
   +      $children = Element::children($cacheable_items);
   +      if ($children) {
   +        $data['#markup'] = '';
   +        // Cache only children markup.
   +        foreach ($children as $key) {
   +          $cacheable_items[$key] = ['#markup' => $cacheable_items[$key]['#markup']];
   +        }
   +      }
   +      $data += $cacheable_items;
   +    }
   +
   +    return $data;
   

   This is all magic to me. :(
   Can we please add some inline commenting here?

2. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -425,6 +426,74 @@ protected function prepareViewResult(array $result) {
   +    // Here we compute a unique identifier for the row by computing the hash of
   +    // its data. We exclude the current index, since the same row could have a
   +    // different result index depending on the user permissions. We exclude also
   +    // entity data, since serializing entity objects is very expensive. Instead
   +    // we include entity cache tags, which are enough to identify all the
   +    // entities associated with the row.
   

   Now these are some good docs. :)

It would be nice to explain why its okay to mark them as safe.

Well, they were originally marked as safe and their parent (which is just a concatenation of their markup) is marked as safe so they must be too :)

I guess we need Fabian or Wim for a better answer :)

This is all magic to me. :(
Can we please add some inline commenting here?

Can you specify where you'd wish to have more comments and what they should clarify? I mean, there are already a couple of inline comments in the code block you outlined :)

How about this? This is my understanding of the code I might be wrong. Please feel free to correct it. :)

This should fix row render tokens handling and uncacheable handlers. I discussed with Daniel the introduction of a ::postRender() method to field handlers, given that #post_render does not work, since its results is cached, and #post_render_cache does not work either, as it's impossible to reconstruct the view context without re-executing it.

If this approach is ok, we just need to update the field handlers mentioned in #113 and add some test coverage and we should be done.

This also incorporates #133 with a few adjustments to avoid reverting a previous review.

Test fixes + clean-up.

Wrong patch, interdiff in #136 is correct.

Only nits. IMHO this is RTBC. But it'd be great if @dawehner could review the #134 + #136 interdiffs — I don't know "Views tokens" well enough to be able to properly judge that.

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -180,7 +179,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +        // Mark the element markup as safe. If we have cached children, we need
   +        // to mark them as safe too.
   

   Great comment, but to address #131, maybe append (The parent markup contains the child markup, so if the parent markup is safe, then the markup of the individual children must be safe as well.)

2. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
   @@ -174,6 +174,19 @@ public function preRender(&$values);
   +   * Runs after every fields has been rendered.
   

   s/fields/field/

3. +++ b/core/modules/views/src/Plugin/views/field/UncacheableFieldHandlerTrait.php
   @@ -0,0 +1,65 @@
   + * Definition of Drupal\views\Plugin\views\field\UncacheableFieldHandlerTrait.
   

   Nit: s/Definition of Drupal/Contains \Drupal/

1. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -491,7 +491,9 @@ public function getRowId(ResultRow $row) {
   -    return hash('sha256', serialize($row_data));
   +    $row_id = hash('sha256', serialize($row_data));
   +
   +    return $row_id;
      }
   

   Any reason for this change?

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1110,6 +1110,14 @@ public function render(ResultRow $values) {
   +    $this->last_render = $output;
   

   It feels like this need some documentation? I don't understand it either, to be honest.

3. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1542,6 +1553,16 @@ public function getRenderTokens($item) {
   +  protected function getTokenPlaceholder() {
   

   Do you mind renaming it to getFieldTokenPlaceholder

4. +++ b/core/modules/views/src/Plugin/views/field/UncacheableFieldHandlerTrait.php
   @@ -0,0 +1,65 @@
   + * Definition of Drupal\views\Plugin\views\field\UncacheableFieldHandlerTrait.
   

   ... contains

+++ b/core/modules/views/tests/src/Unit/Plugin/field/CounterTest.php
@@ -236,11 +235,28 @@ public function testCounterSecondPage($i) {
+    $markup = $handler->render($row);
+    $handler->postRender($row, $markup);
+    return $handler->last_render;

should we maybe replace it with $style->getField()

Rerolled and fixed #139 and #140. Working on additional test coverage and then we should be done for reals :)

Any reason for this change?

I was just reverting a previous unneeded change.

should we maybe replace it with $style->getField()

It's a unit unit test :)

Once we have the complex test example of plach, we should be pretty much done with it.

Once we have the complex test example of plach, we should be pretty much done with it.

+1!

Excellent work, plach!

Now with additional test coverage

1. +++ b/core/modules/views/src/Tests/Plugin/RowRenderCacheTest.php
   @@ -0,0 +1,171 @@
   +  public static $testViews = array('test_render_cache');
   

   Should we use name the view test_render_row_cache maybe? Maybe that is more clear then, what its about.

2. +++ b/core/modules/views/src/Tests/Plugin/RowRenderCacheTest.php
   @@ -0,0 +1,171 @@
   +  function testFieldTokenHandling() {
   

   Nitpick: public ...

3. +++ b/core/modules/views/src/Tests/Plugin/RowRenderCacheTest.php
   @@ -0,0 +1,171 @@
   +      $expected = "<a href=\"/node/$nid\"><span class=\"da-title\">{$node->label()}</span> <span class=\"counter\">$counter_output</span></a>";
   ...
   +      $expected = $access ? "<a href=\"/node/$nid/edit?destination=/\" hreflang=\"en\">edit</a>" : "";
   ...
   +      $expected = $access ? "<a href=\"/node/$nid/delete?destination=/\" hreflang=\"en\">delete</a>" : "";
   ...
   +      $expected = $access ? "  <div class=\"dropbutton-wrapper\"><div class=\"dropbutton-widget\"><ul class=\"dropbutton\">" .
   +          "<li class=\"edit\"><a href=\"/node/$nid/edit?destination=/\" hreflang=\"en\">Edit</a></li>" .
   

   I'm curious whether it works on subdir installlations.

4. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_render_cache.yml
   @@ -0,0 +1,504 @@
   +uuid: 79cd165c-2341-49a2-9355-85a652d17cbd
   

   we don't put uuids in test views.

Two remarks from live-review yesterday:

1. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -425,6 +426,74 @@ protected function prepareViewResult(array $result) {
   +  public function getRowCacheTags(ResultRow $row) {
   ...
   +  public function getRowCacheKeys(ResultRow $row) {
   ...
   +  public function getRowId(ResultRow $row) {
   

   All these functions need to be moved to the interface, because views depends on them being there.

   Else using a custom cache plugin that is not extending CachePluginBase will fail completely and in strange ways ...

   If we don't want to muddy the interface, we would need to check for the functions directly or for another interface and skip caching if that interface / function is not present.

2. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
   @@ -174,6 +174,23 @@ public function preRender(&$values);
   +   *
   +   * This is meant to be used mainly to deal with uncacheable field handlers.
   +   *
   

   I think it would be good to explain a little more that its not for totally uncacheable data, but for data that is uncacheable / dynamic per row, but cacheable for the complete page (or complete view) in upper layers.

.

This should address #147 , #148, fix test failures and improve test coverage.

Investigating the field handlers list in #113 to see whether there's anything left to do.

This fixes the BulkForm handler mentioned in #113. ContextualLinks could not be fixed as they are broken in HEAD (see #2487742: Contextual links Views field handler is broken). HistoryUserTimestamp has its own issue to fix the View field handler (I cannot find it right now).

Additionally operation links are still not working, as they return no cacheability metadata. The admin content view is working though thanks to a favorable combination of things. So, we can either commit this or postpone it to #2473873: Views entity operations lack cacheability support, resulting in incorrect dropbuttons (and make that one critical), but we should be done here.

Let's get it in!

This is what Daniel meant to do :)

The user history issue: #2082319: Comment's node_new_comments View field history markers ("new" comment marker) forces render caching to be per user