Problem/Motivation
When a vocabulary's term list exceeds an indeterminate amount, users may experience a WSOD. This is caused by a call to taxonomy_get_tree() from within taxonomy_overview_terms(). This produces a known memory limit issue.
Steps to reproduce the problem:
- Install Drupal 8.x with the standard profile.
- Install devel.
- Visit: Extend (/admin/modules).
- Enable Devel and Devel generate modules.
- Use Devel to auto-generate terms (/admin/config/development/generate/taxonomy).
- Generate for the "tags" vocabulary
- Number of terms: 10000.
- Browse to terms list for the tags vocabulary (/admin/structure/taxonomy/tags).
- Lower PHP memory limits and/or auto-generate more terms, until WSOD occurs in previous step.
Proposed resolution
- Disable taxonomy_get_tree() if the vocabulary is over a certain size.
- Change term listings from a tree view to a new sibling browser, which requires #698918: Reduce memory usage from \Drupal\taxonomy\TermStorageInterface::loadTree() when parent is specified.
Remaining tasks
- screenshot of the current ui (a before screenshot)
- decide what approach to implement
User interface changes
The proposed siblings browser may introduce new or changed features/functionality in the user interface at admin/structure/taxonomy/tags.
Comment #23 has most recent screenshot of taxonomy terms by vocabulary (before proposed siblings browser).
API changes
API changes/additions might be introduced in core's taxonomy module:
Original report by andypost
Vocabulary has hierarchy field with values from 0 to 2.
So overview page for vocabulary should take into accout hierarchy field.
This is a follow up from:
#762604: Taxonomy terms list broken, term add/edit forgets it's parent
#277200: Add tests for vocabulary hierarchy
catch:
I really dislike that we use taxonomy_get_tree() in taxonomy_overview_terms()
Comment | File | Size | Author |
---|---|---|---|
#83 | 763380-83.patch | 6.66 KB | manojbisht_drupal |
#75 | 763380-75.patch | 6.64 KB | cupcakemuncher |
#73 | 763380-73.patch | 3.86 KB | jesse_k |
#61 | 763380-61.patch | 3.56 KB | ivelin.enchev |
#53 | 763380-53.patch | 3.25 KB | lebster |
Issue fork drupal-763380
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
catchWe should use a similar scheme to #556842: taxonomy_get_tree() memory issues here IMO, disable taxonomy_get_tree() if the vocabulary is over a certain size, not depending on whether it's got hierarchy or not.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commented@catch I've got mixed feelings here. It sounds like we're suggesting that hierarchical vocabularies with more than a certain number of terms can't be supported in Drupal 7 core. The other issue #556842: taxonomy_get_tree() memory issues is about swapping out a form element, but the taxonomy_overview_terms form doesn't have an alternative but to use taxonomy_get_tree or just stop showing hierarchy.
Do we want to change from a tree view to a sibling browser?
Example:
North America (Y number of immediate children)
South America (Z number of immediate children)
Europe ..
Africa ..
Asia ..
Australia etc. ....
Clicking the term refreshes the page to show its children.
This requires only one query per term to show the number of immediate children (and there's sense in avoiding that query as well). Loading the terms on each page could require only one query, yeah?
Comment #3
andypostMain trouble that we have no ability to change vocabulary hierarchy without saving a term or editing with taxonomy_overview_terms()
So I think first we should decide on usability of managing taxonomy vocabularies and terms.
1) managing vocabulary container
2) managing terms
Comment #4
catchYep. I have a site where this page white screens, so that's the case currently, I'd prefer it if rather than wsoding, it just didn't allow drag and drop.
Sibling browser sounds good, but not necessarily possible at this stage of the release cycle, and would still require #698918: Reduce memory usage from \Drupal\taxonomy\TermStorageInterface::loadTree() when parent is specified since taxonomy_get_tree() does the same query regardless of arguments.
Comment #5
klausiSubscribing. Experiencing this nice WSOD at the list term page, when I upgraded a site with a large vocabulary from Drupal 6.
Comment #6
catchIt's bad, but it's been this way forever, downgrading but adding 'major' tag.
Comment #7
klausiI disagree, it has not been forever this way, as it was working in D5 and D6.
1) Upgrade path is broken.
2) a WSOD is not acceptable.
So just because this is lying around for long, it does not mean that it is not critical.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commented@klausi, the upgrade path is broken for different reasons. #706842: Improve comments for the taxonomy upgrade path It would be helpful if you could explain how your vocabulary is larger or deeper than the ones I've tested with already. What causes WSOD? memory or time?
Comment #9
klausiSELECT COUNT(*) FROM `taxonomy_term_data` WHERE `vid` = 2
7184 terms in the vocabulary.
SELECT COUNT(*) FROM (SELECT * FROM `taxonomy_term_hierarchy` group by `parent`) as th JOIN `taxonomy_term_data` ON th.tid = taxonomy_term_data.tid WHERE `vid` = 2
498 terms are parents of at least one other term.
I'm not sure how to get maximum or average depth. The WSOD is caused by a lack of memory and appears after about 8 seconds page loading time.
Tested another vocabulary:
3785 terms, 2 terms are parents of at least one other term. No WSOD, but 17 seconds page loading time to display the first page. Firefox freezes for about 4 seconds while rendering the huge page. Seems like the page contains all terms because they are all children of one single term.
Another bug: there is a pager on the bottom of the page, but when I click next it says "No terms available".
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedKlausi: You should open a new issue. If you're in IRC, ping me with the node ID when you have posted it.
Comment #11
marcingy CreditAttribution: marcingy commentedChanging to major as per tag.
Comment #12
MustangGB CreditAttribution: MustangGB commentedTag update
Comment #13
sun.core CreditAttribution: sun.core commentedCan someone clarify what needs to be fixed here? The WSOD mentioned earlier is a known issue and exists since Drupal 5.
Comment #14
klausiThe exact same vocabulary works fine on the term admin pages in D5 and D6 but causes a WSOD in D7. However, we administer the vocabulary with the taxonomy manager module in D7 now, so investigating this is not a priority anymore for me (I guess one could generate x terms with devel.module in D6 and D7 for comparison and to reproduce the WSOD).
http://drupal.org/project/taxonomy_manager
Comment #15
catchMemory issues with taxonomy_get_tree() may well have regressed since D6, at least one example is #870528: taxonomy_get_parents(), taxonomy_get_children(), and taxonomy_get_tree() do not return a full term objects. However I think this needs to be worked on against Drupal 8 and backported if possible, since whatever we do here is going to mess things up for someone without a major refactoring.
Comment #16
catchRemoving upgrade path tag.
Comment #17
Bojhan CreditAttribution: Bojhan commentedWhat needs review?
Comment #18
xjmI don't know where that tag came from since this is a DX and performance issue, not a UX one.
Comment #19
andypostIs this issue still major? Anyway need a better summary
Comment #20
YesCT CreditAttribution: YesCT commentedI'm also wondering if this is still major.
Anyway.. :) tagging and adding links to contributor task docs to hopefully someone to jump into this.
Comment #21
bdone CreditAttribution: bdone commentedUpdated issue summary per comment #20.
Comment #22
YesCT CreditAttribution: YesCT commentedthanks for updating the issue summary.
I think a screenshot of the current ui would be helpful (a "before")
and another next step would be to decide which of the proposed approaches to the solution to take.
(updating issue summary with those next steps)
Comment #22.0
YesCT CreditAttribution: YesCT commentedbdone issue summary
Comment #23
bdone CreditAttribution: bdone commentedAdding screenshot from: /admin/structure/taxonomy/tags
Taxonomy terms, by vocabulary (before proposed siblings browser)
Comment #23.0
bdone CreditAttribution: bdone commentedupdated remaining steps: screenshot and decide on approach. -c
Comment #24
bdone CreditAttribution: bdone commentedUpdated issue summary to refer to screenshot in #23.
Comment #24.0
bdone CreditAttribution: bdone commentedbdone: adding (before) screenshot
Comment #25
jibranComment #26
YesCT CreditAttribution: YesCT commentedcan we reverify this?
is this a performance bug?
setting to active since there is no patch.
Comment #30
pcambraComment #33
legolasboWe're running into this issue on a media website with 45K tags which runs out of memory trying to display a single page of terms. I've raised the memory limit to over 1GB on my development machine which replaced the out of memory issue with a execution time limit issue. Which made me look into the code that runs this form.
After having looked into this I've found that the main memory issue is caused by the fact that we're calling
loadTree()
with theload_entities
option set to true. This causes all 45K term entities to be loaded into memory, which A fills up the memory and B takes a very long time.I've worked on a quick fix to at least get the overview page accessible again. Reordering the items and then submitting the form still doesn't work, because it runs out of memory because this also loads all entities. I've tried changing the submission handler to use the same approach as I used in the form builder. This does solve the memory issue but even at 250 terms causes time outs. For my current project I don't need the reordering So the attached patch just fixes the overview timeout, but I'd rather have a functional overview which allows me access to term edit pages and such than no overview at all.
This obviously needs follow up, but I'm hoping this patch can land and help other people in the mean time.
Comment #35
legolasboOops, I missed setting the depth.
Comment #36
legolasboThis patch cleans up the code a bit.
Comment #37
timKruijsen CreditAttribution: timKruijsen at Ordina Digital Services for Sijthoff Media commentedThe patch seems to solve at least a part of the issue and therefore I'd say it's good to get this in ASAP. The rest of the problem should be solved in the future.
Comment #38
klausiSuper, the next step is to include a test case in the patch. How can we do a meaningful test?
Comment #39
alexpott@klausi fortunately there is some existing test coverage of the pager - \Drupal\Tests\taxonomy\Functional\TaxonomyTermPagerTest and \Drupal\taxonomy\Tests\TermTest::testTaxonomyTermChildTerms() but in the review below I point out one thing that might be improved.
Can be
$tree = $this->storageController->loadTree($taxonomy_vocabulary->id())
.This should be at the bottom of the doc block underneath the @return. You can't put @see's every where.
There's no need for this to be private - should be protected
Comment #40
legolasboAttached patch addresses the feedback by @alexpott and I fully agree with point 5, this UI needs a lot of work over the course of many iterations probably.
As for adding a test that proves that the memory issue has been resolved, I don't think that's easily done because it requires loads of terms to be created and loaded, which either blows up because of memory consumption or just makes tests run really really long. It's easily tested by hand though.
Comment #43
legolasboHmm, I can't get any tests to run locally. (Getting 50X errors when running them through the simpletest interface) and I don't have time to figure out why. I did test the functionality manually and the pager acts as it is supposed to.
Someone else is going to have to add the missing test coverage.
Comment #44
legolasboAfter looking into this some more, I found out that pagination with nested terms actually IS tested. See
Drupal\taxonomy\Tests\TermTest::testTaxonomyTermChildTerms
. I'm therefore re-uploading the patch from #36 and setting this back to Needs Review.Comment #46
legolasbo?? Test failed with no failed tests?
Comment #47
alexpott@legolasbo there's a problem with bots. In #39 I already pointed out the existing test coverage. But said that it needs to be improved to cover re-ordering terms not on the first page. This is directly related to the patch because it changes how they are loaded.
All the other points from #39 still need to be addressed too.
Comment #48
pdenooijer CreditAttribution: pdenooijer as a volunteer commentedFixed the code changes from #39, the test coverage still has to be extended. Also did some boy scouting, like setting all unset variables to a default value and changed all the checks to identical. I tried to get the simpletest running, but it keeps failing at \Drupal\taxonomy\Tests\TermTest::testTaxonomyNode as it cannot find the node that is created.
Comment #49
pdenooijer CreditAttribution: pdenooijer as a volunteer commentedComment #50
pdenooijer CreditAttribution: pdenooijer as a volunteer commentedComment #52
cenoura CreditAttribution: cenoura as a volunteer commentedReroll over 8.5.0 (763380-36 was failing to apply)
Comment #53
lebster CreditAttribution: lebster commentedPatch #52 can't be applied via composer. Updated patch is attached. Tested on 8.6.9.
Comment #56
jimkeller CreditAttribution: jimkeller commentedWhile it's an improvement to not load entities when rendering the tree, this is still a poor solution. The code for loadTree still slurps in the entire table even if you're only showing 50 rows. We have a site with over a hundred thousand terms in a vocabulary (it's storing product serial numbers) and the overview screen throws a 500 due to memory limit (which makes sense).
The overview page needs to be rebuilt using a range() for the query; loading in the whole table here is ridiculous.
For those looking for an interim solution, you can create a view that lists taxonomy terms; it doesn't call loadTree. You'll also want to set the following in settings.php to keep the edit screen from bombing:
Comment #57
pdenooijer CreditAttribution: pdenooijer as a volunteer commentedFixed in 8.7.x/8.8.x dev, so this can be closed.
Comment #58
lhridley CreditAttribution: lhridley at Promet Source commentedI'm going to reopen.
We are running into this issue on 8.7.3 so I would dispute that this is outdated and that the problem has been addressed. There is no commit associated with the fix, it was simply closed as outdated.
Please provide a link to the relevant "fix" that was included in 8.7 so we can assess whether or not this is still an outstanding issue.
Comment #59
ThomWilhelm CreditAttribution: ThomWilhelm commentedYeah same just ran into this same issue on 8.7.3. None of the previously supplied patches apply anymore.
Comment #60
ivelin.enchev CreditAttribution: ivelin.enchev at FFW commentedReroll #53 for 8.7
Disregard this, as it's from the wrong directory. The below one should be fine.
Comment #61
ivelin.enchev CreditAttribution: ivelin.enchev at FFW commentedComment #62
pdenooijer CreditAttribution: pdenooijer at Ordina Digital Services for RTL Nieuws commentedWe do encounter the problem again, so your right lhridley, this is not outdated.
Comment #64
acb CreditAttribution: acb commentedI can confirm this is still a problem. esp. with fielded terms
Comment #65
Alex Razumov CreditAttribution: Alex Razumov commentedI can confirm. I have the same problem on site with over 30K terms in vocabulary.
Possible solution will be creation of views page for terms overview. In this case all works as expected, but terms manipulation with weight will not work. Also action link "Add new term", term tabs should be rebuilded for views page because route name will be changed.
Comment #67
nord102I'm currently on 8.8.8 and this is still an issue, the patch did not work for me, as I have over 300K terms in one of my vocabularies, I just made a Views page, as others did, to allow to the terms to be paginated and still provide a way to get to the edit page for them without a WSOD
Comment #68
seth.e.shaw CreditAttribution: seth.e.shaw commentedCurrently on 8.9.x and just ran into this issue on a dev site I'm building with 12k terms and no hierarchy. Fortunately upping my max memory was sufficient for now. But I shouldn't have to double my max memory for a single page; not to mention the eventual problem of timeouts as the taxonomy grows (which I'm sure it will).
Comment #69
seth.e.shaw CreditAttribution: seth.e.shaw commentedI've tested the patch on #61 for 8.9.5 and it works well for me. I realize that the version for this ticket was bumped to 9.x, but I would *really* like it as a bug-fix on the 8.9.x branch. Who do I need to convince to make that happen?
Comment #72
jesse_k CreditAttribution: jesse_k commentedThe latest patch did solve the memory issues on Drupal 8.9.14, but we did run into a bug when trying to display terms with multiple parents:
Comment #73
jesse_k CreditAttribution: jesse_k commentedI created a patch that fixes the multi-parent issue I described above.
Comment #74
cupcakemuncher CreditAttribution: cupcakemuncher as a volunteer commentedI am also subscribing to this.
When you have a large vocabulary where the bulk of the terms is located in the leaves of the hierarchy-tree and this tree has 3-5 levels a significant portion of the tree will be loaded for a lot of pages, especially when there are only 1 or 2 root elements.
You do not necessarily encounter WSOD but you will be unable to change the hierarchy via drag and drop using the form.
A submit would display the changes but a following reload or look into the DB-tables will show that the change was not written to DB.
The problem is that the form will always do a recursion until it finds a root element.
I only ran Unit-tests on the patch and based it on jesse_k's patch, but it is meant as a discussion point, because fixed page sizes will allow for more stability when faced with a deep and wide hierarchy tree.
In the example you could still manipulate the tree to you liking, but, admittedly, the usability does suffer without some update to the frontend code.
//Edit
The form will then also suffer from terms who's key is not in the user-input. This will generate a lot of notices
> Notice: Undefined index tid:883883 in Drupal\taxonomy\Form\OverviewTerm->buildForm()(line256 of ......)
//Edit 2:
Please also note: meant as a discussion point only to show how keep the vocabulary editable, the current patch is not stable in regards to the weights ATM. --> changing the hierarchy alters the position in regards to the weights.
Comment #75
cupcakemuncher CreditAttribution: cupcakemuncher as a volunteer commentedComment #82
Amavi CreditAttribution: Amavi commentedI correct it on my website Drupal 10:
I up the SQL "max_allowed_packet" on my hosting to maximum 64MB and all is oki, it is configured at 1MB basically and too low for Drupal, best ways may be is to write in Tutorial to check this??? I read 8MB is a minimum for Drupal 9....
Comment #83
manojbisht_drupal CreditAttribution: manojbisht_drupal as a volunteer and commentedPatch 74/75 is not applying for 10.2, so created a new patch for 10.2
Comment #84
catchI think this is a duplicate of (or technically duplicated by, since the other issue is newer than this even though it's also a decade old) #2183565: Avoid loading all terms on the taxonomy overview form.
Postponing on that issue given it's RTBC.