Views using pagers should specify a cache context

Re-opening to take the views parts into account.

Views has a setPager method and we need to deal with that somehow in regards to cache context.

We'll find a solution :).

Is this all we need?

Is that patch trying to say "use the regular pager cache context, but enforce the Views pager's 'current page'"?

Yeah, would that make sense?

Let's give another example.
Random code in your template preprocess:

$view = Views::getView($name);
$view->setItemsPerPage(12);
$view->setCurrentPage(2);
$vars['foo'] = $view->preview();

There is a world beyond pages :)

In that case the template code needs to specify manually what it wants this to be varied on in my opinion.

Does $view->preview() show a pager, btw.?

Yes it does.

In that case the template code needs to specify manually what it wants this to be varied on in my opinion.

Well, I still think that if we could do something about it, we should do, because otherwise there will be just tons of bugs by people all over the place.

I agree and it is not forgotten.

So what should we do?

Lets work on some test cases?

Or we could use the 'hard, but correct method' and set max-age==0 when you use setPage() as we cannot determine if you do:

if ($this->moonService->getRandom() == '2') {
  $this->view->setCurrentPage(20);
} else {
  $this->view->setCurrentPage(10);
}

but if you know what it varies on you would need to specify something like this for example:

if ($this->moonService->getPhase() == 'full') {
  $this->view->setCurrentPage(20);
} else {
  $this->view->setCurrentPage(10);
}
$this->view->setMaxAge(Cache::PERMANENT);
$this->view->addCacheContexts(['moon']);

What do you think?

I'd like to point that view can render nodes with comments but comment formatter provides own "dumb" setting for pager.
Was done in #2155387: Multiple comment fields per entity are supported, but break horribly when they need pagers and #2430341: Comment pager-dependent cache key should be a cache context

So you can easily get in situation (probably in tests) where the same cache context would be exposed from comment field and view itself.

Let's see ...

The idea is: once you have those methods, you opt out of caching, but if you want, you can opt in and provide your corresponding cache contexts.

The last patch makes sense to me, but in the case of regular page displays I guess we could keep caching enabled and specify the regular pager contexts.

So we need to ensure CachePluginBase does not make things uncacheable when retrieving results.

Spoke with Daniel: #18 is the wrong fix. This addresses #19, I'll test this a bit more to see what I can do to fix #18.

No interdiff as I touched almost every line.

OMG, I actually posted the interdiff instead...

This makes pagers actually work with cached full views. Still investigating Wim's issue.

