Running latest version of 8.x-dev;

- enabled the View which overrides /taxonomy/term/%
- updated pager settings and entered 0 for "Items per page"
- got Warning: Division by zero in pager_default_initialize() (line 123 of core/includes/pager.inc). on term page, zero nodes displayed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

olli’s picture

Component: base system » views.module
Status: Active » Needs review
Issue tags: -view, -pager, -zero +Needs tests, +VDC
FileSize
2.66 KB

I saw this too: Install drupal, create an article, change front page pager to "Mini" and set "Items per page" to 0. Visit front page, see the warning and "No front page content has been created yet." message.

Status: Needs review » Needs work

The last submitted patch, vdc-1958470-1.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.53 KB
1.88 KB

I guess it's not so common to use the mini pager when you want to show all items, but it should be possible to expose the "- All -" items per page option.

olli’s picture

FileSize
5.63 KB

Here is an alternative that removes pager_default_initialize() and uses updatePageInfo() which is called also when view result is cached.

Status: Needs review » Needs work

The last submitted patch, vdc-1958470-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

The fix and the new test looks fine. Did someone tried that with the full pager and maybe even on Drupal 7 views?

alexpott’s picture

@dawehner can you make it clear if you are rtbc-ing #3 or #4?

olli’s picture

#4: vdc-1958470-4.patch queued for re-testing.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/PagerPluginBase.phpundefined
@@ -183,13 +183,12 @@ public function executeCountQuery(&$count_query) {
-    $this->updatePageInfo();
     return $this->total_items;

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -1431,9 +1431,8 @@ function execute(ViewExecutable $view) {
-        if ($view->pager->useCountQuery() || !empty($view->get_total_rows)) {
-          $view->total_rows = $view->pager->getTotalItems();
-        }
+        $view->pager->updatePageInfo();
+        $view->total_rows = $view->pager->getTotalItems();

I like that updatePageInfo() is moved away from executeCountQuery!

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/Mini.phpundefined
@@ -89,7 +89,7 @@ public function postExecute(&$result) {
+    $this->total_items = $total;

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/MiniPagerTest.phpundefined
@@ -105,7 +112,7 @@ public function testMiniPagerRender() {
+    $this->assertIdentical($view->total_rows, 1, 'The query returned the total number of rows.');

I'm not sure about that line, as 1 is just the wrong number. We have no clue what the total amount of items is of a mini pager, so there should be no lie about it?

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/MiniPagerTest.phpundefined
@@ -105,7 +112,7 @@ public function testMiniPagerRender() {
+    $this->assertIdentical($view->total_rows, 1, 'The query returned the total number of rows.');

Well, actually the pager calculated the total items, it was not returned from the query. Let's improve the assertion message a bit.

olli’s picture

FileSize
803 bytes
5.64 KB

Thanks! Here is an improved assertion message. I'd be equally fine removing the line, since I'm not sure if that assertion is really relevant anymore after this patch, but 1 is the number to expect.

dawehner’s picture

Are you really sure that the number 1 is expected. The number is somehow random, because like written before we have no clue what the actual value should be. It seems to be wrong just from the DX side.

olli’s picture

I'm pretty sure it is 1, there is (just few lines above the assertion):

    // Remove all items beside 1, so there should be no links shown.
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.php
@@ -1431,9 +1431,8 @@ function execute(ViewExecutable $view) {
-        if ($view->pager->useCountQuery() || !empty($view->get_total_rows)) {
-          $view->total_rows = $view->pager->getTotalItems();
-        }

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.php
@@ -1431,9 +1431,8 @@ function execute(ViewExecutable $view) {
+        $view->total_rows = $view->pager->getTotalItems();

If we want to keep $view->total_rows NULL, we should revert this change. Should I work on that or do you have better suggestions?

dawehner’s picture

I always thought that the total rows is the amount of items which are available, but maybe this is not the case if I read the documentation on ViewExecutable, which also simply could be just broken :) URGS

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Well, let's keep this then.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.