Result summary return number in format:
Displaying 1 - 10 of 4.6116860184274E+18

This occurs only when you are on the first page of results and you choose to return more than one content type at the time.

Second page returns:
Displaying 11 - 12 of 12

Steps to reproduce:
- Add a view displaying nodes
- Add the mini pager
- Add a result summary area containing the @total token (set by default)
- Add enough content for the pager to show
- See a big number and not the total rows

Proposed fix:
Show the correct number for @total.

The mini pager by default doesn't run a count query but this is overridden when @total is set, so this information is available. In order to determine if a 'next' link should be shown, some tricks were preformed on the view total which result in an incorrect number being shown.

Screenshots:
before:

after:

CommentFileSizeAuthor
#67 2798521-67-mini-pager-totals.patch5.07 KBmikeker
#67 2798521-62-67.interdiff.txt1.62 KBmikeker
#65 mini-pager-after.png12.61 KBLendude
#65 mini-pager-before.png14.74 KBLendude
#62 2798521-mini-pager-unknown-total-62.patch5.06 KBjamesrward
#60 2798521-mini-pager-tests-60.patch2.38 KBjamesrward
#57 2798521-mini-pager-tests-57.patch2.03 KBjamesrward
#52 2798521-mini-pager-unknown-total-52.patch2.69 KBjamesrward
#49 interdiff.txt795 bytesManuel Garcia
#49 result_summary_returns-2798521-49.patch3 KBManuel Garcia
#47 2798521-mini-pager-unknown-total-47.patch3.06 KBjamesrward
#40 2798521-mini-pager-unknown-total-40.patch3.03 KBjamesrward
#39 2798521-mini-pager-unknown-total-39.patch1.8 KBjamesrward
#37 2798521-mini-pager-unknown-total-37.patch1.12 KBjamesrward
#35 2798521-mini-pager-unknown-total-35.patch654 bytesjamesrward
#33 2798521-mini-pager-unknown-total-33.patch937 bytesjamesrward
#29 2798521-mini-pager-unknown-total-29.patch1.18 KBjamesrward
#25 2798521-mini-pager-unknown-total-25.patch700 bytesjamesrward
#18 2798521-mini-pager-unknown-total-18.patch42.22 KBjamesrward
#14 2798521-mini-pager-unknown-total.patch690 bytesjamesrward
#12 views.view_.all_articles.yml5.98 KBjamesrward
#7 PhpStorm64_2016-09-26_09-40-39.png35.19 KBprzemek_leczycki
#6 2798521.png17.47 KBManuel Garcia
#3 itlsweb-2015-201.png98.18 KBaklump
#3 itlsweb-2015-202.jpg75.73 KBaklump
chrome_2016-09-12_10-45-38.png5.69 KBprzemek_leczycki
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

przemek_leczycki created an issue. See original summary.

zalak.addweb’s picture

Issue tags: +views
aklump’s picture

FileSize
75.73 KB
98.18 KB

I've run into this issue as well. I've uploaded a couple screenshots. Single content type. The value of $this->total_rows is incorrect in the view. Float appears on page 2 as well. Drupal version 8.1.8. The pager does not appear on the first page.

przemek_leczycki’s picture

Status: Active » Needs work
Lendude’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.3.x-dev
Component: Views Data » views.module
Priority: Major » Normal
Status: Needs work » Active
Issue tags: -header, -summary, -views

Moving to the right queue, didn't try to reproduce. Setting priority back to normal, might be persuaded that this is major but need more info on when (and how often) this happens.

Manuel Garcia’s picture

FileSize
17.47 KB

I've been trying to reproduce this bug, but so far I'm unable.

I've got 10 articles, 13 pages created.
A view with content type filter exposed

When I select both content types on the exposed filters this is what I see:
testing 2798521

So I've been so far unable to reproduce this bug, could you provide an export of the view perhaps so we can try replicate your environment more closely?

przemek_leczycki’s picture

