Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The [view:page-count]
token incorrectly returns 1 when there are several pages of results for a given view.
To reproduce:
- drush si standard -y --account-pass=admin
- drush en -y devel_generate
- drush generate-content 50
- Log in as admin and navigate to /admin/structure/views/view/frontpage
- Add a "Global: text area" handler to the "Header" area with the following text:
[view:current-page] out of [view:page-count]
- Save the view
- Navigate to the site home page.
1 out of 1
will appear in the header area, however three pages of Views results show in the pager at the bottom of the page.
Comment | File | Size | Author |
---|---|---|---|
#85 | interdiff-2572355-83-85.txt | 715 bytes | yogeshmpawar |
#85 | 2572355-85-page-count-token.patch | 6.27 KB | yogeshmpawar |
#83 | interdiff-2572355-80-83.txt | 1.64 KB | jamesrward |
#83 | 2572355-83-page-count-token.patch | 6.27 KB | jamesrward |
#80 | interdiff-2572355-78-80.txt | 760 bytes | jamesrward |
Comments
Comment #2
mikeker CreditAttribution: mikeker as a volunteer commentedThe calculation for this token was using $view->results which no longer returns all the results for a given view.
Comment #3
olli CreditAttribution: olli commentedFix looks good.
Maybe out of scope but if items per page is 0 (no limit) then page-count is 1?
Comment #4
mikeker CreditAttribution: mikeker as a volunteer commented@olli, "needs tests" -- Agreed! Should go to NW when the testbot gets back to us... Must be overworked based on all the DC Barcelona sprinting!
I think it's correct since there will be at least one page of results -- even if it's the empty result text. Or do you feel that a result of zero rows should equal zero page-count?
Comment #5
dawehnerMHH, yeah we need tests first, one for a normal pager, one for a mini pager.
Mh, total rows is not always set, like for the mini pager
Comment #6
mikeker CreditAttribution: mikeker as a volunteer commentedStupid mini-pager... :)
Comment #7
mikeker CreditAttribution: mikeker as a volunteer commentedAfter playing with this for a bit, it looks like [view:total-rows] is also incorrect, showing the number of items on a given page rather than the total number of results in the query.
Since the test in question uses
(string) $view->total_rows
as it's expected value, I suspect this may be more than just bogus tokens...Comment #8
mikeker CreditAttribution: mikeker as a volunteer commentedAnd, as @dawehner mentioned, the mini-pager doesn't calculate the total number of rows. And it looks like it's supposed to be that way...
core/modules/views/src/Tests/Plugin/MiniPagerTest.php:116
So, we now have a token that may or may not be set. Do we re-execute the view if there are tokens involved? Only if the ones requiring a total row count are involved? Or do we force the mini-pager to calc the number of rows in the query? Or...?
Comment #9
dawehnerSounds horrible, sorry
Comment #10
mikeker CreditAttribution: mikeker as a volunteer commentedAgreed!
Here's the tests-only patch. We didn't have any existing test coverage for tokens and the mini-pager.
Still working on the correct solution with regards to the mini pager. My preference would be to have it do the
total_rows
query regardless of tokens being used. Is there that big a performance hit for adding that to the query?Comment #11
mikeker CreditAttribution: mikeker as a volunteer commentedThis export was made by round-tripping the existing test_tokens view. I'm not sure why the base_field changed, though.
Comment #12
mikeker CreditAttribution: mikeker as a volunteer commentedThis patch forces the mini-pager to query the total number of rows. Also tweaks some tests to deal with string -> int conversion needed for
assertIdentical()
as shown in the interdiff.Comment #13
mikeker CreditAttribution: mikeker as a volunteer commented... and the files...
Comment #16
vadim.hirbu CreditAttribution: vadim.hirbu at FFW commentedTested on latest stable version. Works like a charm.
Comment #17
dawehnerSorry, but the entire point of the mini pager is to not run those queries. It is a limit you need to accept. Maybe we could check the tokens and not expose them, when you have no count query.
Comment #18
mikeker CreditAttribution: mikeker as a volunteer commentedYeah, I had a feeling you would say that! :)
If that is a hard rule, then we should add wording explaining the lack of real values for anything involving total row count with the mini-pager. I think trying to remove these from the available tokens would be overly complicated.
I'll look into this more early next week.
Comment #19
mikeker CreditAttribution: mikeker as a volunteer commentedFixes #17.
I would like to revisit the mini-pager optimization that removes the count query from the mix, but that's not in scope for this issue. Instead, this patch adds text to the global tokens
detail
element that let's users know that some tokens are not available if the mini-pager is used.The warning text is only shown if a mini-pager is enabled -- curious if others feel it should be shown all the time...
Comment #20
mikeker CreditAttribution: mikeker as a volunteer commentedAlso, this doesn't remove the tokens if they are unavailable -- figured it would be better to show a
0
than[view:total-rows]
.Comment #22
mikeker CreditAttribution: mikeker as a volunteer commentedUgh... That's what I get for not properly reading the patch I'm trying to fix. I missed the tests and logic that was added when the earlier patch allowed the mini-pager to query for the row count.
This should be the proper fix for #17.
Comment #24
mikeker CreditAttribution: mikeker as a volunteer commentedOh, fer cryin' out loud!
Time to go to sleep.
Comment #25
dawehnerThis is a 100% strict role. No way around that. This is a major performance improvement for Drupal potentially.
IMHO we should check for
useCountQuery()
or
[view:page-count]
, will not be available in displays using the mini pager.');Appending a description looses safeness.
Comment #26
mikeker CreditAttribution: mikeker as a volunteer commentedFrom #25:
Done. Also renamed the local var to be consistent.
Didn't realize that -- I figured everything was escaped when rendered. Is that not the case?
Or did you mean the
.=
bit? See attached -- overrides, instead of append to, the#description
.Comment #28
dawehnerAny idea yet, why its broken?
Comment #29
mikeker CreditAttribution: mikeker as a volunteer commentedMight be that I was calling
useCountQuery
on the display instead of the pager...Also I had turned the the
$has_count
logic around when refactoring. This patch correct that.Comment #30
LendudeThis overlapped with #2510076: The [view:page-count] token should never return 0 so this will need a reroll.
Comment #31
mikeker CreditAttribution: mikeker as a volunteer commentedRerolled. Thanks for the pointer to that issue, @Lendude. Unfortunately, the fix there is incorrect -- I'll post a comment shortly.
Comment #33
DuaelFrIt looks really good!
Just one question:
Isn't it out of the scope of this issue?
If it tests something that's missing in all the other tests, we should add these expectations in another issue to help maintainers to focus on the problem we are solving here.
Comment #34
mikeker CreditAttribution: mikeker as a volunteer commented@DuaelFr, thank you for the review! From #33:
I don't believe so. Currently there are no mini-pager tests. This issue highlights the differences in how the mini-pager acts vs the normal pager, so adding tests for the mini-pager seems in scope. Yes, I am adding mini-pager tests for tokens that are not technically part of this issue but it seems silly to add some of the existing pager token tests and leave the rest to a followup issue.
But if maintainers feel otherwise, I'm happy to pull them and open a followup. I'll ping @dawehner in IRC to see if he can weigh in on this.
Comment #35
jibranIs this the only fix?
Comment #36
mikeker CreditAttribution: mikeker as a volunteer commented@jibran, functionally, yes. There is also warning to users in TokenizeAreaPluginBase that shows in the UI.
Comment #37
jibranThanks @mikeker
or
[view:page-count]
, will not be available in displays using optimized pagers such as the mini pager.');I'm not sure about this change but other then this seems RTBC. We are in a string freeze so I don't know whether we can add this or not.
Comment #38
dawehnerWe can add new strings, we just can't edit existing ones for 8.0.x.
Comment #41
smk-ka CreditAttribution: smk-ka commentedRerolled.
Comment #43
smk-ka CreditAttribution: smk-ka commentedSorry, real patch this time.
Comment #44
smk-ka CreditAttribution: smk-ka commentedComment #45
LendudeAm I overlooking something or is there no test for this logic yet? If not, than that needs a test right?
Comment #46
mikeker CreditAttribution: mikeker as a volunteer commented@Lendude, Sort of... We test for the lack of tokens involving the total row count in
testTokenReplacementWithMiniPager
, but we should probably test for the description text as well.I'll add that.
Comment #47
mikeker CreditAttribution: mikeker as a volunteer commentedTests only patch includes a reroll of the tests-only patch in #31, plus new tests to check that the description warning appears in views with a mini-pager display and does not in a view that only has full pager displays.
Also, bumping to 8.2.x as this includes a string change.
Sorry that took so long...
Comment #49
DuaelFrThank you very much @mikeker for your patch.
I made a bunch of manual testing and it's working as expected with full pagers.
On mini pagers, the warning is properly displayed but if you still use one of these tokens, they output the maximum signed integer of the system (
PHP_INT_MAX / 2
). Given the warning, I expected them to output nothing instead. I wonder if we should add a condition inviews_tokens()
to only replace the tokens if the pager uses a countQuery.I also took the time to review the code and I found out a lot of things that seems out of scope to me. Is it wanted?
Looks out of scope.
Looks out of scope.
Looks out of scope.
Looks out of scope.
Comment #50
mikeker CreditAttribution: mikeker as a volunteer commented@DuaelFr, Thank you for the review!
re: out of scope changes. #1 and #2 come from round-tripping the test view through config management -- basically things that were added to Views since the last time this view was last exported. Since the test view is changing, I would argue that these are in scope for this issue. #3 and 4, yeah, the
format_string
toFormattableMarkup
is out of scope. I'll update the patch shortly.re: Whether to not replace tokens if the pager uses a
countQuery
. I remember there being a reason why we couldn't do that, but I can't recall it at the moment. I'll investigate...Comment #51
mikeker CreditAttribution: mikeker as a volunteer commentedTwo interdiffs: one for the out-of-scope changes and another for functionality change.
In this patch, if the current display is not using a count query, I don't replace the page-count and total-rows tokens at all. Eg:
[view:page-count]
returns "[view:page-count]". We could return an empty stings instead. Is there somewhere else in core where tokens can go in and out of scope?Comment #53
jamesrward CreditAttribution: jamesrward as a volunteer commentedGiven the outcome of #2798521: Views result summary returns float number when using the mini pager I think this needs a re-work to use the count query on demand. The new logic in the mini-pager should make this easier, no more PHP_MAX_INT/2 calculations and some better test-coverage in the mini-pager. I'm happy to take assignment of this issue and try to see it through but if @mikeker wants to keep rolling in that new direction that's great too.
Comment #54
jamesrward CreditAttribution: jamesrward as a volunteer commentedSo here's a patch for discussion using the on-demand total functionality of the mini-pager. If [view:page-count] or [view:total-rows] are used we set view->get_total_rows = TRUE; If everyone is ok with this approach I can start reworking the tests.
This also exposed another issue with the Result area and the mini-pager which I will create a separate issue for, but it makes me wonder why we need the Result area at all when we can accomplish the same thing with the Text area and tokens.
Comment #56
DuaelFrI wonder if the best solution wouldn't be to have a
getTotalRows()
method on ViewExecutable that would run the count request then statically cache the result. No moreget_total_row
attribute needed, if someone needs to know the number of results, they just have to call the lazy getter.What do you think?
Comment #57
jamesrward CreditAttribution: jamesrward as a volunteer commentedDefinitely open to the getTotalRows() idea. Just trying again to get a green from the testbot before moving on with the discussion.
Comment #58
jamesrward CreditAttribution: jamesrward as a volunteer commentedAnd triggering the bot.
Comment #60
Lendudeshouldn't this just be $view->total_rows without the count?
Comment #61
yogeshmpawarUpdated the patch as per comment #60, also added interdiff.
Comment #62
dawehnerWe need some tests here, given that we fix a bug. Sorry :)
Comment #63
jamesrward CreditAttribution: jamesrward as a volunteer commented@dawehner definitely, I just wanted to be sure we are all ok with this approach before I start reworking the tests. I'll get started.
Comment #64
jamesrward CreditAttribution: jamesrward as a volunteer commentedHere's a tests only patch. Should produce 2 failures, 1 each for the full and mini pager.
Comment #65
jamesrward CreditAttribution: jamesrward as a volunteer commentedTriggering test bot.
Comment #67
jamesrward CreditAttribution: jamesrward as a volunteer commentedWhelp, that was the wrong file. Let's try this again. Should be 2 failing tests.
Comment #69
jamesrward CreditAttribution: jamesrward as a volunteer commentedAnd here's the fix and tests rolled together.
Comment #71
jamesrward CreditAttribution: jamesrward as a volunteer commentedWhoops, mixing ints and strings with assertIdentical. Let's try this.
Comment #72
dawehnerNice work!
maybe using an array_combine here would make things a bit more readable.
Note: we no longer use format_string there. Just go with
sprintf
Comment #73
jamesrward CreditAttribution: jamesrward as a volunteer commented@dawehner what do you think of this? I couldn't figure a way to make array_combine work so I tried array_keys instead.
Comment #74
LendudeBit of nitpicking, looking good so far.
Don't know but this feels like it should be done in preQuery. I know Result does it in query too, but meh, whatever.
When taking this change out, the added tests still pass, so we need coverage for this bit. Something like what happens is
\Drupal\Tests\views\Unit\Plugin\area\ResultTest::testQuery
I'd say.These assertions are deprecated, lets not use them for new tests.
Setting back to needs work mostly for the additional test coverage.
Comment #75
jamesrward CreditAttribution: jamesrward as a volunteer commented@Lendude that all makes sense. The issue with the lack of test coverage surprised me. The minipager always knows about exactly one more record than the current page total so in this case it knows the total-rows without running the query. If we reduce the items per-page from 4 to 3 then we should see a failure without those lines. I still like the idea of checking the query like we did for Result but I think changing the items-per-page to 3 for this test is worthwhile as well.
I'll roll that up shortly and see how it looks.
Comment #76
jamesrward CreditAttribution: jamesrward as a volunteer commentedHere's a patch with a new view import for the mini-pager and an additional test to check that $view->get_total_rows is TRUE. We should see 3 failures here.
Comment #78
jamesrward CreditAttribution: jamesrward as a volunteer commentedAnd this should pass. I don't know why we only see the one failure on the previous test. It's like this set of tests is --die-on-fail. If I remove the failing test it moves on to the next failing test as expected. I'll pop on IRC and see if I can get some insight into that.
Comment #79
dawehnerPlease always post an interdiff. Thank you!
you should be able to use
array_combine(array_keys($expected), array_fill(0, count($expected), $base_bubbleable_metadata));
Comment #80
jamesrward CreditAttribution: jamesrward as a volunteer commented@dawehner I'm not convinced we are making things more readable with this but here you go.
Comment #81
LendudeLast bit of nitpicking then I think this is good to go.
Maybe just add a comment as to why we are doing this?
first line indent not ok
Comment #82
dawehnerYeah that's fair. To be honest I'm way too much in functional programming these days, so yeah I agree, perspectives are indeed different :)
Please choose the variant you think is better and more readable.
Good point
Comment #83
jamesrward CreditAttribution: jamesrward as a volunteer commentedThanks for the input and good catch on the nitpicks @lendude.
@dawehner The more I look at this the more I feel like putting this value in an array is totally unnecessary. All we are doing here is testing to confirm that the metadata we used to test the token replacement matched the metadata we had prior to token replacement. No need for more than one copy of that base value. I also had expected and actual reversed.
Comment #84
dawehnernice simplification @jamesrward!
nitpick on commit: missing dot at the end
Comment #85
yogeshmpawarRerolled the patch against #84.
Comment #87
LendudeUnrelated fails #2828045: Tests failing with unrelated „Translation: The French translation is not available”, will RTBC again when that is resolved.
Comment #88
LendudeNitpick fixed and unrelated fails, back to RTBC
Comment #90
catchIt's a bit strange looking for the token and altering the view, but every other possible solution seems worse. So went ahead and committed/pushed to 8.3.x, thanks!