Terms lose <root> as the parent when editing

Confirmed. I'm going to look into it further.

I noticed what I think is another bug. When you give parents of root plus a real term, the drag handles disappear from the term list. That's correct and as expected but they don't return again if the term is then intentionally saved with another term as its only parent. The only way to make the drag handles return seems to be to save with root being the only parent.

I edited the description. Angle brackets around the word root was making it disappear as an unknown html tag. :)

Lets try this.

I added the TermStorage::hasRootAsParent method rather than change TermStorage::loadParents to include root because there is code that calls loadParents which expects to get only entities back.

Thanks @timmillwood. I'll add a test in the next day or so.

I guess we need to sort.

I've created a related issue https://www.drupal.org/node/2900139. It's tempting to fix them both in one patch but I guess it's best to keep them separate. It will be easy to fix that one after this one is committed.

+++ b/core/modules/taxonomy/src/TermStorage.php
diff --git a/core/modules/taxonomy/src/TermStorageInterface.php b/core/modules/taxonomy/src/TermStorageInterface.php
index 05d8d7f4c7..a4f386eec0 100644

index 05d8d7f4c7..a4f386eec0 100644
--- a/core/modules/taxonomy/src/TermStorageInterface.php

--- a/core/modules/taxonomy/src/TermStorageInterface.php
+++ b/core/modules/taxonomy/src/TermStorageInterface.php

+++ b/core/modules/taxonomy/src/TermStorageInterface.php
+++ b/core/modules/taxonomy/src/TermStorageInterface.php
@@ -118,4 +118,18 @@ public function resetWeights($vid);

TermStorage isn't a base class, so I'm not 100% sure if we can add a method to TermStorageInterface.

Has anyone checked whether this bug exists in 7.x? A quick review of the code suggests that it does. I can see this surviving for years given multiple parents usage is rare, and multiple parents with root as one of the parents likely rarer.

I don't think we'd want to add hasRootAsParent() to TermForm. That would mean injecting database into TermForm which doesn't feel good but I guess that would work.

I don't really understand what the problem is adding an additional method to TermStorageInterface. I guess I'm missing something because it seems that not being a base class, i.e., being taxonomy specific, is a good thing. I can understand that adding taxonomy specific code to its parent, ContentEntityStorageInterface would clearly be wrong.

That makes sense. I see what you mean.

I'm happy to do another patch with that method in TermForm if you think that's the right thing to do I wonder if that causes a bigger issue. It would totally tie TermForm to SQL. I would have thought something like TermForm should be storage agnostic. That might be more a theory than reality but it seems to be way too far down the inheritance chain to be aware of storage implementation.

+1 to #18. With no base class I don't see a way around the additional interface - we can have it extend TermStorageInterface and deprecate TermStorageInterface for 9.x

ok sounds good.

So I should make a new interface extending TermStorageInterface. Any thoughts on what I should call the new interface?

Could we do something like Drupal\taxonomy\Entity\TermStorageInterface? Usually we don't put the handlers inside the entity namespace but I don't really see why not? Or Drupal\taxonomy\Term\TermStorageInterface if not?

See how this goes. I went for Drupal\taxonomy\Entity\TermStorageInterface

I wasn't sure what, if any, comment I should put at the top of either the old or new interface.

I removed an unnecessary query for $parent.

working on it as part of the DrupalCon Vienna sprints

I tested on Drupal 9.4.x and confirmed this is still a problem. I am postponing the related issue on this one.

I added some remaining tasks and tags to clarify Novice tag. The issue summary also could be updated with @catch's proposed resolution in the proposed resolution section as well as filling out other sections based on the latest patch.

Rerolled patch for #22 against 9.4

Rerolled patch against #37 for 9.4.x. Fixed custom commands

Couple of change suggestions:

