Testing issue #242324: Going to page 2 on "list terms" page doesn't display additional terms, I found no pager on the taxonomy terms overview page.

Steps to Reproduce

  1. Install D8 using the Standard install profile (so that taxonomy is enabled and a vocabulary is created for us).
  2. Create many taxonomy terms
    • Downloading the devel module and using devel_generate is recommended
    • Default "terms per page" setting is 100, so either generate more than 100 terms OR adjust the terms_per_page_admin (to set via settings.php, use $config['taxonomy.settings']['terms_per_page_admin'] = 5;
  3. Beta phase evaluation

    Reference: https://www.drupal.org/core/beta-changes
    Issue category Bug because the pager for taxonomy term lists doesn't show up.
    Issue priority Major, because this makes it nearly impossible to administer some taxonomies through the UI.
    Unfrozen changes None.
    Prioritized changes Prioritized, because the main goal of this issue is to fix a bug.
    Disruption None.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lucaschain’s picture

This bug is being caused because the $form['pager_pager'] value is not being set. Also, there was a bug that displayed the same term in two pages, this patch fixes that too

lucaschain’s picture

Issue tags: +Needs manual testing
lucaschain’s picture

Component: page.module » taxonomy.module
Assigned: Unassigned » lucaschain
balagan’s picture

FileSize
189.92 KB

I confirm that applying this patch the pager shows up properly, and there is no extra term displayed regarding the terms_per_page_admin setting.

balagan’s picture

FileSize
168.99 KB

Well, as I see, the array_pop() conflicts with chx's patch here https://www.drupal.org/node/242324#comment-9618045
Applying both patches, only 2 terms show up with the setting terms_per_page_admin: 3
I think we should omit this array_pop(), since hopefully all the logic behind paging will be changed.

balagan’s picture

Status: Active » Needs work
balagan’s picture

Issue summary: View changes
balagan’s picture

I have removed the comments and the array_pop() function.

balagan’s picture

Status: Needs work » Needs review

The last submitted patch, 1: 2425535-no-pager-shows-on-taxonomy-overview-pages.patch, failed testing.

roderik’s picture

Issue summary: View changes
Status: Needs review » Postponed
Issue tags: +drupaldevdays, +Novice
Related issues: +#242324: Going to page 2 on "list terms" page doesn't display additional terms

OK - I agree that it is better to not add the array_pop() function. But that means this issue is effectively postponed on #242324: Going to page 2 on "list terms" page doesn't display additional terms, right?

Officially marking as such, for clarity.

If anyone sees the linked issue solved, please unpostpone and review this. I'm not removing the "needs manual testing" tag, but most (all?) of the manual testing has been done and screenshots posted.

balagan’s picture

@roderik, I think the Going to page 2 on "list terms" page doesn't display additional terms issue should not postpone this issue. This patch simply displays the pager, whatever the logic is behind it. I don't see any problem for reviewing this issue. Let's just display that pager, no matter what it shows, so I am reopening this.

balagan’s picture

Assigned: lucaschain » Unassigned
Status: Postponed » Needs review
roderik’s picture

> This patch simply displays the pager, whatever the logic is behind it.

Fair enough :)

akalata’s picture

Issue summary: View changes
Status: Needs review » Needs work

I updated the issue summary with testing steps, but in my manual testing the patch did not cause the pager to appear.

balagan’s picture

@akalata, indeed, it does not solve the issue anymore.
I had the following notice:
Notice: Undefined index: #element in template_preprocess_pager() (line 176 of ...\drupal8\core\includes\pager.inc).

Same notice for #parameters (line 177), #quantity (line 178), and an unnamed index (line 182).

The referred code in pager.inc is:

