Problem/Motivation
Taxonomy terms that have more children than will fit on one page (>100) then when paging to next page term page becomes blank.
Steps to reproduce
- Create a new vocabulary (/admin/content/taxonomy/add/vocabulary)
- Run
drush ev '$config = \Drupal::configFactory()->getEditable("taxonomy.settings");$config->set('terms_per_page_admin', 3);$config->save();
ordrush vset taxonomy_terms_per_page_admin 3'
in D7, or modify drupal8\core\modules\taxonomy\config\install\taxonomy.settings.yml file manually, and change terms_per_page_admin to the value of 3. - Add 5 terms than will fit onto the "list terms" page.
Make sure that one parent has more than 100 childrenNo need for this, if you modify the terms_per_page_admin value. Instead use the number you have set + 2.- 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)
The whole behavior and codebase is confusing.
Remaining tasks
TBD
Original report by whatistocome
To re-produce issue:
* Create a new vocabulary (/admin/content/taxonomy/add/vocabulary)
* Add more terms than will fit onto the "list terms" page
* Click on page 2 (or "next") on the "list terms" page
* It will go to the the next page but says "No terms available."
Comment | File | Size | Author |
---|---|---|---|
#134 | 242324-134-d7.patch | 8.49 KB | douggreen |
#133 | 242324-133-d7--do-not-test.patch | 8.49 KB | douggreen |
#123 | 242324-123-do-not-test.patch | 8.37 KB | MaxMendez |
#119 | vocabulary_terms_list_not_working_with_pager-242324-119.patch | 761 bytes | alejomc |
#98 | 242324-98-do-not-test.patch | 8.44 KB | douggreen |
Comments
Comment #1
triclops CreditAttribution: triclops commentedI 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.
Comment #2
triclops CreditAttribution: triclops commentedThe 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.
Comment #3
agentrickardPatch works as designed. This seems to be a good fix to the problem described.
Comment #4
Crell CreditAttribution: Crell commentedI 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.
Comment #5
coupet CreditAttribution: coupet commentedI can confirm bug! subscribing.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedThis needs a test written.
Comment #7
rmjiv CreditAttribution: rmjiv commentedConfirmed bug! Subscribing
Comment #8
rich.yumul CreditAttribution: rich.yumul commentedsubscribing.... I found this bug too...
Comment #9
t11o CreditAttribution: t11o commentedsubscribing
Comment #10
wonder95 CreditAttribution: wonder95 commentedThis patch worked for me in 6.13. This definitely needs to be addressed in D7.
Comment #11
agentrickardComment #12
wonder95 CreditAttribution: wonder95 commentedChanging to D7 so it can be fixed there, too.
Comment #14
deekayen CreditAttribution: deekayen commentedbad trial run of new testing bot
Comment #15
wonder95 CreditAttribution: wonder95 commentedRe-uploading to have it tested again.
Comment #16
agentrickardPatch looks clean. RTBC?
Comment #18
theemstra CreditAttribution: theemstra commentedConfirmed, in D6,
Subscribed.
Comment #19
theemstra CreditAttribution: theemstra commented#15: taxonomy-242324-20080422-1.patch queued for re-testing.
Comment #20
theemstra CreditAttribution: theemstra commentedStill not working in 6.9, first patch failed, no effective results.
Same for second patch.
Comment #21
Crell CreditAttribution: Crell commentedThis is still a bug in D7 so it has to get fixed there first.
Comment #22
theemstra CreditAttribution: theemstra commentedOk.
I was looking, but didn' t find anything yet.
Comment #23
pfrenssenRerolled patch for 7.x.
Comment #24
delta CreditAttribution: delta commentedSame 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 !
Comment #25
delta CreditAttribution: delta commented#23: 242324-23-taxonomy-empty_page.patch queued for re-testing.
Comment #26
sepehr.sadatifar CreditAttribution: sepehr.sadatifar commented#2: taxonomy-242324-20080422-1.patch queued for re-testing.
Comment #27
xjmWe 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.
Comment #28
xjmMarked #1300208: Taxonomy paging generates wrong pages as a duplicate of this issue.
Comment #29
xjmLet's:
Comment #30
perceptum CreditAttribution: perceptum commentedCreated 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.
Comment #31
BrockBoland CreditAttribution: BrockBoland commentedNeeds issue summary
Comment #32
TimVS CreditAttribution: TimVS commentedTo 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.
Comment #33
kbasarab CreditAttribution: kbasarab commentedRerolls patch from #23 into Drupal 8.x Also editing issue summary.
Comment #34
kbasarab CreditAttribution: kbasarab commentedNeeds review to ensure initial reroll didn't break anything.
Comment #35
estrejlau CreditAttribution: estrejlau commentedWith 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.
Comment #36
COBadger CreditAttribution: COBadger commentedTested patch #33 and it works for me.
Comment #37
star-szrUpdating tags, thanks @kbasarab!
Comment #38
ryan.gibson CreditAttribution: ryan.gibson commentedThe #33 patch shows the same results on the second page rather than the empty page.
Comment #39
kbasarab CreditAttribution: kbasarab commentedryanissamson: 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.
Comment #40
kbasarab CreditAttribution: kbasarab commentedFirst 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.
Comment #41
LinL CreditAttribution: LinL commentedRe-roll to use
config('taxonomy.settings')->set
instead ofvariable_set
.Comment #42
cgalli CreditAttribution: cgalli commentedreroll of the patch, adapted to the chances since then.
Comment #44
Kevin Morse CreditAttribution: Kevin Morse commented#42: 242324-42.patch queued for re-testing.
Comment #46
m.sant CreditAttribution: m.sant commentedI'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
Comment #47
svenhoutmeyers CreditAttribution: svenhoutmeyers commentedSame problem here with 7.22, and patch in #23 didn't work for me.
Comment #47.0
svenhoutmeyers CreditAttribution: svenhoutmeyers commentedIssue Summary Initiative change.
Comment #48
prabeen.giri CreditAttribution: prabeen.giri commented41: 242324-41.patch queued for re-testing.
Comment #50
rychannel CreditAttribution: rychannel commentedRerolling the test to see if the test works on current 8.x head.
Comment #51
rychannel CreditAttribution: rychannel commentedSetting to needs review to have the testbot run
Comment #52
rychannel CreditAttribution: rychannel commentedReroll of test to see if works on current current 8.x head PLUS removal of excessive newlines at end of testTaxonomyTermChildTerms function.
Comment #55
rychannel CreditAttribution: rychannel commentedI 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.
Comment #57
rychannel CreditAttribution: rychannel commentedThis 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.
Comment #59
rychannel CreditAttribution: rychannel commentedThis 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.
Comment #60
chx CreditAttribution: chx commentedThis 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.
Comment #61
rychannel CreditAttribution: rychannel commentedI'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.
Comment #62
rychannel CreditAttribution: rychannel commentedThis 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.
Comment #65
rychannel CreditAttribution: rychannel commentedshoot, I somehow mixed the fix in the patch. *sigh* I'll try to get it generated properly shortly.
Comment #66
rychannel CreditAttribution: rychannel commentedAlright folks, lets try this again. This includes the modified test and the fix.
Comment #68
rychannel CreditAttribution: rychannel commentedI 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.
Comment #69
rychannel CreditAttribution: rychannel commentedI had an issue in the last upload and then I found more areas that needed cleaned up.
Comment #70
chx CreditAttribution: chx commentedThis 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.
Comment #71
alexpottConfirmed that the test fails without the patch. Nice work.
Committed 9bc009b and pushed to 8.x. Thanks!
Comment #74
kenorb CreditAttribution: kenorb commentedWhat about 7.x? It is still happening in 7.32. Patch #23 or any other doesn't seems to be committed.
Comment #75
chx CreditAttribution: chx commentedI 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.Comment #77
chx CreditAttribution: chx commentedComment #78
chx CreditAttribution: chx commentedI 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.
Comment #79
chx CreditAttribution: chx commentedComment #80
chx CreditAttribution: chx commentedTo clarify: the current behavior:
This is incredibly confusing to clients. The code to do this is incredibly confusing to me. Both can be eliminated by this simple rule:
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.
Comment #81
balagan CreditAttribution: balagan commentedFor 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.
Comment #82
balagan CreditAttribution: balagan commentedComment #83
balagan CreditAttribution: balagan commentedComment #84
balagan CreditAttribution: balagan commentedComment #87
balagan CreditAttribution: balagan commentedRerolled 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.
Comment #88
balagan CreditAttribution: balagan commentedComment #89
balagan CreditAttribution: balagan commentedI confirm that applying the patch in #87 all children terms appear on the page, and the next page starts with the next parent item.
Comment #90
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedGot here from #2425535: No pager shows on taxonomy overview pages, which I just RTBC'd. This will resolve the issue that no pager is displayed as mentioned in #81.
The patch in this ticket looks good and has good coverage. It will need manual testing once the patch above gets in.
Comment #91
xjmIs this still an issue in 8.0.x? When testing #2425535: No pager shows on taxonomy overview pages I did not encounter this bug.
Comment #92
balagan CreditAttribution: balagan commented@xjm: Did you use the steps mentioned in this summary?
Comment #93
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #96
chx CreditAttribution: chx commentedBe 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.
Comment #97
chx CreditAttribution: chx commentedNewer D7 patch.
Comment #98
douggreen CreditAttribution: douggreen commentedminor change to above D7 patch from @chx, just because our patch system can't apply this properly
Comment #102
gambryPatch 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.
Comment #103
sukh.singh CreditAttribution: sukh.singh commentedThanks a lot @chx and @douggreen for the patch 242324-98-do-not-test.patch. This works for me For D7.
Comment #104
gambryPatched committed to 8.0.x (#72) and 8.1.x (#101), so changing version and status as apparently only 7.x commits is needed.
Comment #105
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe 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.
Comment #106
balagan CreditAttribution: balagan commentedThis was already commited 8.1.x, see #101.
Comment #107
gambry@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.
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.
Comment #112
Alienpruts CreditAttribution: Alienpruts commentedHello,
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,
Comment #119
alejomc CreditAttribution: alejomc as a volunteer and at Linkstaria commentedI'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
Comment #120
alejomc CreditAttribution: alejomc as a volunteer and at Linkstaria commentedComment #123
MaxMendez CreditAttribution: MaxMendez commentedI 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.
Comment #129
pameeela CreditAttribution: pameeela commentedClosed #691110: Taxonomy overview form pager bug resulting in "No terms available" for vocabularies with hierarchy as a duplicate and added credit.
Comment #130
AndrewGearhart CreditAttribution: AndrewGearhart at Pennsylvania State University commentedI'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.
Comment #131
joseph.olstad@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)
Comment #132
quietone CreditAttribution: quietone as a volunteer commentedThis 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.
Comment #133
douggreen CreditAttribution: douggreen commentedUpdated patch applies cleanly to 7.98
Comment #134
douggreen CreditAttribution: douggreen commentedUploading the same patch, but without the do-not-test name, since this is now a 7.x ticket.