Override number of items to display in contextual filter doesn't work

Broken also in 8-rc1

This is broken in 8.x RC2 and ALSO affects the shipped "Archive" view:

1. Install Drupal 8.x RC2.
2. Make some nodes.
3. Enable the "Archive" view.
4. Go to /archive.

RESULT: White screen of death and the stated LogicException in the watchdog.

To fix the core view, one needs to uncheck the "Override number of items to display in contextual filter" in the filter.

This might be critical, definitely RC target.

meh.

Looking at some test coverage for now.

There we go

+++ b/core/modules/views/src/Plugin/views/style/DefaultSummary.php
@@ -37,7 +37,7 @@ protected function defineOptions() {
-      $this->view->setItemsPerPage(intval($this->options['items_per_page']));
+//      $this->view->setItemsPerPage(intval($this->options['items_per_page']));

Looks unintentional

Looks unintentional

Oh yeah, well the other test coverage already proves that something is going on wrong

For this particular usecase we already vary by URL due to the usage of the arguments/->url cache context.
Given that its save to not deal with cache keys/contexts in ViewExecutable::setItemsPerPage()
Maybe we want to add a parameter back which allows callers to not deal with cache keys?

Can't we just do the same as ViewExecutable::setCurrentPage() is doing, and check if the element is already pre rendered?

#14 works for me. RTBC - looks great!

+++ b/core/modules/views/src/ViewExecutable.php
@@ -553,7 +553,11 @@ public function getItemsPerPage() {
+    // Check whether the element is pre rendered. At that point, the cache keys
+    // cannot longer be manipulated.
+    if (empty($this->element['#pre_rendered'])) {
+      $this->element['#cache']['keys'][] = 'items_per_page:' . $items_per_page;
+    }

Should we provide some unit test coverage for this behaviour? We would though need to apply the same fixes to setOffset and setCurrentPage

There we go.

There we go.

Seriously, testbot, patches should be allowed to be treated as if they would be equal.

Back to RTBC - Looks great to me!

+++ b/core/modules/views/src/Tests/Plugin/StyleSummaryTest.php
@@ -0,0 +1,76 @@
+    for ($i = 0; $i < 5; $i++) {
+      for ($j = 0; $j < 5; $j++) {
+        $this->entities[] = $entity = EntityTest::create([
+          'name' => 'Entity 1',
+          'type' => 'type' . $i,
+        ]);
+        $entity->save();
+      }
+    }
+  }

This could do with a comment - why do we need 5 of each? And why do they all have the same name/label?

Good old numeric code:
($i * 5 + $j)

s/node types/entity types but that's fixable on commit.

If you correct it, please use '/entity bundles/

doh, yes.

Committed/pushed to 8.0.x, thanks!