View preview is broken in UI if a pager has to be displayed

The patch works very well.

For testing I installed an new empty drupal 8.x and could reproduce the error very easy by generating 50 items with devel and creating a views using a pager with 10 items at each site.

Before patching:
There are no items displayed in preview:

After patching:
After applying patch and clearing cache same view displayes items im preview as expected. The pager itself is also being rendered (out of screenshot...)

Considering a php error is demonstrable we have tests to ensure we don't break this in the future

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/Full.phpundefined
@@ -89,12 +89,13 @@ function render($input) {
+    $output = array(
+      '#theme' => $pager_theme,
+      '#tags' => $tags,
+      '#element' => $this->options['id'],
+      '#parameters' => $input,
+      '#quantity' => $this->options['quantity'],
+    );
     return $output;

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/Mini.phpundefined
@@ -101,11 +101,12 @@ function render($input) {
+    $output = array(
+      '#theme' => $this->themeFunctions(),
+      '#parameters' => $input,
+      '#element' => $this->options['id'],
+      '#tags' => $tags,
+    );
     return $output;

we can just return the array... no need to set $output :)

For testing I created a new view at simplytest.me containing a pager .
It displays pager in preview so I think this patch works very well.

Here you can see views preview with pager:

has tests, and the code looks good.

no api changes, I think.

For some reason, the test-only patch in #11 didn't actually fail. This suggests that the test may need work. I'm moving this to 'needs work' so we can investigate what is wrong and fix it. We should be close though ... :)

I cannot judge how serious it is, that automatic test results in one exception.
But like before I tested patch I in the same way I described in #15and found no errors.
So in my opinion the patch works fine.

Here you find screenshot:

This is looking great and is nearly RTBC.

Ensure to tag everything as VDC, if it is related with views.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/Full.phpundefined
@@ -78,10 +78,9 @@ public function summaryTitle() {
   function render($input) {

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/Mini.phpundefined
@@ -93,7 +93,7 @@ public function postExecute(&$result) {
   function render($input) {

Here is a small follow up which is unrelated: #2036087: Add public identifier to all render methods in all views plugins in core

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/PreviewTest.phpundefined
@@ -7,6 +7,9 @@
+use SimpleXMLElement;

@@ -80,6 +83,101 @@ function testPreviewUI() {
+    $result = simplexml_load_string('<viewPreview>' . $rendered_preview . '</viewPreview>');

@@ -93,4 +191,21 @@ public function testPreviewController() {
+   * @param SimpleXMLElement $element
+   *   The element to test.
...
+  protected function assertClass(SimpleXMLElement $element, $class, $message = NULL) {

Afaik we add the root namespace directly here, so

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/PreviewTest.phpundefined
@@ -80,6 +83,101 @@ function testPreviewUI() {
+  function testPreviewWithPagersUI() {

let's add a public identifier

Nearly :)

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/PreviewTest.phpundefined
@@ -93,4 +190,21 @@ public function testPreviewController() {
+   * @param SimpleXMLElement $element

This should use "\" as well.

just #25.

Thanks!

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/PreviewTest.phpundefined
@@ -80,6 +80,113 @@ function testPreviewUI() {
+  public function testPreviewWithPagersUI() {

I am wondering whether we could reuse the test code between mini and full pagers a bit.

I tested patch #34 at simplytest.me and found pager displayed at UI.

So just from the behaviour of views this patch works fine for me:

+1 for the code

I am wondering whether there is an explicit test somewhere which checks that stuff is just rendered once. This might be hard

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/Full.phpundefined
@@ -90,13 +89,13 @@ function render($input) {
+      '#theme' => $this->view->buildThemeFunctions('pager'),

As a follow up I would suggest to use $this->themeFunctions which would require a "theme = 'pager' and register_theme = FALSE" in the annotation.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/PreviewTest.phpundefined
@@ -80,17 +80,128 @@ function testPreviewUI() {
+    // Verify elements and links to pages.
+    foreach ($elements as $page => $element) {
+      // Make element/page index 1-based.
+      $page++;
+      // We expect to find 5 elements: current page == 1, links to pages 2 and
+      // and 3, links to 'next >' and 'last >>' pages.
+      switch ($page) {
+        case 1:
+          $this->assertClass($element, 'pager-current', 'Element for current page has .pager-current class.');
+          $this->assertFalse(isset($element->a), 'Element for current page has no link.');
+          break;
+
+        case 2:
+        case 3:
+          $this->assertClass($element, 'pager-item', "Element for page $page has .pager-item class.");
+          $this->assertTrue($element->a, "Link to page $page found.");
+          break;
+
+        case 4:
+          $this->assertClass($element, 'pager-next', "Element for next page has .pager-next class.");
+          $this->assertTrue($element->a, "Link to next page found.");
+          break;
+
+        case 5:
+          $this->assertClass($element, 'pager-last', "Element for last page has .pager-last class.");
+          $this->assertTrue($element->a, "Link to last page found.");
+          break;
+
+      }
+    }

I think the whole foreach loop here is adding complexity... it is simpler and more explicit to write it like this... (also 11 lines of executable code vs 22 in the original... less to maintain :) )

    // We expect to find 5 elements: current page, links to pages 2 and
    // and 3, links to 'next >' and 'last >>' pages.
    $this->assertEqual(count($elements), 5, 'Full pager has five list items.');
    // Current page.
    $this->assertClass($elements[0], 'pager-current', 'Element for current page has .pager-current class.');
    $this->assertFalse(isset($elements[0]->a), 'Element for current page has no link.');
    // Link to page 2.
    $this->assertClass($elements[1], 'pager-item', "Element for page 2 has .pager-item class.");
    $this->assertTrue($elements[1]->a, "Link to page 2 found.");
    // Link to page 3.
    $this->assertClass($elements[2], 'pager-item', "Element for page 3 has .pager-item class.");
    $this->assertTrue($elements[2]->a, "Link to page 3 found.");
    // Link to next.
    $this->assertClass($elements[3], 'pager-next', "Element for next page has .pager-next class.");
    $this->assertTrue($elements[3]->a, "Link to next page found.");
    // Link to last.
    $this->assertClass($elements[4], 'pager-last', "Element for last page has .pager-last class.");
    $this->assertTrue($elements[4]->a, "Link to last page found.");

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/PreviewTest.phpundefined
@@ -80,17 +80,128 @@ function testPreviewUI() {
+    // Verify elements and links to pages.
+    foreach ($elements as $page => $element) {
+      // Make element/page index 1-based.
+      $page++;
+      // We expect to find 3 elements: previous page (with no link), current page == 1,
+      // and link to 'next >' page.
+      switch ($page) {
+        case 1:
+          $this->assertClass($element, 'pager-previous', 'Element for previous page has .pager-previous class.');
+          $this->assertFalse(isset($element->a), 'Element for previous page has no link.');
+          break;
+
+        case 2:
+          $this->assertClass($element, 'pager-current', 'Element for current page has .pager-current class.');
+          $this->assertFalse(isset($element->a), 'Element for current page has no link.');
+          break;
+
+        case 3:
+          $this->assertClass($element, 'pager-next', "Element for next page has .pager-next class.");
+          $this->assertTrue($element->a, "Link to next page found.");
+          break;
+
+      }
+    }

Add this too...

    // We expect to find 3 elements: previous page (with no link), current page == 1,
    // and link to 'next >' page.
    $this->assertEqual(count($elements), 3, 'Mini pager has three list items.');
    // Previous with no link because we are viewing the first page.
    $this->assertClass($elements[1], 'pager-previous', 'Element for previous page has .pager-previous class.');
    $this->assertFalse(isset($elements[1]->a), 'Element for previous page has no link.');
    // Current page.
    $this->assertClass($elements[1], 'pager-current', 'Element for current page has .pager-current class.');
    $this->assertFalse(isset($elements[1]->a), 'Element for current page has no link.');
    // Link to next.
    $this->assertClass($elements[2], 'pager-next', "Element for next page has .pager-next class.");
    $this->assertTrue($elements[2]->a, "Link to next page found.");

Really good idea!

Committed 982452f and pushed to 8.x. Thanks!