Avoid loading all terms on the taxonomy overview form

We just ran into this issue migrating a Drupal 7 website with a large taxonomy to Drupal 10. So this is still important to fix, as it breaks large sites. Reading the comments above, I think we still need a plan on how to fix this deep underlying issue, before we invest more time in code here?

Not sure why this issue derailed. The patch in #45 🐛 Avoid loading all terms on the taxonomy overview form RTBC worked for me in several tests/sites with large taxonomies and had the blessing by Catch → . All that remained was to add a comment. In all honesty I didn't understand exactly what the comment was but I believe once that comment is added this was RTBC.

There is also a version for 10.x. in #54 🐛 Avoid loading all terms on the taxonomy overview form RTBC . I recommend you test that version and leave a comment if it works.

I don't recommend the solutions that try to increase the memory size as suggested as they will eventually break. The proposed solution worked for me on two large sites I tested.

Patch #54 works well with the latest Drupal 10.0.9. Thanks.

Created MR and patch against 11.x based on #49and including suggestions by catch in #52

As a bug will need a test case showing the issue please

Thanks!

Thank you for the heads-up smustgrave.

Attaching new version of the patch including tests and improving the load terms logic for the submit handelr as well.

The last submitted patch, 67: 2183565-67-test-only.patch, failed testing. View results →

New patch version fixing typo.

smustgrave → credited bbrala → .

Giving credit to bbrala for pointing me out to a good way for doing a profile using ddev and phpstorm.

Using devel generate created 1000 terms and went to the taxonomy overview page of my vocabulary

Attaching before/after

Before

After

So seeing a slight faster render.

The last submitted patch, 69: 2183565-69.patch, failed testing. View results →

Patch 2183565-69.patch is not working for me. Still getting:
Fatal error: Allowed memory size of 805306368 bytes exhausted (tried to allocate 4096 bytes) in /var/www/web/modules/contrib/taxonomy_menu/src/TaxonomyMenuHelper.php on line 104
Fatal error: Allowed memory size of 805306368 bytes exhausted (tried to allocate 24576 bytes) in Unknown on line 0

Hi :)

Since 9.4 all my taxo more big 2000 is block, I put this patch and works now:
https://www.drupal.org/project/drupal/issues/3327118#comment-15252799 🐛 Chunk multiple cache sets into groups of 100 to avoid OOM/max_allowed_packet issues Fixed

But, all big content is still block: Error Vanish cache server.

