Going to page 2 on "list terms" page doesn't display additional terms

I am encountering a similar issue.

We have a taxonomy with around 20-30 root terms and a few hundred terms sitting below each of those. The first page of admin/content/taxonomy/1 shows a total of 53 terms ($page_increment is 10) and only 2 of those terms are 'root' terms. The first has 38 descendants and the second 13.

Clicking 'next' will got to the next page but it says 'No terms available'. In fact pages 2, 3, and 4 all show 'No terms available'.

Page 5 will then show the 2nd root term shown on Page 1, it's 13 descendants and the next two root terms (which have no descendants). From page 5 onward the term listing appears to work more or less correctly.

The bug is in taxonomy_overview_terms() in the do {} while() loop that builds the list of terms for the page and only occurs if the 1st root term in a vocabulary has more than a page of descendants (i.e. 10 or more).

This is because it is trying to rewind the internal array pointer in $tree using prev() to before the beginning of the array so the second page starts with a root term. Doing so causes subsequent calls to next() (or current() or prev()) to return FALSE, aborting the loop early.

I have attached a patch to fix this bug. Instead of using prev() and next() it uses an index instead - which is allowed to go negative.

Patch works as designed. This seems to be a good fix to the problem described.

I can confirm this bug as well.

Coding style looks overall good, but I don't get $back_peddle. Seems like an odd variable name. It should probably either have a more descriptive name or better inline docs to explain what it's doing.

I can confirm bug! subscribing.

Confirmed bug! Subscribing

subscribing.... I found this bug too...

subscribing

This patch worked for me in 6.13. This definitely needs to be addressed in D7.

Changing to D7 so it can be fixed there, too.

bad trial run of new testing bot

Re-uploading to have it tested again.

Patch looks clean. RTBC?

Confirmed, in D6,
Subscribed.

#15: taxonomy-242324-20080422-1.patch queued for re-testing.

Still not working in 6.9, first patch failed, no effective results.
Same for second patch.

This is still a bug in D7 so it has to get fixed there first.

Ok.
I was looking, but didn' t find anything yet.

Rerolled patch for 7.x.

Same for me, i have a large vocabulary with few root term and under her, a long tree (more than 200 items)
when i come to the second pages, the page is empty, the pager was not.

with the patch proposed, the second page show the same term than the first page.

the only way i find for the moment is to count in a hook_taxonomy_term_presave()
the nb of term under the first root term, and then set the variable variable_set('taxonomy_terms_per_page_admin', $count + 20);

+ 20 ? if not count 20 more terms weird issue, like that works well... but it's a crap

so suscribing !

#23: 242324-23-taxonomy-empty_page.patch queued for re-testing.

#2: taxonomy-242324-20080422-1.patch queued for re-testing.

We need to add a test for this that fails without the patch in #23, and passes with it. Before/after screenshots of the pager would also be helpful.

Marked #1300208: Taxonomy paging generates wrong pages as a duplicate of this issue.

Let's:

1. Confirm whether this is reproducible on D8 HEAD. If it does:
2. Write a test case that fails when the bug is present.
3. Update the existing patch to D8 HEAD (maybe adding some code comments), and upload a test-only patch followed by the combined patch to expose the test failures and confirm that the patch fixes them.
4. Provide before-and-after screenshots illustrating the fix.

Created 1000 new terms using devel (gent) on D8 HEAD install. This is still a problem in D8. The links for "next page" and "previous page" change the page, but "No terms available" message results. Selecting "page 2" in the pager is also broken. The first and last links however do work as expected.

Needs issue summary

To re-produce issue:

* Create a new vocabulary (/admin/content/taxonomy/add/vocabulary)
* Add more terms than will fit onto the "list terms" page, this is 100
* Make sure that one parent has more than 100 siblings
* On page one everything is working (screenshot 1)
* Click on page 2 (or "next") on the "list terms" page
* It will go to the the next page but says "No terms available."(screenshot 2)