Fixed failures

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2323,6 +2323,7 @@ public function buildRenderable(array $args = []) {
+        'keys' => ['view', $this->view->storage->id(), $this->display['id']],

We should ensure to not include that as part of the patch, right?

Meh, too many branches

+++ b/core/modules/views/src/Tests/Handler/FieldWebTest.php
@@ -182,7 +182,7 @@ protected function xpathContent($content, $xpath, array $arguments = array()) {
+  public function atestAlterUrl() {

@@ -329,7 +329,7 @@ public function testAlterUrl() {
+  public function atestFieldClasses() {

@@ -438,7 +438,7 @@ public function testFieldClasses() {
+  public function atestTextRendering() {

Indeed

Correct interdiff

Added the needed expected cache contexts.

I think this should be ready to go. I investigated Wim's issue but I cannot reproduce it: when result cache is enabled (and $this->view->setCurrentPage($cache->data['current_page'], TRUE) is changed to $this->view->setCurrentPage($cache->data['current_page'])) max age is 0 and the view is not cached as expected.

Unintended status change

There we go, added test coverage for it.

Here is the test which will fail as well.

And the interdiff

+++ b/core/modules/views/src/Tests/Plugin/PagerKernelTest.php
@@ -0,0 +1,74 @@
+  protected function setUp($import_test_views = TRUE) {

Fixed missing PHP doc, aside from that looks great to me.

Fixed missing PHP doc, aside from that looks great to me.

+1

@wim
Do you mind reviewing stuff again?

Blocks #2381277: Make Views use render caching and remove Views' own "output caching", bumping to critical.

+++ b/core/modules/views/src/ViewExecutable.php
@@ -491,10 +491,20 @@ public function setArguments(array $args) {
+   *   The current page.
+   * @param bool $keep_cacheability
+   *   (optional) Keep the cacheability, by default we mark the issue as not
+   *   cacheable. Defaults to FALSE.
    */
-  public function setCurrentPage($page) {

Should this explain what to do if you set $keep_cacheability to TRUE?

Also 'mark the issue' - should it be 'mark the view'? - this is repeated each time the doc is added.

Also 'mark the issue' - should it be 'mark the view'? - this is repeated each time the doc is added.

Ha!

Explained a bit, why we need this parameter and why its FALSE by default, feel free to improve it.

just some grammar changes I noticed in the last interdiff.

Next I'll read the whole thing.

+++ b/core/modules/views/src/Tests/Plugin/PagerKernelTest.php
@@ -0,0 +1,77 @@
+      \Drupal::service('renderer')->renderPlain($output);
+      $this->assertIdentical(0, $output['#cache']['max-age'], 'Max age set to 0 without $keep_cacheablity.');
...
+      \Drupal::service('renderer')->renderPlain($output);
+      $this->assertIdentical(CacheBackendInterface::CACHE_PERMANENT, $output['#cache']['max-age'], 'Max age kept on -1 with $keep_cacheablity.');

+++ b/core/modules/views/src/ViewExecutable.php
@@ -491,10 +491,22 @@ public function setArguments(array $args) {
+    if (!$keep_cacheability) {
+      $this->element['#cache']['max-age'] = 0;
+    }

if it is not cacheable, we override whatever it might be. (daniel says we dont know what it is yet, and could be set in a views setting.)

In the test, we test for 0 (not cacheable) and -1 (permanent), but seems we do not have a test that tests the value from the setting actually bubbles up.

Maybe we should add a test for that?

------

Everything else looked ok to me. (read the whole patch)

Does not seem to need manual testing or anything, because the test is checking the header to see if the context for the page is added.
#2381277: Make Views use render caching and remove Views' own "output caching" will be the issue that will use that context in the header to actually use the context.

Value array ( 0 => 'languages:language_interface', 1 => 'theme', 2 => 'url.query_args.pagers:0', 3 => 'url.query_args:order', 4 => 'url.query_args:sort', ) is identical to value array ( 0 => 'languages:language_interface', 1 => 'theme', 2 => 'url.query_args:order', 3 => 'url.query_args:sort', ). Other FieldWebTest.php 81 Drupal\views\Tests\Handler\FieldWebTest->testClickSorting()

fail is from #2489966: The Views table style plugin does not specify cache contexts for click sorting which added a test that went in today. I'll fix it now.

just added the new pager context to that test.
test passes locally.

Thinking more, the test I asked about in #51 is probably not worth delaying this critical. So I opened a normal task to add the test (and discuss if we need it). #2490852: Add a test for views pager cache age setting

updated the issue summary.

I'm guessing we do not need a change record for this. #2381277: Make Views use render caching and remove Views' own "output caching" probably doesn't need one either...

RTBC, Let's get this in.

Test coverage looks good, we know that bubbling works from other tests, too.

The solution to explicitly opt-in to keep the current cacheability looks great!

@tim.plunkett and I were trying to see the page cache tag in the browser... and we haven't figured out how to see it on admin/content (yet)

I can see it on a view I made.

Reading through \Drupal\Core\Cache\CacheContextsManager::optimizeTokens() @YesCT and I finally figured out why we couldn't see url.query_args.pagers:0, because url is already included and is an "ancestor", so the more specific context is not needed.

RBTC +1

Thank you for the final push @yesct!
Its amazing how things work together at the end.

Thanks for the additional docs.

Committed/pushed to 8.0.x, thanks!

Yes that was an awful cross-post.

I've gone ahead and reverted for #62, back to CNW.

I can still reproduce this. Using the front page view, with tag-based caching enabled. With or without the cited TRUE, the front page gets an output cache item.

So the corresponding cache contexts don't appear in the HTTP response?
I try to understand what you actually expect as part of this issue. Output caching doesn't use render caching ... but output caching relies on $view->result which itself depends on the dynamic pager data:

      $key_data['pager'] = [
        'page' => $this->view->getCurrentPage(),
        'items_per_page' => $this->view->getItemsPerPage(),
        'offset' => $this->view->getOffset(),
      ];

aka. we don't need such workarounds for output caching in its current form. Plach and I talked about getting rid of output caching though, but for sure, this is 100% out of scope of this issue.
I don't consider this as a blocker of the issue itself.

Fixed the nits ...

Assigning to Wim to provide:

- Steps to reproduce

Looked over the code and ran all the tests locally under PHP 5.5 with no issues. RTBC?

Yes, lets create a follow-up to remove the output cache - as its broken currently anyway due to no bubbling support.

We could also do it as part of the parent: "Make views use render-caching issue" though.

Committed/pushed to 8.0.x, thanks!

Removing output caching will happen in #2381277: Make Views use render caching and remove Views' own "output caching", see the proposed solution.