I can not make Sitemap too for big content, Drupal 9.4 break all web site... :(

Moving back to RTBC (per #71) - there was a random failure.

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -520,9 +527,10 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
 
     // Build a list of all terms that need to be updated on previous pages.
     $weight = 0;
-    $term = $tree[0];
-    while ($term->id() != $form['#first_tid']) {
-      if ($term->parents[0] == 0 && $term->getWeight() != $weight) {
+    $raw_term = $tree[0];
+    while ($raw_term->tid != $form['#first_tid']) {
+      if ($raw_term->parents[0] == 0 && $raw_term->weight != $weight) {
+        $term = $this->storageController->load($raw_term->tid);
         $term->setWeight($weight);

I think it would be worth building the list of term IDs in advance, and then using ::loadMultiple() here.

Similar in the next hunk of the MR too.

Otherwise looks good though.

Good point.

Added new version of the patch storing in an index all the entities and their weight to load all of them in a single call afterwards.

Add missing interdiff.

There are no test failures in the patch #77, but I am curious how is the while loop supposed to work now, as the $raw_term variable is not being changed at all?

@@ -526,14 +533,13 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
     $weight = 0;
+    $raw_term = $tree[0];
+    $term_weights = [];
+    while ($raw_term->tid != $form['#first_tid']) {
+      if ($raw_term->parents[0] == 0 && $raw_term->weight != $weight) {
+        $term_weights[$raw_term->tid] = $weight;
       }
       $weight++;
-      $term = $tree[$weight];
     }

Good catch!

That part of the code is only affected if we are loading a page that's not the first and root terms weight is not starting by 0.

Added extra tests to cover that case.

While working on this, I found out there are no tests to ensure that overview terms form submit callback behaves as expected. Should they be added here or maybe better to open a follow up issue for that?

Thanks!

Fix typo in comment.

Failed asserting that actual size 11 matches expected size 5.

Ran new tests to make sure they failed and clearly they did.

Seems feedback has been updated.

1. +++ b/core/modules/taxonomy/tests/modules/taxonomy_test/taxonomy_test.module
   @@ -46,3 +46,16 @@ function taxonomy_test_form_taxonomy_term_form_alter(&$form, FormStateInterface
   +  $value = \Drupal::state()->get(__FUNCTION__);
   +  if (isset($value)) {
   +    foreach ($entities as $entity) {
   +      $value[$entity->id()] = $entity;
   +    }
   +    \Drupal::state()->set(__FUNCTION__, $value);
   +  }
   

   I don't think we should be save the term objects to state.

   I think this could be

   $value = \Drupal::state()->get(__FUNCTION__);
   // Only record loaded terms is the test has set this to an empty array.
   if (is_array($value)) {
     $value = array_merge($value, array_keys($entities));
     
     \Drupal::state()->set(__FUNCTION__, array_unique($value));
   }
   </codede>
   </li>
   
   <li>
   <code>
   +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyTermPagerTest.php
   @@ -73,4 +73,48 @@ public function testTaxonomyTermOverviewPager() {
   +  public function testTaxonomyTermOverviewTermLoad() {
   +    // Set limit to 3 terms per page.
   +    $this->config('taxonomy.settings')
   +      ->set('terms_per_page_admin', '3')
   +      ->save();
   +
   +    $state = $this->container->get('state');
   +    $state->set('taxonomy_test_taxonomy_term_load', []);
   

   The first state->set() is not required.

I've attached a patch that does the above. It also adjusted things so that we use loadMultiple() in the buildForm - this is very similar to what @catch requested in for the submit in #76

Ah actually we already have a list of tids to load :) no need to built yet-another-list-of-tids...

Updated the MR with all the changes from the issue after the last commit to it. Hiding all the files.

Seems you addressed your own feedback :)

This is not a deep performance benchmark, but I can confirm that the MR !4110 is working on few our sites with thousands of taxonomy terms. Before changes from MR were applied, there was an OOM error on the taxonomy overview page (admin/structure/taxonomy/manage/vocabulary/overview). After applying the changes, the page is working again.

I think a more detailed benchmark could help for this to be committed (for example memory usage on that page before/after, etc..), as there is a "needs profiling" tag, but otherwise I think this works pretty well.

I think it should be possible to write a performance test for this using 📌 Allow assertions on the number of database queries run during tests RTBC - I wouldn't block it on that, this issue has existing for many years longer than that capability, but I'll open a spin-off and try to do a before/after comparison if I can and then we can commit the test coverage in its own issue.

I opened 📌 Performance tests for the taxonomy overview page Needs review and wrote the test, then merged the branch here in and ran the test again - and as I was writing the test coverage, realised we multiple load taxonomy terms in taxonomy get tree, which will also multiple get the cache items, and multiple set the cache items too. Given that, we can't detect the performance improvement here with PerformanceTestBase and the test run after the merged branch confirmed that - we're exchanging a massive database query for a smaller one, and nothing runs per-term.

Bit of a shame that the tests can't detect it, but also if it had detected a difference, there'd be something wrong with multiple loading, it the test would have least have probably detected the multiple load regression fixed in #84 though so worth adding anyway

Committed/pushed to 11x and cherry-picked to 10.2.x, thanks!

PS this issue impacts not just the taxonomy overview page, but taxonomy edit, batch update etc etc. (using Drupal 10.2.0 and php 8.1)
I have a large location based taxonomy with continents -> countries -> cities and unable to make edits, bulk update url aliases...

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