1.   public function hasRootAsParent($tid) {
       if (!isset($this->hasRootAsParent[$tid])) {
         $query = $this->database->select('taxonomy_term_hierarchy', 'h');
         $query
           ->condition('h.tid', $tid)
           ->condition('h.parent', 0);
   
         $count = $query->countQuery()->execute()->fetchCol()[0];
         $this->hasRootAsParent[$tid] = $count == 0 ? FALSE : TRUE;
       }
       return $this->hasRootAsParent[$tid];
     }
   

   As per 8.6.0 release note (https://www.drupal.org/node/2936675) taxonomy_term_hierarchy is deprecated, so we can change it to something like this...

     public function hasRootAsParent($tid) {
       if (!isset($this->hasRootAsParent[$tid])) {
         $query = $this->database->select('taxonomy_term__parent', 'ttp');
         $query
           ->condition('ttp.entity_id', $tid)
           ->condition('ttp.parent_target_id', 0);
         $count = $query->countQuery()->execute()->fetchCol()[0];
         $this->hasRootAsParent[$tid] = $count == 0 ? FALSE : TRUE;
       }
       return $this->hasRootAsParent[$tid];
     }
   

2. Phpcs error need to be fix for core/modules/taxonomy/tests/src/Functional/TermTest.php on line 570

Looks like patch #41 add some issues and #42 some suggestions that should be looked at.

@smustgrave,
I made the changes as required at comments #42
Needs review.

@smustgrave,
Sorry for the previous patch.
I made the changes as required in point 1 at comments #42
Needs review.

Still looks like the patch is having issues

Fixed a syntax issue

Fixing test case

Applied patch #49 and it fixes the issue.Attaching screen recordings for reference.

If you feel the issue is fixed please move to RTBC

I have tested the patch from comment #50 on 9.4.6-dev and 9.5.0-dev. Verified and the patch is applied successfully fixing the issue on both 9.4.6-dev and 9.5.0-dev.

Moving to RTBC
Looks good to Merge!

Looks good to merge!

I don't think we need to add a new method using SQL to term storage for this. I think we can get the parents from the entity and be done. See attached patch.

Whoops my test didn't fail against HEAD because of caching stuff and the test not testing what it says it was testing.

#55 Patch Applied on 9.5.x-dev and it fixes the issue.

Also manually tested this and appears working.

Also from the videos in #58 think remvoing the manual testing is fine.

phpstan was complaining, fixed this on commit:

diff --git a/core/modules/taxonomy/tests/src/Functional/TermTest.php b/core/modules/taxonomy/tests/src/Functional/TermTest.php
index f87fd78d03..819578b227 100644
--- a/core/modules/taxonomy/tests/src/Functional/TermTest.php
+++ b/core/modules/taxonomy/tests/src/Functional/TermTest.php
@@ -613,6 +613,7 @@ private function reloadTermByName(string $name): TermInterface {
    *   A sorted array of tids and 0 if the root is a parent.
    */
   private function getParentTids($term) {
+    $parent_tids = [];
     foreach ($term->get('parent') as $item) {
       $parent_tids[] = (int) $item->target_id;
     }

Committed to 10.1.x and cherry-picked back through to 9.4.x, thanks!

Reverting as this broke HEAD :(

The patch in #56 is no longer good for 10.x - works on 9.5 and 9.4 though...

Fixed the test. This was caused by #2826592: No redirection to term view display page from term edit page. The new patch will work on Drupal 9 and 10 and as the changes are test only setting back to RTBC.

Whoops, thanks for fixing. Committed/pushed to 10.1.x and cherry-picked back to 9.4.x again.

This change broke the rdf_taxonomy sub-module of https://www.drupal.org/project/rdf_entity:

--- a/core/modules/taxonomy/src/TermForm.php
+++ b/core/modules/taxonomy/src/TermForm.php
@@ -23,7 +23,12 @@ public function form(array $form, FormStateInterface $form_state) {
     $taxonomy_storage = $this->entityTypeManager->getStorage('taxonomy_term');
     $vocabulary = $vocab_storage->load($term->bundle());
 
-    $parent = array_keys($taxonomy_storage->loadParents($term->id()));
+    $parent = [];
+    // Get the parent directly from the term as
+    // \Drupal\taxonomy\TermStorageInterface::loadParents() excludes the root.
+    foreach ($term->get('parent') as $item) {
+      $parent[] = (int) $item->target_id;
+    }
     $form_state->set(['taxonomy', 'parent'], $parent);
     $form_state->set(['taxonomy', 'vocabulary'], $vocabulary);

Specifically, we're swapping the storage of the taxonomy_term entity in order to store the terms in a triplestore backend. Along with this, the TIDs are strings (URIs), not integers. Casting here as integers is prohibiting us to use TermForm as it is.

At least, is it possible to move the parents computing in a protected method, so it would make easier to extend the class w/o copy/paste the whole TermForm::form() method?

I've created #3332877: Added TermForm::getParentIds for allowing to override in contrib to fix the issue.