Create TermInterface methods :: isTopLevel(), :: isChild()

Great idea 👍💡

I'll do some groundwork.

All set! I've added the method and added some tests as well.

Thanks, this looks like a nice feature.

Tested the MR and for terms with only one parent it seems to work. However, taxonomy terms could have mutiple parents and if using $this->parent->target_id, I think we are checking only the first parent, not all parents.

Imagine a situation where you have a term with two parents - one/first parent is a root (= 0) and the second parent is another term (!= 0). The MR will return FALSE for such term, despite the fact that it has also regular parent, because it will check only the first parent (which is root = 0). I think we need to loop thru all parents to check if there is a valid one.

Hi poker10 you're right. When I created the issue, I was under the impression that parent contains the IDs of all the ancenstors. I had never noticed that a taxonomy term can have multiple parents and also be a top-level term at the same time. Thanks.

I've updated the code and the tests to include the case that you specified. However, I have two thoughts:

1. Technically, root (0) is also a parent, so the method name ::hasParent() doesn't seem adequate any more. I'm thinking about making it "isTopLevel()". Any opinions?
2. In the long-term, maybe Drupal Core should consider renaming the field "parent" to be "parents" because it can contain multiple values.

I'll wait for an opinion on ::isTopLevel(). Apart from that, it seems to be working.

What about both?

isTopLevel and hasParents could both be useful and several different purposes I think. Example 3 levels, you could have a parent term that's not topLevel.

Whatever is decided will need a simple CR for announcing new methods.

Indeed smustgrave. I'll take care of it later today. I think I'll go with the following names that seem more self-explanatory:

1. ::hasRootParent(): bool
2. ::hasNonRootParent(): bool

All set. I've updated the code and the test.

On a similar note, if this issues ✨ Create method EntityReferenceFieldItemList::referencedIds() Needs review gets updated soon, then I can use ::referencedIds() in this method.

Mind writing a quick change record.

Sure thing. I've created a change record.

Has test coverage
Has a CR
Typehints appear correct.

Think this is a good addition!

This should probably get taxonomy subsytem maintainer review. Fortunately, I am a taxonomy subsystem maintainer.

Good feedback from @poker10 and @smustgrave. Adding credits. When I started reading the issue I almost NWed it because of multiple term parenting; glad to see it got addressed in the course of the issue.

Technically, adding methods to an interface is a BC break. However, given the 1:1 relationship between TermInterface and Term, this point of the BC policy → applies:

Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.

However, since it is a BC break, I think we need to make the CR more explicit in saying that if they are implementing TermInterface directly without using Term, they will need to add implementations of these two methods.

Overall, I think this is a good addition to the API, especially since there are oddities with the term storage where <root> is not returned in the parent data. I hemmed and hawed a bit about the method names. However, I couldn't come up with anything better. It's good that the method names are parallel, and they're probably the best we can do given multiple parenting.

NW for the change record updates and the comments on the MR, but also giving my signoff on the change as a whole. Nice work!

Also the followup for a constant for the root term ID.

Re-saving credits -- something ate them.

Thanks for the review. I've updated the CR as per your feedback.

Ah! I always forget something before hitting save.

Aside: This will be my first significant contribution to the Drupal Core (= A good way to mark 10 years of working with Drupal.

Opened follow up,

Appears to be some feedback in the MR.

I've addressed both the comments, so I'm marking this as "Needs review" again.

Believe feedback has been addressed.

I think hasRootParent() is potentially confusing - to me that sounds like the term has a parent that is the root of a tree, i.e. second level. IMO terms at the top of the tree don't have a parent at all - although we store it as 0, there is no entity representing 0, what we really store is multiple trees in one vocabalury or more precisely a graph.

I think we should consider going back to the earlier idea of isTopLevel() or maybe isRoot(). For me those still cover the edge case where a term is both at the root level and also a child of another term.

isTopLevel doesn't sounds bad.

Also wondering - instead of hasNonRootParent() which sounds like it's checking if the term is third level instead of second level, what about isChild()?

Updating title and remaining tasks list.