The [view:page-count] token incorrectly returns 1 when there are several pages of results for a given view.

To reproduce:

  1. drush si standard -y --account-pass=admin
  2. drush en -y devel_generate
  3. drush generate-content 50
  4. Log in as admin and navigate to /admin/structure/views/view/frontpage
  5. Add a "Global: text area" handler to the "Header" area with the following text: [view:current-page] out of [view:page-count]
  6. Save the view
  7. 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.

CommentFileSizeAuthor
#85 interdiff-2572355-83-85.txt715 bytesyogeshmpawar
#85 2572355-85-page-count-token.patch6.27 KByogeshmpawar
#83 interdiff-2572355-80-83.txt1.64 KBjamesrward
#83 2572355-83-page-count-token.patch6.27 KBjamesrward
#80 interdiff-2572355-78-80.txt760 bytesjamesrward
#80 2572355-80-page-count-token.patch6.29 KBjamesrward
#78 2572355-78-page-count-token.patch6.33 KBjamesrward
#76 2572355-76-mini-pager-test-failures.patch5.46 KBjamesrward
#73 2572355-73-page-count-token.patch6.02 KBjamesrward
#71 2572355-71-page-count-token.patch6.45 KBjamesrward
#69 2572355-69-page-count-token.patch6.45 KBjamesrward
#67 2572355-67-failing-tests.patch4.47 KBjamesrward
#64 2572355-64-tests-only.patch13.42 KBjamesrward
#61 interdiff-2572355-57-61.txt668 bytesyogeshmpawar
#61 page_count_token-2572355-61.patch1.98 KByogeshmpawar
#57 2572355-57-page-count-token.patch1.98 KBjamesrward
#54 2572355-54-page-count-token.patch1.97 KBjamesrward
#51 2572355-47-51-no-token-replacement.interdiff.txt3.83 KBmikeker
#51 2572355-47-51-out-of-scope.interdiff.txt2.17 KBmikeker
#51 2572355-51-page-count-token.patch11.56 KBmikeker
#47 2572355-43-47.interdiff.txt2 KBmikeker
#47 2572355-47-page-count-token.patch11.68 KBmikeker
#47 2572355-47-tests-only.patch8.83 KBmikeker
#43 2572355-43-page-count-token.patch9.38 KBsmk-ka
#41 2572355-41-page-count-token.patch9.57 KBsmk-ka
#31 2572355-31-page-count-token.patch9.36 KBmikeker
#31 2572355-tests-only-reroll.patch6.81 KBmikeker
#29 2572355-29.patch9.03 KBmikeker
#29 2572355-26-29.interdiff.txt1.27 KBmikeker
#26 2572355-26.patch9.02 KBmikeker
#26 2572355-24-26.interdiff.txt1.57 KBmikeker
#24 2572355-24.patch9.13 KBmikeker
#24 2572355-22-24.interdiff.txt859 bytesmikeker
#22 2572355-22.patch9.01 KBmikeker
#22 2572355-19-22.interdiff.txt3.68 KBmikeker
#19 2572355-19.patch10.8 KBmikeker
#19 2572355-13-19.interdiff.txt1.53 KBmikeker
#13 2572355-tests.interdiff.txt1.91 KBmikeker
#13 2572355-12-page-count-token.patch9.9 KBmikeker
#10 2572355-tests-only.patch6.52 KBmikeker
#2 2509716-2-page-count-token.patch661 bytesmikeker
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeker created an issue. See original summary.

mikeker’s picture

Status: Active » Needs review
FileSize
661 bytes

The calculation for this token was using $view->results which no longer returns all the results for a given view.

olli’s picture

Issue tags: +Needs tests

Fix looks good.