I`ll try to write an automated test so it can be reproduced.

Rerolls patch from #23 into Drupal 8.x Also editing issue summary.

Needs review to ensure initial reroll didn't break anything.

With the patch in #23, I'm having the same problems as delta (#24). Also, 161 terms show up on the first page even though my variable taxonomy_terms_per_page_admin is set to the default, 100. Subscribing.

Tested patch #33 and it works for me.

Updating tags, thanks @kbasarab!

The #33 patch shows the same results on the second page rather than the empty page.

ryanissamson: Yeah I thought the same thing. I'm actually doing some other testing now working on writing the tests for this. Seems as though when the term has more children then the page allows to show the items per page is not inherited.

Screenshots below:
Set to show only 9 terms per page via variable_set but this actually shows 10 terms per page (Separate issue)

Page 1: Shows A root term with two children and then our test term with 11 children.
Page 2: Show the same test term with 11 children but then continues on to show more terms
Page 3: Shows 10 terms as expected without any duplication as nothing on this page has more children than page needs.
Page 4: Final terms.

I'm just working on testing now. If someone wants to correct the other behavior. But it is better than before which showed zero terms on following page.

First run at creating tests for this. I feel there is probably a better way of testing this but I'm open to suggestions.

The FAIL patch also serves as the interdiff between 33 and 40.

Also removing screenshots tag as there are quite a few screenshots showing issue on case now.

Re-roll to use config('taxonomy.settings')->set instead of variable_set.

reroll of the patch, adapted to the chances since then.

#42: 242324-42.patch queued for re-testing.

I'm experiencing the bug in a D7 (7.22) installation with a large Taxonomy (9 pages in admin/structure/taxonomy).

Any news on this?

Should I use the patch provided in #23?

Cheers,
Marco

Same problem here with 7.22, and patch in #23 didn't work for me.

Issue Summary Initiative change.

41: 242324-41.patch queued for re-testing.

Rerolling the test to see if the test works on current 8.x head.

Setting to needs review to have the testbot run

Reroll of test to see if works on current current 8.x head PLUS removal of excessive newlines at end of testTaxonomyTermChildTerms function.

I modified the config function call to call for \Drupal::config(). I should be able to spend more time on this tonight.

Setting to Needs Review so that the testbot will run.

This test patch should show that there is still some issue with taxonomy in D8. It will show that Page 1 works fine, Page 2 is empty, and Page 3 appears to work properly.

Once again setting to Needs Review to activate TestBot.

This patch *should* fix the issue. It passed my test script that I posted in #57. I'll let the TestBot run and see what he can find.

I adapted the code from the folks that posted previous patches. Their code wouldn't work in the current 8.0.x HEAD.

This looks great, thanks for the patch and the test!

At first I didn't understand the change from current-prev-next to indexing but the problem is that once prev "steps off the cliff" and moves the array pointer beyond the first element, next will not bring it back. So that change is good and I do not think it requires commenting; noone would want to change it back in such a convoluted piece of code, I am quite sure -- and the test would break if they would.

My second problem is that the test while very nice uses the UI to edit the terms. If you would add an optional $values array to createTerm() and then pass it to entity_create like entity_create('taxonomy_term', $values + array( this then you wouldn't need an additional (and very slow) step of using the UI to edit the term you just created. Setting it to needs work for this.

Now, this doesn't need a test any more and doesn't need manual testing thanks to the very nice test written.

I'll see what I can do to modify the test code to work this way. All of the code was pulled from previous patches from other folks. I just modified it enough to make it work with current 8.0.x

Either they didn't know there was a better way or they were trying to simulate how the end user would have added terms.

This patch removes the use of the UI when adding Taxonomy Terms within the test. Hopefully this will increase the performance of the test. The createVocabulary function in TexonomyTestBase now accepts an optional $values array as well.

shoot, I somehow mixed the fix in the patch. *sigh* I'll try to get it generated properly shortly.

Alright folks, lets try this again. This includes the modified test and the fix.

I cleaned up the code in this patch to meet Drupal Coding Standards. See interdiff for lines affected.

Also removed a couple lines of redundant code that specified the term had no parent if it wasn't one of the first 12 terms. Parent by default has a value of 0.

I had an issue in the last upload and then I found more areas that needed cleaned up.

This is ready, thanks much for the fixes, looking really nice.

createTerm() was missing doxygen and typehints, I added that. But that's not a significant enough change to wait for another reviewer, I felt OK with RTBC'ing this.

Confirmed that the test fails without the patch. Nice work.

Committed 9bc009b and pushed to 8.x. Thanks!

What about 7.x? It is still happening in 7.32. Patch #23 or any other doesn't seems to be committed.

I deemed the code unfixable, wiped it and wrote a new one. This code will always display at least taxonomy_terms_per_page_admin and a new page will always start with a top level element.

I do not even know what HEAD is supposed to do. The test passes but looking back either I was much smarter in July and comprehended the code or I haven't comprehended the problem and haven't considered the true extent of it. This version does away with the forward and backward jumps as it's very confusing. Very simple code. Works well.

To clarify: the current behavior:

Terms from previous and next pages are shown if the term tree would have been cut in the middle.

This is incredibly confusing to clients. The code to do this is incredibly confusing to me. Both can be eliminated by this simple rule:

Do not cut trees in the middle. Once a tree starts, it displays on a page.

This makes the setting a minimum instead of a hard value but I think that's less of a problem. It makes much easier to administer trees as well. If you have so many terms the browser kneels from this then use some other UI. Paginating a tree is utterly and completely hopeless user experience wise.

For me no pager shows up at all.
The steps in problem/motivation need to be updated, when setting terms_per_page_admin = 3, there is no need for 100 terms to have 1 parent.

Rerolled the patch in #78. The core/modules/taxonomy/css/taxonomy.module.css file was renamed to core/modules/taxonomy/css/taxonomy.theme.css, but its contents were the same, so I deleted that.
The line referring to the above mentioned css file was changed in core/modules/taxonomy/taxonomy.libraries.yml file to its new name, so I bravely deleted this file too.

I confirm that applying the patch in #87 all children terms appear on the page, and the next page starts with the next parent item.

Is this still an issue in 8.0.x? When testing #2425535: No pager shows on taxonomy overview pages I did not encounter this bug.

@xjm: Did you use the steps mentioned in this summary?

Be careful: if you tree grows over 250 items then the default 1000 POST vars will make Drupal silently disregard the form submission since the form_id is after the tree and each tree item is 4 variables.

Newer D7 patch.

minor change to above D7 patch from @chx, just because our patch system can't apply this properly

Patch on #98 works on v7.41 (the one on #97 has a reference to a docroot/ folder).

Issue is pretty bad. I thought terms where missing and I was going to delete and recreate the vocabulary (198 terms).
It should be included in D7 ASAP.

Thanks a lot @chx and @douggreen for the patch 242324-98-do-not-test.patch. This works for me For D7.

Patched committed to 8.0.x (#72) and 8.1.x (#101), so changing version and status as apparently only 7.x commits is needed.

The latest Drupal 7 patch is very different from the patch that was committed to Drupal 8, and doesn't have tests yet (in fact it's even marked "do-not-test").

I think @chx moved this back to Drupal 8 based on the idea that the Drupal 8 fix wasn't good (see #80).

However if what went into Drupal 8 is at least an improvement over the previous situation (?), we should backport that to Drupal 7 here, and then move the further improvements to a followup issue instead.

Moving back to "needs review" for feedback on that.

This was already commited 8.1.x, see #101.

@David_Rothstein I'm not really sure where @chx moved this back to Drupal 8 - apologize for that -, but I do see his concerns on #80 are issued on patch from @balagan at #87 and committed to 8.1.x at #101.

Also D7 patch from @chx at #97 covers these concerns as well.

I understand no automatic tests have ran against the D7 patch due the -do-not-test suffix, but @dougreen, @sukh.singh and I have tested the patch manually and that's why I set the issue as Reviewed ant tested by the community.

However if what went into Drupal 8 is at least an improvement over the previous situation (?), we should backport that to Drupal 7 here, and then move the further improvements to a followup issue instead.

I'm not sure what has been committed to 8.0.x, but both commits to 8.1.x and patch for 7.x covers @chx concerns, are definitely a big improvement and fix a dangerous bug (see #102 about why I consider it dangerous).

Just my thoughts, let's collect more feedbacks.

Hello,

not entirely sure if this is related or not (apologies if not), but I think that the changes implemented here have led to an unintended side effect : parents with a lot of children tend to break pagination of the Taxonomy Overview Terms page.

If interested, https://www.drupal.org/node/2806551 .

Tnx,

I've found there is an issue with query even if the page is the first one ?page=0.

created patch from core 8.7.1

I think this problem has been forgotten since many years have passed and it remains unsolved.

I have migrated patch on # 98 to D7.72 and tested with a big taxonomy with three levels of depth (Costa Rica's regional distribution, Province, Canton, District) and since to work good.

Thanks a lot @chx and @douggreen for your work.

Closed #691110: Taxonomy overview form pager bug resulting in "No terms available" for vocabularies with hierarchy as a duplicate and added credit.

I'm curious what else needs to be done to get this committed. This is a problem that our editorial unit is running into for a Drupal 7 site that we run with many terms in a taxonomy. I'm willing to do testing and/or tweak code... but will need to know what folks are looking for to see this as fixed so it can be committed. It seems like all of the "remaining tasks" are taken care of... unless I'm missing something. Happy to help, but will need a bit of guidance.

@AndrewGearhart, for this to get into D7, we probably need to do some sort of test coverage for this use case. The D8 commit added testing for this (it's above in comment #101 https://git.drupalcode.org/project/drupal/commit/9bc009b)

This was committed to 8.0n.x in Aug 2014. Then the fix was considered not suitable and a new patch made. Then there are variety of comments about Drupal 7 and Drupal 8. It is challenging to figure out what needs to be done here.

Using the steps in the IS I tested this on Drupal 9.3.x, demo_umami install and Drupal 7, standard install. In both cases I was not able to reproduce the problem. According to that there is nothing that needs to be done here. And yet #130 says this is still a problem with Drupal 7. So, what exactly is the problem is the problem?

I think a way forward is to move this to Drupal 7. If anyone know that something needs to be done for Drupal 9, open a new issue. This will need an Issue Summary update and steps to reproduce the problem.

Updated patch applies cleanly to 7.98

Uploading the same patch, but without the do-not-test name, since this is now a 7.x ticket.