function template_preprocess_pager(&$variables) {
  $element = $variables['pager']['#element'];
  $parameters = $variables['pager']['#parameters'];
  $quantity = $variables['pager']['#quantity'];
  global $pager_page_array, $pager_total;

  // Nothing to do if there is only one page.
  if ($pager_total[$element] <= 1) {
    return;
  }

It seems another issue is holding back this patch.

balagan’s picture

I have created an issue for the missing indices. https://www.drupal.org/node/2472361

balagan’s picture

mondrake’s picture

Have you tried using #type => pager instead of #theme => pager

see https://www.drupal.org/node/2446539

nicoloye’s picture

Status: Needs work » Needs review
FileSize
454 bytes

@mondrake, I confirm that your solution do works for me. Good catch.
Here is balagan patch with your suggestion.

balagan’s picture

It works indeed, and then the other issue with indices is gone. Gonna review it properly though.

balagan’s picture

Status: Needs review » Reviewed & tested by the community

I confirm, the patch applies properly, and the pager shows up.
Anyone knows about the changes that made it necessary to use #type instead of #theme? 2 months ago using #theme was OK.

nicoloye’s picture

I think it comes from this : https://www.drupal.org/node/2446539
It seems it's some cache content related issue.

balagan’s picture

@nicoloye: Thanks, indeed!

roderik’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs manual testing +Needs tests

Great work.
But fact that the pager is missing, but we do not get any notification of this, means that... we need a test for this.

Please see https://www.drupal.org/contributor-tasks/write-tests if necessary.

It looks like core/modules/taxonomy/src/Tests/TaxonomyTermIndentationTest is a good example to use as a basis for this.

balagan’s picture

Issue tags: -Novice

Since now only writing tests are needed, removing Novice tag

akalata’s picture

Assigned: Unassigned » akalata

I think I'll try to tackle this after lunch!

lucaslg’s picture

I wrote a test which :
- Create a vocabulary
- Set term_per_page_admin to 3
- Create 6 terms
- Go to the overview page
- Check the pager is visible
- Go to page 2
- Check the pager is visible

I didn't look at the content of the page (if terms are placed on the right place) because it is not coherent as long as #242324: Going to page 2 on "list terms" page doesn't display additional terms is not fixed.

I wonder if a test is needed to check the absence of a pager when there is only one page of result.

jhedstrom’s picture

Priority: Normal » Major
Issue tags: -Needs tests

Confirmed that the patch in #28resolves the issue. Bumping status to major and removing the 'needs tests' tag since there are now tests.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.3 KB
2.82 KB

Cleaned up the test a bit, there was no need to load it after creating it.

Otherwise, looks great.

Because I'm only tweaking, still marking RTBC.

roderik’s picture

@lucaslg:
> I wonder if a test is needed to check the absence of a pager when there is only one page of result.

I think that's a good idea and it seems easy to do inside your existing test (tim.plunkett-cleaned version) with just a few lines addition.

I'm not going to change RTBC status for that but please go ahead and post a new version of the patch (and set Needs Review) if you want to :)

Anonymous’s picture

Status: Reviewed & tested by the community » Needs work

This patch looks great already, but let's do @lucaslg proposal as mentioned in #32.

  1. +++ b/core/modules/taxonomy/src/Tests/TaxonomyTermPagerTest.php
    @@ -0,0 +1,64 @@
    +   * Tests that the pager is visible when there are more terms to display
    +   * than the terms_per_page_admin setting
    

    Nitpick, but this wrapping is not correct and the sentence should end in a dot.

lucaslg’s picture

Ok, i'll have a look tomorrow.

lucaslg’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
1.23 KB

Here we go, it is now checking that there is no pager when there is only one page displayed.

@pjonckiere : Excuse me but I don't understand the wrapping problem. What should I change ? The two lines are both less than 80 characters.

Anonymous’s picture

Status: Needs review » Needs work

@lucaslg: "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over", so the word 'than' should move up a line (see the standards for comments).

I just tested the patch manually and it works as expected. The tests look great too. If you can just fix the comment, I'll RTBC if the patch comes back green.

Fwiw: while testing, I encountered the bug you mention in #28, but I also think this is out of scope for this ticket, since it will be dealt with in #242324: Going to page 2 on "list terms" page doesn't display additional terms.

Anonymous’s picture

Issue summary: View changes

Added a beta evaluation to the issue summary.

Anonymous’s picture

Issue summary: View changes

Typo and minor clarification.

lucaslg’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
1.31 KB

Thanks for the explanation pjonckiere. I stopped at the first coding standard page which does not mention it.

The comment is fixed.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, comments guidelines are separate. The guidelines as a whole are quite the read :)

The comment is fixed, but it seems like you introduced a trailing space (the red mark in dreditor, see below). I think that could be fixed on commit.

Thanks for sticking with the issue. This is a very nice first patch!

+++ b/core/modules/taxonomy/src/Tests/TaxonomyTermPagerTest.php
@@ -0,0 +1,73 @@
+   * Tests that the pager is visible when there are more terms to display than ¶
lucaslg’s picture

ah, trailing spaces, I already had troubles with them during the sprint. My IDE is now set correctly to get rid of them.
It should be all good now.
Thank you all for your help and guidelines.

Anonymous’s picture

Ok, brilliant. I didn't want to put it in "Needs work" for a single space.

Still looks good!

balagan’s picture

Assigned: lucaslg » Unassigned

Unassigning, I guess you forgot to do it.

xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Nice find. Agreed that this is major since it pretty much prevents large vocabularies from being administered in that UI. Thanks for the beta evaluation and the test-only patch in #28.

+++ b/core/modules/taxonomy/src/Tests/TaxonomyTermPagerTest.php
@@ -0,0 +1,73 @@
+   * Tests that the pager is visible when there are more terms to display than
+   * the terms_per_page_admin setting.

Technically this is only supposed to be a single line of 80 characters or fewer. I reworded it slightly on commit to fix that:

diff --git a/core/modules/taxonomy/src/Tests/TaxonomyTermPagerTest.php b/core/modules/taxonomy/src/Tests/TaxonomyTermPagerTest.php
index 9d33167..515d4dbe 100644
--- a/core/modules/taxonomy/src/Tests/TaxonomyTermPagerTest.php
+++ b/core/modules/taxonomy/src/Tests/TaxonomyTermPagerTest.php
@@ -38,8 +38,7 @@ protected function setUp() {
   }
 
   /**
-   * Tests that the pager is visible when there are more terms to display than
-   * the terms_per_page_admin setting.
+   * Tests that the pager is displayed properly on the term overview page.
    */
   public function testTaxonomyTermOverviewPager() {
     // Set limit to 3 terms per page.

Reference: https://www.drupal.org/node/1354#drupal (in the bulleted list).

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x (with credit for the reviews as well). Thanks everyone!

As a followup, we should check whether there are any duplicates of this issue that can be closed.

  • xjm committed 73cf5bd on 8.0.x
    Issue #2425535 by lucaslg, balagan, tim.plunkett, lucaschain, nicoloye,...
Anonymous’s picture

I looked through the taxonomy issue queue, but couldn't find any duplicates.

Status: Fixed » Closed (fixed)

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