+++ b/core/modules/views/views.tokens.inc
@@ -127,7 +127,7 @@ function views_tokens($type, $tokens, array $data, array $options, BubbleableMet
           // If there are no items per page, set this to 1 for the division.
           $per_page = $view->getItemsPerPage() ?: 1;

Maybe out of scope but if items per page is 0 (no limit) then page-count is 1?

mikeker’s picture

@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!

Maybe out of scope but if items per page is 0 (no limit) then page-count is 1?

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?

dawehner’s picture

MHH, yeah we need tests first, one for a normal pager, one for a mini pager.

+++ b/core/modules/views/views.tokens.inc
@@ -127,7 +127,7 @@ function views_tokens($type, $tokens, array $data, array $options, BubbleableMet
-          $replacements[$original] = (int) ceil(count($view->result) / $per_page);
+          $replacements[$original] = (int) ceil($view->total_rows / $per_page);

Mh, total rows is not always set, like for the mini pager

mikeker’s picture

Status: Needs review » Needs work

Stupid mini-pager... :)

mikeker’s picture

Title: [view:page-count] token is incorrect » Some view tokens ([view:page-count] and [view:total-rows]) are incorrect

After 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...

mikeker’s picture

And, 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

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

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...?

dawehner’s picture

Do we re-execute the view if there are tokens involved?

Sounds horrible, sorry

mikeker’s picture

Sounds horrible, sorry

Agreed!

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?

mikeker’s picture

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tokens.yml
@@ -7,7 +7,7 @@ module: views
-base_field: id
+base_field: ''

This export was made by round-tripping the existing test_tokens view. I'm not sure why the base_field changed, though.

mikeker’s picture

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

This 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.

mikeker’s picture

The last submitted patch, 10: 2572355-tests-only.patch, failed testing.

The last submitted patch, 10: 2572355-tests-only.patch, failed testing.

vadim.hirbu’s picture

Tested on latest stable version. Works like a charm.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/pager/Mini.php
@@ -67,7 +67,7 @@ public function query() {
   public function useCountQuery() {
-    return FALSE;
+    return TRUE;
   }
 

Sorry, 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.

mikeker’s picture

Sorry, but the entire point of the mini pager is to not run those queries.

Yeah, 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.

mikeker’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
10.8 KB

Fixes #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...

mikeker’s picture

Also, this doesn't remove the tokens if they are unavailable -- figured it would be better to show a 0 than [view:total-rows].

Status: Needs review » Needs work

The last submitted patch, 19: 2572355-19.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
9.01 KB

Ugh... 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.

Status: Needs review » Needs work

The last submitted patch, 22: 2572355-22.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
859 bytes
9.13 KB

Oh, fer cryin' out loud!

Time to go to sleep.

dawehner’s picture

Status: Needs review » Needs work

If that is a hard rule

This is a 100% strict role. No way around that. This is a major performance improvement for Drupal potentially.

  1. +++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
    @@ -92,7 +92,26 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
    +      }
    +      if ($display->options['pager']['type'] == 'mini') {
    +        $uses_mini = TRUE;
    +        break;
    +      }
    

    IMHO we should check for useCountQuery()

  2. +++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
    @@ -92,7 +92,26 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
    +      $form['global_tokens']['#description'] .= t('Any tokens involving the total row count, such as <code>[view:total-rows]

    or [view:page-count], will not be available in displays using the mini pager.');

    Appending a description looses safeness.

mikeker’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
9.02 KB

From #25:

#1: IMHO we should check for useCountQuery()

Done. Also renamed the local var to be consistent.

#2: Appending a description looses safeness.

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.

Status: Needs review » Needs work

The last submitted patch, 26: 2572355-26.patch, failed testing.

dawehner’s picture

Any idea yet, why its broken?

mikeker’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
9.03 KB

Might 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.

Lendude’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
Related issues: +#2510076: The [view:page-count] token should never return 0

This overlapped with #2510076: The [view:page-count] token should never return 0 so this will need a reroll.

mikeker’s picture

Rerolled. Thanks for the pointer to that issue, @Lendude. Unfortunately, the fix there is incorrect -- I'll post a comment shortly.

The last submitted patch, 31: 2572355-tests-only-reroll.patch, failed testing.

DuaelFr’s picture

It looks really good!

Just one question:

+++ b/core/modules/views/src/Tests/TokenReplaceTest.php
@@ -72,7 +75,52 @@ function testTokenReplacement() {
+      '[view:label]' => 'Test tokens',
+      '[view:description]' => 'Test view to token replacement tests.',
+      '[view:id]' => 'test_tokens',
+      '[view:title]' => 'Test token page with minipager',
+      '[view:url]' => $view->getUrl(NULL, 'page_3')->setAbsolute(TRUE)->toString(),
+      '[view:base-table]' => 'views_test_data',
+      '[view:base-field]' => 'id',
...
+    $metadata_tests['[view:label]'] = $base_bubbleable_metadata;
+    $metadata_tests['[view:description]'] = $base_bubbleable_metadata;
+    $metadata_tests['[view:id]'] = $base_bubbleable_metadata;
+    $metadata_tests['[view:title]'] = $base_bubbleable_metadata;
+    $metadata_tests['[view:url]'] = $base_bubbleable_metadata;
+    $metadata_tests['[view:base-table]'] = $base_bubbleable_metadata;
+    $metadata_tests['[view:base-field]'] = $base_bubbleable_metadata;

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.

mikeker’s picture

@DuaelFr, thank you for the review! From #33:

Isn't it out of the scope of this issue?

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.

jibran’s picture

+++ b/core/modules/views/views.tokens.inc
@@ -113,7 +113,7 @@ function views_tokens($type, $tokens, array $data, array $options, BubbleableMet
-          $replacements[$original] = count($view->result);
+          $replacements[$original] = $view->total_rows;

@@ -124,7 +124,7 @@ function views_tokens($type, $tokens, array $data, array $options, BubbleableMet
-          $replacements[$original] = max(1, (int) ceil(count($view->result) / $per_page));
+          $replacements[$original] = max(1, (int) ceil($view->total_rows / $per_page));

Is this the only fix?

mikeker’s picture

@jibran, functionally, yes. There is also warning to users in TokenizeAreaPluginBase that shows in the UI.

jibran’s picture

Thanks @mikeker

+++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
@@ -92,7 +92,23 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
+      $form['global_tokens']['#description'] = t('Any tokens involving the total row count, such as <code>[view:total-rows]

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.

dawehner’s picture

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.

We can add new strings, we just can't edit existing ones for 8.0.x.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 31: 2572355-31-page-count-token.patch, failed testing.

smk-ka’s picture

Status: Needs work » Needs review
FileSize
9.57 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 41: 2572355-41-page-count-token.patch, failed testing.

smk-ka’s picture

Sorry, real patch this time.

smk-ka’s picture

Status: Needs work » Needs review
Lendude’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
@@ -87,7 +87,23 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
+    foreach($displays as $display) {
+      if (!$display->usesPager()) {
+        continue;
+      }
+      if (!$display->getPlugin('pager')->useCountQuery()) {
+        $has_count = FALSE;
+        break;
+      }
+    }
     $this->globalTokenForm($form, $form_state);
+    if (!$has_count) {
+      $form['global_tokens']['#description'] = t('Any tokens involving the total row count, such as

Am I overlooking something or is there no test for this logic yet? If not, than that needs a test right?

mikeker’s picture

Assigned: Unassigned » mikeker

@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.

mikeker’s picture

Version: 8.1.x-dev » 8.2.x-dev
Assigned: mikeker » Unassigned
Status: Needs work » Needs review
FileSize
8.83 KB
11.68 KB
2 KB

Tests 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...

The last submitted patch, 47: 2572355-47-tests-only.patch, failed testing.

DuaelFr’s picture

Status: Needs review » Needs work

Thank 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 in views_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?

  1. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tokens.yml
    @@ -7,7 +7,7 @@ module: views
    -base_field: id
    +base_field: ''
    
    @@ -40,10 +40,16 @@ display:
    +      display_extenders: {  }
    

    Looks out of scope.

  2. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tokens.yml
    @@ -40,10 +40,16 @@ display:
    +    cache_metadata:
    +      contexts:
    +        - 'languages:language_interface'
    +        - url.query_args
    +      cacheable: false
    
    @@ -57,6 +63,12 @@ display:
    +      display_extenders: {  }
    +    cache_metadata:
    +      contexts:
    +        - 'languages:language_interface'
    +        - url.query_args
    +      cacheable: false
    

    Looks out of scope.

  3. +++ b/core/modules/views/tests/src/Kernel/TokenReplaceTest.php
    @@ -66,7 +69,52 @@ function testTokenReplacement() {
    -      $this->assertIdentical($output, $expected_output, format_string('Token %token replaced correctly.', array('%token' => $token)));
    +      $this->assertIdentical($output, $expected_output, new FormattableMarkup('Token @token replaced correctly.', ['@token' => $token]));
    +      $this->assertEqual($bubbleable_metadata, $metadata_tests[$token]);
    

    Looks out of scope.

  4. +++ b/core/modules/views/tests/src/Kernel/TokenReplaceTest.php
    @@ -86,7 +134,7 @@ function testTokenReplacementNoResults() {
    -      $this->assertIdentical($output, $expected_output, format_string('Token %token replaced correctly.', array('%token' => $token)));
    +      $this->assertIdentical($output, $expected_output, new FormattableMarkup('Token @token replaced correctly.', array('@token' => $token)));
    

    Looks out of scope.

mikeker’s picture

@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 to FormattableMarkup 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...

mikeker’s picture

Two 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?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jamesrward’s picture

Given 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.

jamesrward’s picture

So 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.

Status: Needs review » Needs work

The last submitted patch, 54: 2572355-54-page-count-token.patch, failed testing.

DuaelFr’s picture

I 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 more get_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?

jamesrward’s picture

Definitely open to the getTotalRows() idea. Just trying again to get a green from the testbot before moving on with the discussion.

jamesrward’s picture

Status: Needs work » Needs review

And triggering the bot.

Status: Needs review » Needs work

The last submitted patch, 57: 2572355-57-page-count-token.patch, failed testing.

Lendude’s picture

+++ b/core/modules/views/views.tokens.inc
@@ -119,7 +119,7 @@ function views_tokens($type, $tokens, array $data, array $options, BubbleableMet
+          $replacements[$original] = count($view->total_rows);

shouldn't this just be $view->total_rows without the count?

yogeshmpawar’s picture

Assigned: jamesrward » Unassigned
Status: Needs work » Needs review
FileSize
1.98 KB
668 bytes

Updated the patch as per comment #60, also added interdiff.

dawehner’s picture

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

We need some tests here, given that we fix a bug. Sorry :)

jamesrward’s picture

@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.

jamesrward’s picture

Assigned: Unassigned » jamesrward
FileSize
13.42 KB

Here's a tests only patch. Should produce 2 failures, 1 each for the full and mini pager.

jamesrward’s picture

Status: Needs work » Needs review

Triggering test bot.

Status: Needs review » Needs work

The last submitted patch, 64: 2572355-64-tests-only.patch, failed testing.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
4.47 KB

Whelp, that was the wrong file. Let's try this again. Should be 2 failing tests.

Status: Needs review » Needs work

The last submitted patch, 67: 2572355-67-failing-tests.patch, failed testing.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
6.45 KB

And here's the fix and tests rolled together.

Status: Needs review » Needs work

The last submitted patch, 69: 2572355-69-page-count-token.patch, failed testing.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
6.45 KB

Whoops, mixing ints and strings with assertIdentical. Let's try this.

dawehner’s picture

Nice work!

  1. +++ b/core/modules/views/tests/src/Kernel/TokenReplaceTest.php
    @@ -41,12 +43,61 @@ function testTokenReplacement() {
    +    $metadata_tests['[view:label]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:description]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:id]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:title]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:url]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:total-rows]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:base-table]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:base-field]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:items-per-page]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:current-page]'] = $base_bubbleable_metadata;
    +    $metadata_tests['[view:page-count]'] = $base_bubbleable_metadata;
    

    maybe using an array_combine here would make things a bit more readable.

  2. +++ b/core/modules/views/tests/src/Kernel/TokenReplaceTest.php
    @@ -41,12 +43,61 @@ function testTokenReplacement() {
    +      $this->assertIdentical($output, $expected_output, format_string('Token %token replaced correctly.', array('%token' => $token)));
    

    Note: we no longer use format_string there. Just go with sprintf

jamesrward’s picture

@dawehner what do you think of this? I couldn't figure a way to make array_combine work so I tried array_keys instead.

Lendude’s picture

Status: Needs review » Needs work

Bit of nitpicking, looking good so far.

  1. +++ b/core/modules/views/src/Plugin/views/area/Text.php
    @@ -46,6 +46,16 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +  public function query() {
    

    Don't know but this feels like it should be done in preQuery. I know Result does it in query too, but meh, whatever.

  2. +++ b/core/modules/views/src/Plugin/views/area/Text.php
    @@ -46,6 +46,16 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +    $content = $this->options['content']['value'];
    +    if (strpos($content, '[view:page-count]') !== FALSE || strpos($content, '[view:total-rows]') !== FALSE) {
    +      $this->view->get_total_rows = TRUE;
    +    }
    +  }
    

    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.

  3. +++ b/core/modules/views/tests/src/Kernel/TokenReplaceTest.php
    @@ -72,6 +74,48 @@ function testTokenReplacement() {
    +      $this->assertIdentical($output, $expected_output, sprintf('Token %token replaced correctly.', array('%token' => $token)));
    +      $this->assertEqual($bubbleable_metadata, $metadata_tests[$token]);
    

    These assertions are deprecated, lets not use them for new tests.

Setting back to needs work mostly for the additional test coverage.

jamesrward’s picture

@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.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
5.46 KB

Here'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.

Status: Needs review » Needs work

The last submitted patch, 76: 2572355-76-mini-pager-test-failures.patch, failed testing.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
6.33 KB

And 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.

dawehner’s picture

Please always post an interdiff. Thank you!

+++ b/core/modules/views/tests/src/Kernel/TokenReplaceTest.php
@@ -71,6 +73,48 @@ function testTokenReplacement() {
+    $metadata_tests = [];
+    foreach(array_keys($expected) as $expected_token) {
+      $metadata_tests[$expected_token] = $base_bubbleable_metadata;
+    }

you should be able to use array_combine(array_keys($expected), array_fill(0, count($expected), $base_bubbleable_metadata));

jamesrward’s picture

@dawehner I'm not convinced we are making things more readable with this but here you go.

Lendude’s picture

Last bit of nitpicking then I think this is good to go.

  1. +++ b/core/modules/views/src/Plugin/views/area/Text.php
    @@ -46,6 +46,16 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +    $content = $this->options['content']['value'];
    +    if (strpos($content, '[view:page-count]') !== FALSE || strpos($content, '[view:total-rows]') !== FALSE) {
    +      $this->view->get_total_rows = TRUE;
    +    }
    

    Maybe just add a comment as to why we are doing this?

  2. +++ b/core/modules/views/tests/src/Kernel/TokenReplaceTest.php
    @@ -71,6 +73,45 @@ function testTokenReplacement() {
    + /**
    +   * Tests core token replacements generated from a view.
    +   */
    

    first line indent not ok

dawehner’s picture

@dawehner I'm not convinced we are making things more readable with this but here you go.

Yeah 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.

Maybe just add a comment as to why we are doing this?

Good point

jamesrward’s picture

Thanks 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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

nice simplification @jamesrward!

+++ b/core/modules/views/src/Plugin/views/area/Text.php
@@ -48,6 +48,7 @@
+    // Check for tokens that require a total row count

nitpick on commit: missing dot at the end

yogeshmpawar’s picture

Rerolled the patch against #84.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 85: 2572355-85-page-count-token.patch, failed testing.

Lendude’s picture

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

Nitpick fixed and unrelated fails, back to RTBC

  • catch committed 8766c99 on 8.3.x
    Issue #2572355 by mikeker, jamesrward, Yogesh Pawar, smk-ka, dawehner,...
catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/modules/views/src/Plugin/views/area/Text.php
@@ -46,6 +46,17 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+    if (strpos($content, '[view:page-count]') !== FALSE || strpos($content, '[view:total-rows]') !== FALSE) {

It'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!

Status: Fixed » Closed (fixed)

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