Thanks guys for trying to reproduce bug. I debug it and found actually place where this raised Drupal\views\Plugin\views\pager\Mini line 79 (see screenshot). This value is PHP_INT_MAX divided by 2. I don't understand a reason to put this peace of code here. Comment is meanness for me. I did a fix but I'm not completely sure about the solution. It worked for me.

  public function postExecute(&$result) {
    // In query() one more item might have been retrieved than necessary. If so,
    // the next link needs to be displayed and the item removed.
    if ($this->getItemsPerPage() > 0 && count($result) > $this->getItemsPerPage() && $this->total_items > 0) {
      array_pop($result);
      // Make sure the pager shows the next link by setting the total items to
      // the biggest possible number but prevent failing calculations like
      // ceil(PHP_INT_MAX) we take PHP_INT_MAX / 2.
      $total = PHP_INT_MAX / 2;
    }
    else {
      $total = $this->getCurrentPage() * $this->getItemsPerPage() + count($result);
    }
    $this->total_items = $total;
  }
przemek_leczycki’s picture

Version: 8.3.x-dev » 8.1.x-dev
Lendude’s picture

As far as I can tell this happens when you use the @total token in combination with the mini pager. If I remember correctly, the mini pager doesn't do a count query (by design), there is no way to tell the total number of rows in the query. Hence this weird number.

I would say that the expected behaviour would be to return something like 'unknown' or just an empty token.

przemek_leczycki’s picture

