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:
Comment | File | Size | Author |
---|---|---|---|
#67 | 2798521-67-mini-pager-totals.patch | 5.07 KB | mikeker |
#67 | 2798521-62-67.interdiff.txt | 1.62 KB | mikeker |
#65 | mini-pager-after.png | 12.61 KB | Lendude |
#65 | mini-pager-before.png | 14.74 KB | Lendude |
#62 | 2798521-mini-pager-unknown-total-62.patch | 5.06 KB | jamesrward |
Comments
Comment #2
zalak.addweb CreditAttribution: zalak.addweb commentedComment #3
aklump CreditAttribution: aklump at In the Loft Studios commentedI'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.
Comment #4
przemek_leczycki CreditAttribution: przemek_leczycki commentedComment #5
LendudeMoving 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.
Comment #6
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedI'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:
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?
Comment #7
przemek_leczycki CreditAttribution: przemek_leczycki commentedThanks 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.Comment #8
przemek_leczycki CreditAttribution: przemek_leczycki commentedComment #9
LendudeAs 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.
Comment #10
przemek_leczycki CreditAttribution: przemek_leczycki commentedThat 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?
Comment #11
LendudeOne 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.Comment #12
jamesrward CreditAttribution: jamesrward as a volunteer commentedHere'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
Comment #13
jamesrward CreditAttribution: jamesrward as a volunteer commentedComment #14
jamesrward CreditAttribution: jamesrward as a volunteer commentedHere'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.
Comment #15
jamesrward CreditAttribution: jamesrward as a volunteer commentedComment #17
Lendude@jamesrward thanks for working on this, I think this change is a big improvement over showing a big float.
The string needs to be passed through $this->t() though.
Comment #18
jamesrward CreditAttribution: jamesrward as a volunteer commentedString passed through $this-t(). Thanks for the quick feedback @Lendude.
Comment #19
przemek_leczycki CreditAttribution: przemek_leczycki commentedSorry 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.
Comment #20
jamesrward CreditAttribution: jamesrward as a volunteer commented@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.
Comment #21
przemek_leczycki CreditAttribution: przemek_leczycki commented@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
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.
Comment #23
jamesrward CreditAttribution: jamesrward as a volunteer commented@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?
Comment #24
Lendude@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?
Comment #25
jamesrward CreditAttribution: jamesrward as a volunteer commented@Lendude here's a clean patch. I'll see what I can do about the failing tests.
Comment #26
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedI 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.
Comment #27
jamesrward CreditAttribution: jamesrward as a volunteer commented@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.
Comment #28
LendudeDisplaying 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.
Comment #29
jamesrward CreditAttribution: jamesrward as a volunteer commentedOK 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:
Here is the code I am removing:
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?
Comment #30
jamesrward CreditAttribution: jamesrward as a volunteer commentedComment #33
jamesrward CreditAttribution: jamesrward as a volunteer commentedOk 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.
Comment #35
jamesrward CreditAttribution: jamesrward as a volunteer commentedOK. 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.
Comment #37
jamesrward CreditAttribution: jamesrward as a volunteer commentedThis 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:
Comment #38
jamesrward CreditAttribution: jamesrward as a volunteer commentedI think we can do a better job here with a combination of usePager() and useCountQuery(). I'll see what I can come up with.
Comment #39
jamesrward CreditAttribution: jamesrward as a volunteer commentedThis 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.
Comment #40
jamesrward CreditAttribution: jamesrward as a volunteer commentedSo 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.
Comment #41
jamesrward CreditAttribution: jamesrward as a volunteer commentedWorth 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.
Comment #43
jamesrward CreditAttribution: jamesrward as a volunteer commented@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
Comment #44
dawehner@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:
$view->get_total_rows
Comment #45
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedWe're already setting checking if @total appears in the summary string and
$view->get_total_rows
on Drupal\views\Plugin\views\area\Result::query().Comment #46
jamesrward CreditAttribution: jamesrward as a volunteer commented@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.
Comment #47
jamesrward CreditAttribution: jamesrward as a volunteer commented@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.
Comment #48
dawehnerI like this change. Can we remove the if/else and just have a single if?
Given the code in
Sql.php
iswe should check here whether we want a count query or not.
Comment #49
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedHad a bit of time at the airport back from dublin, unfortunately not too much, anyway addressing #48.1
Comment #50
jamesrward CreditAttribution: jamesrward as a volunteer commented@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:
Not a big deal for sure so I won't bother submitting until we hear some feedback on #49
Comment #51
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented@jamesrward lol cool! It makes sense to carry on the comment line =)
Comment #52
jamesrward CreditAttribution: jamesrward as a volunteer commentedHere's another kick at this. I think this solutions covers all the key points here:
Here's the key bit of code in postExecute():
@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.
Comment #53
LendudeI like how the fix looks now. Now we need tests.
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?
Comment #54
jamesrward CreditAttribution: jamesrward as a volunteer commented@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.
Comment #55
Lendude@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.
We need tests to back that statement up :)
Comment #56
jamesrward CreditAttribution: jamesrward as a volunteer commented@Lendude Absolutely. I'll roll some failing tests and we can go from there.
Comment #57
jamesrward CreditAttribution: jamesrward as a volunteer commentedHere'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:
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.
Comment #59
Lendude$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!
Comment #60
jamesrward CreditAttribution: jamesrward as a volunteer commentedCool. 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.
Comment #62
jamesrward CreditAttribution: jamesrward as a volunteer commentedAnd here's the fix and the tests rolled together. Should be no failing tests here.
Comment #63
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedExcellent work @jamesrward - +1 on RTBC
Comment #64
dawehnerConceptually this fix looks really nice!
Comment #65
LendudeYeah this looks nice.
Updated the issue summary to reflect the fix and the research done into this.
Comment #66
przemek_leczycki CreditAttribution: przemek_leczycki commentedWhat I can say :) Thank you guys. I really appreciate your work. And I'm really happy about final result. Thumbs up!
Comment #67
mikeker CreditAttribution: mikeker as a volunteer commentedSuper-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.
Comment #68
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @mikeker for that.
Comment #71
catchCommitted/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!