That make sense to me. Query limit is set to itemsPerPage + 1 if result is biggest than itemsPerPage that means there is another page and next button should be added (correct me if I'm wrong).
So as I think the best idea is to check whether @total is used with mini pager. If so, count query is required. If not, drupal can do one query less.

In my opinion:
View should have some kind of isResultCounted property.
if this is true, then postExecute logic can be omitted.
What do you think?

Lendude’s picture

If so, count query is required

One of the features of the mini pager is that it never runs a count query as these tend to be very expensive for large data sets. So the right behavior here is not to run a count query after all, it's to just report back that we don't have this information.

\Drupal\views\Plugin\views\pager\PagerPluginBase has a method called useCountQuery, so maybe we can utilize that somehow.

jamesrward’s picture

Here's a views export (is that still the right term?) that shows the issue with Devel generated articles.

https://www.drupal.org/files/issues/views.view_.all_articles.yml

jamesrward’s picture

Assigned: Unassigned » jamesrward
jamesrward’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Active » Needs review
FileSize
690 bytes

Here's the easiest fix I can see. The mini-pager sending a high number for total when it doesn't know the value is probably due for a refactor but this should fix this issue and catch any other pagers that return false for useCountQuery.

jamesrward’s picture

Issue tags: +Dublin2016

Status: Needs review » Needs work

The last submitted patch, 14: 2798521-mini-pager-unknown-total.patch, failed testing.

Lendude’s picture

@jamesrward thanks for working on this, I think this change is a big improvement over showing a big float.

+++ b/core/modules/views/src/Plugin/views/area/Result.php
@@ -102,7 +102,7 @@ public function render($empty = FALSE) {
+    $replacements['@total'] = $this->view->pager->useCountQuery() ? $total : "Unknown";

The string needs to be passed through $this->t() though.

jamesrward’s picture

String passed through $this-t(). Thanks for the quick feedback @Lendude.

przemek_leczycki’s picture

Sorry guys for saying this but this is not what I expected. Of course this resolve the issue but please think the business way. What would I say to my client if he require to use mini pager and total results in view header. He don't pay much attention to performance. He just want to have this task completed. Setting "unknown" is not a complete solution. For me better solution is to use even another count query. Maybe with additional warning that using @total with mini pager is less efficient.

jamesrward’s picture

Status: Needs work » Needs review

@przemek_leczycki I agree but I would suggest that what you are describing is a separate issue from the large number currently displayed. Let's make a new issue along the lines of "mini-pager should not allow @total token" and get this improvement into core. Per #11 mini-pager won't ever know the total so it makes sense to exclude it from the view. If the business needs the total then they will need to use a different pager, removing it as an option from mini-pager should make that more obvious.

przemek_leczycki’s picture

@jamesrward thank you for taking a conversation.
I fully understand the idea of making it countless but why to limit functionality while solution is within reach. It's effortless to check if

core/modules/views/src/Plugin/views/area/Result.php
$this->options['content'];

contain '@total' while pager instance is Mini. If so, do count query. Then the count query is done let's say "on users demand". I know it's though to push my solution. I can realize how much speech was done during design phase on this peace of code but would be fantastic if we'll develop something and not limit it. For 90% of people this solution would be really efficient. For rest 10% it's a matter of removing @total tag to make performance even better. Then it make sense to me to raise a task as a improvement. "mini-pager should not allow @total token" is just another bug caused by this issue.

Status: Needs review » Needs work

The last submitted patch, 18: 2798521-mini-pager-unknown-total-18.patch, failed testing.

jamesrward’s picture

@przemek_leczycki isn't the full-pager already providing the functionality you need while allowing the mini-pager to remain safe for sites with large datasets?

Lendude’s picture

@przemek_leczycki making the mini pager switch between not using and using a count query because of the use of a token somewhere is way too unclear. I feel we need clear parameters for the core plugins. Full pager: count, Mini pager: no count.

You want to use @total? Use the full pager.

You have a client that demands the mini pager and demands a count? Extend the mini pager, override useCountQuery to return TRUE and use your custom pager (and charge your customer for the custom work).

@jamesrward your last patch seems to include the highwater changes to migrate? Patch size went from 690 bytes to 42 KB :) Also, do you want to take a look at the failing tests?

jamesrward’s picture

@Lendude here's a clean patch. I'll see what I can do about the failing tests.

Manuel Garcia’s picture

+++ b/core/modules/views/src/Plugin/views/area/Result.php
@@ -102,7 +102,7 @@ public function render($empty = FALSE) {
+    $replacements['@total'] = $this->view->pager->useCountQuery() ? $total : $this->t("Unknown");

I think displaying Unknown to the user is not going to be very friendly, nor add any value, although it would be technically correct.
@total perhaps should just be empty if using a minipager?
This is just an opinion of course, and yes it would be an improvement over showing a crazy float.

From a UX perspective, when using the minipager we should just display Displaying 20 - 25.

jamesrward’s picture

@Manuel Garcia agree completely. Getting the tests to pass is going to require some more complicated logic anyway so I will look at making those improvements as part of the refactor.

Lendude’s picture

Displaying nothing or Unknown tries to signal the same thing to the site builder: "this is a useless token, remove it". I'm fine with either. Leaving it empty is more inline with what unresolvable tokens usually do but might lead to more 'why is this not rendering anything?' questions.

some more discussion on what to do with unresolvable tokens in views for those interested #2804375: Exception thrown when using [view:url] token when there is no display with a URL in the view.

jamesrward’s picture

OK so it looks like this issue was introduced by the postExecute method in Mini.php. Removing that method gets the mini pager working with the correct total based on count($this->view->result) per this line in Result.php:

    $total = isset($this->view->total_rows) ? $this->view->total_rows : count($this->view->result);

Here is the code I am removing:


public function postExecute(&$result) {
    // In query() one more item might have been retrieved than necessary. If so,
    // the next link needs to be displayed and the item removed.
    if ($this->getItemsPerPage() > 0 && count($result) > $this->getItemsPerPage()) {
      array_pop($result);
      // Make sure the pager shows the next link by setting the total items to
      // the biggest possible number but prevent failing calculations like
      // ceil(PHP_INT_MAX) we take PHP_INT_MAX / 2.
      $total = PHP_INT_MAX / 2;
    }
    else {
      $total = $this->getCurrentPage() * $this->getItemsPerPage() + count($result);
    }
    $this->total_items = $total;
  }

I can't see any useful info on these lines from the git log so I don't fully understand the problem they were attempting to solve. Submitting a patch without that method to see what the testbot says. @Lendude any opinion on the performance implications of have count($this->view->result) fire when using mini-pager?

jamesrward’s picture

Status: Needs work » Needs review

The last submitted patch, 25: 2798521-mini-pager-unknown-total-25.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 29: 2798521-mini-pager-unknown-total-29.patch, failed testing.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
937 bytes

Ok so clearly we need the postExecute for some cases but not the one we are hitting in this issue. Let's see what the test bot thinks of this.

Status: Needs review » Needs work

The last submitted patch, 33: 2798521-mini-pager-unknown-total-33.patch, failed testing.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
654 bytes

OK. So if per #2804375: Exception thrown when using [view:url] token when there is no display with a URL in the view we should just return an empty string than this is probably the shortest route to success here. Obviously it would be nice to use exceptions to test all the tokens and that is likely the long-term solution here.

I'm also very concerned that count($this->view->result) is giving an accurate count of total rows when using the mini-pager. That seems to indicate we are taking the performance hit that we are trying to avoid.

Status: Needs review » Needs work

The last submitted patch, 35: 2798521-mini-pager-unknown-total-35.patch, failed testing.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
1.12 KB

This one should pass and yield an empty value for @total. Any suggestions for a better way to check for the Mini pager would be appreciated. I didn't want to introduce a dependency on the Mini pager class so this seemed the least intrusive.

I can't wipe out the $total variable or the $output will be blank so I have to do this at the end of the replacements and deal directly with the @total token. I'm not sure why $output is wrapped in if(!empty($total)) but I assume there is a good reason.

Here's the pertinent addition:

if (get_class($this->view->pager) === 'Drupal\views\Plugin\views\pager\Mini') {
  $replacements['@total'] = "";
} else {
  $replacements['@total'] = $total;
}
jamesrward’s picture

Status: Needs review » Needs work

I think we can do a better job here with a combination of usePager() and useCountQuery(). I'll see what I can come up with.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
1.8 KB

This uses usePager true and useCountQuery false to establish we have a pager but no total. The ResultTest needed a small modification to allow for the presence of these methods but I think that is a long-term gain.

I also changed the final output check to use count($this->view->result) instead of !empty($total). We shouldn't try to render anything if we don't have any results to show and clearly $total is a less reliable indicator.

jamesrward’s picture

So with this passing I suggest a bit more refactoring of Mini.php. Using PHP_MAX_INT / 2 feels sloppy and creates an edge-case where the pager won't page past PHP_MAX_INT / 2 records. A high number I know but it could theoretically handle larger datasets with some better logic here. We just need to increment the total_records by the items per-page + 1.

Long-term I'd like to look at other paging solutions and come up with a better way to handle all of this. My initial thoughts would be to create a buildPager() method and inject the pager object into it when usePager() is true.

jamesrward’s picture

Worth considering changing the default result summary as the default pager doesn't support it. Just Displaying @start - @end would probably make more sense. Either that or change the default pager to Full so all the tokens are available.

Status: Needs review » Needs work

The last submitted patch, 40: 2798521-mini-pager-unknown-total-40.patch, failed testing.

jamesrward’s picture

Status: Needs work » Needs review

@Lendude in the name of getting the reported behaviour fixed in core I'm happy to just go with #39 for now and I can create a separate issue for the stuff I tried to solve in #40

dawehner’s picture

@jamesrward
Thank you for the contributions. Conceptually I believe this is a bug of the result summary, so changing mini pager is another context. Do you mind open up a new issue for the changes you proposed there?

On top of that I believe we should do the following:

  • Figure out whether @total appears in the summary string
  • In case it does, set $view->get_total_rows
  • Warn the user about potential performance issues
Manuel Garcia’s picture

We're already setting checking if @total appears in the summary string and $view->get_total_rows on Drupal\views\Plugin\views\area\Result::query().

jamesrward’s picture

@Manuel Garcia I noticed that. Looks like all the work for this will happen in Mini.php and we possibly have some logic duplication with useCountQuery(). I'll get going on a patch and see what I can come up with.

jamesrward’s picture

@dawehner let's see what the test bot thinks of this. We'll need to look at performance implications of the pager when @total isn't used if we go this route. useCountQuery() has to be TRUE for this to work.

dawehner’s picture

  1. +++ b/core/modules/views/src/Plugin/views/pager/Mini.php
    @@ -47,14 +47,20 @@ public function summaryTitle() {
    +    // Only modify the query if we don't want to do a total row count
    +    if ($this->view->get_total_rows) {
    +      //$this->view->query->setLimit("");
    +    }
    +    else {
    

    I like this change. Can we remove the if/else and just have a single if?

  2. +++ b/core/modules/views/src/Plugin/views/pager/Mini.php
    @@ -62,26 +68,28 @@ public function query() {
    +    return TRUE;
       }
    

    Given the code in Sql.php is

            if ($view->pager->useCountQuery() || !empty($view->get_total_rows)) {
              $view->pager->executeCountQuery($count_query);
            }

    we should check here whether we want a count query or not.

Manuel Garcia’s picture

Had a bit of time at the airport back from dublin, unfortunately not too much, anyway addressing #48.1

jamesrward’s picture

@Manuel Garcia I just rolled almost the exact same patch at the Dublin airport then spotted yours as I'm stuck on the tarmac. Only difference was I carried the same comment from the query modification into the result modification:

  public function postExecute(&$result) {
    // Only modify the result if we didn't do a total row count
    if (!$this->view->get_total_rows) {

Not a big deal for sure so I won't bother submitting until we hear some feedback on #49

Manuel Garcia’s picture

@jamesrward lol cool! It makes sense to carry on the comment line =)

jamesrward’s picture

Here's another kick at this. I think this solutions covers all the key points here:

  • Provides an accurate @total for the mini-pager
  • Only uses the count-query when @total is used by keeping useCountQuery() FALSE
  • Reduce the complexity of the postExecute code, now just 4 lines
  • Remove the usage of PHP_MAX_INT/2 increasing our theoretical result limit

Here's the key bit of code in postExecute():

    if (!$this->view->get_total_rows) {
      $this->total_items = $this->getCurrentPage() * $this->getItemsPerPage() + count($result);
      if ($this->getItemsPerPage() > 0 && count($result) > $this->getItemsPerPage()) {
        array_pop($result);
      }
    }

@dawehner as for the need to include a performance warning I'm not really sure where to go with that. I started looking at doing something in the views_ui but the more I think about it I feel like just about any decision in views creation could have performance implications. I think the best we can do here is write clear code with good comments for those who may need to understand it when investigating performance concerns.

Lendude’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I like how the fix looks now. Now we need tests.

+++ b/core/modules/views/src/Plugin/views/pager/Mini.php
@@ -69,19 +72,15 @@ public function useCountQuery() {
-      // Make sure the pager shows the next link by setting the total items to
-      // the biggest possible number but prevent failing calculations like
-      // ceil(PHP_INT_MAX) we take PHP_INT_MAX / 2.

Does the next link still show in all scenarios with this taken out? Is there sufficient test coverage for the mini pager for that currently?

jamesrward’s picture

@Lendude I think we are pretty well covered by tests here already. The only additional scenario I would like to see is confirming the total rows are not calculated when $view->get_total_rows is not set. There is something close in the tests already:

$this->assertIdentical($view->get_total_rows, NULL, 'The query was not forced to calculate the total number of results.');

but it's dealing with the 1 item edge-case instead of a large rowset. I'll see if I can come up with something.

As for the pager continuing to function without PHP_MAX_INT/2, I'm very confident that this solution will provide exactly the same, slightly improved functionality. The goal of the arbitrarily high number was to keep the pager showing next links while we still have results beyond this page, the query that looks for one extra row is providing the same thing but with better information. By using that query result we are providing the page builder with exactly as many rows as we are currently aware of. If we added a test that tried to show PHP_MAX_INT/2 + 1 record it would fail with the old code, with the new code this should pass. I'm not advocating adding such a test as it will look pretty strange in there if this patch is accepted.

Lendude’s picture

@jamesrward at the very least we need a test that is failing with the current HEAD and that passes with the fix applied, see here. Something that checks and verifies the output of @total for the mini pager I'd say?

Like I said, also just making sure we have sufficient existing test coverage for the mini pager 'next' link that we know we aren't breaking anything. And if we see a hole in the coverage, close the hole.

I'm very confident that this solution will provide exactly the same, slightly improved functionality

We need tests to back that statement up :)

jamesrward’s picture

@Lendude Absolutely. I'll roll some failing tests and we can go from there.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
2.03 KB

Here's a first crack at a regression test. I added a display to the view with the default result summary and check the values on the first and last page. The first page test should fail and the last page test should pass. With #52 both should pass.

I wanted to also do something more explicit along the lines of:

    $view = Views::getView('test_mini_pager_total');
    $this->executeView($view);
    $this->assertIdentical($view->get_total_rows, TRUE, 'The query was set to calculate the total number of rows.');
    $this->assertEqual(count($this->nodes), $view->total_rows, 'The total row count is equal to the number of nodes.');

but I don't know how to specify the display instead of grabbing the entire view so this doesn't work and it seems excessive to create a new view import just for this test.

As for specifically testing the problem with PHP_MAX_INT/2, I think it would be irresponsible to have the testbot create PHP_MAX_INT/2+1 nodes. If we accept that PHP_MAX_INT/2 is just a number x and that at x+1 nodes we will have a failure than all you need to do is replace PHP_MAX_INT/2 with 19 and the current test suite will show the failures you would get with PHP_MAX_INT/2+1 nodes in the resultset.

Status: Needs review » Needs work

The last submitted patch, 57: 2798521-mini-pager-tests-57.patch, failed testing.

Lendude’s picture

Issue tags: -Needs tests

but I don't know how to specify the display instead of grabbing the entire view

$view->setDisplay($display_id); should do the trick.

Tests looks good and I agree that testing for PHP_MAX_INT/2 is ridiculous. This shows the bug just fine.

\Drupal\views\Tests\Plugin\MiniPagerTest::testMiniPagerRender has nice coverage of the 'next' link, so with that coming back green we appear not to be breaking anything.

Ok so now we just need a patch with both test and fix combined, nice work!

jamesrward’s picture

Status: Needs work » Needs review
FileSize
2.38 KB

$view->setDisplay($display_id); should do the trick.

Cool. Here's two extra test cases to check if we are set to calculate total_rows and if that calculation is correct. This should add one more failing test without #52. Once the testbot is done with this I will roll it all into a single patch.

Status: Needs review » Needs work

The last submitted patch, 60: 2798521-mini-pager-tests-60.patch, failed testing.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
5.06 KB

And here's the fix and the tests rolled together. Should be no failing tests here.

Manuel Garcia’s picture

Excellent work @jamesrward - +1 on RTBC

dawehner’s picture

Conceptually this fix looks really nice!

Lendude’s picture

Title: Result summary returns float number » Views result summary returns float number when using the mini pager
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
14.74 KB
12.61 KB

Yeah this looks nice.

Updated the issue summary to reflect the fix and the research done into this.

przemek_leczycki’s picture

What I can say :) Thank you guys. I really appreciate your work. And I'm really happy about final result. Thumbs up!

mikeker’s picture

+++ b/core/modules/views/src/Plugin/views/pager/Mini.php
@@ -47,14 +47,17 @@ public function summaryTitle() {
+        // Increase the items in the query in order to be able to find out whether

@@ -69,19 +72,15 @@ public function useCountQuery() {
+      // query() checks if we need a next link by setting limit 1 record past this page
+      // If we got the extra record we need to remove it before we render the result

Super-minor coding standards nitpicks: > 80 chars, also missing a period on the second comment. Leaving at RTBC since these are only changes to code comments.

Manuel Garcia’s picture

Thanks @mikeker for that.

  • catch committed 5c9d1a9 on 8.3.x
    Issue #2798521 by jamesrward, Manuel Garcia, mikeker, Lendude,...

  • catch committed 30b82f3 on 8.2.x
    Issue #2798521 by jamesrward, Manuel Garcia, mikeker, Lendude,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

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