Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Taxonomy hierarchy is really a reference between entities, with some extra stuff added on top. So this could potentially use entity reference.
Closely related issues:
#344019: New implementation for database tree parsing ( taxonomy / comment )
#1207326: Refactor taxonomy hierarchy API for performance, consistency, and convenience
Let's hope we'll get it right this time :)
Comment | File | Size | Author |
---|---|---|---|
#15 | taxonomy-term-parent-1915056-15.interdiff.txt | 5.3 KB | Arla |
#15 | taxonomy-term-parent-1915056-15.patch | 8.94 KB | Arla |
#13 | taxonomy-term-parent-1915056-13.interdiff.txt | 4 KB | Arla |
#13 | taxonomy-term-parent-1915056-13.patch | 7.72 KB | Arla |
#11 | taxonomy-term-parent-1915056-11.patch | 4.42 KB | Berdir |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedI've been talking to @catch about this for a while, I guess now I have to get to work :)
Comment #2
amateescu CreditAttribution: amateescu commentedEven though this issue was opened for taxonomy terms, my plan was to do this all over core (menu links and comments included). I've been busy with various other D8 work until now, but I still think this is viable for D8 if we follow some well defined steps, which, if done in this order, do not bring us farther from a release.
\Drupal\Core\TypedData\TypedDataInterface::getParent()
to something else (getParentStructure(), getParentDataType(), getParentObject() ?)While "typed data guys" need to agree and maybe work on step 1), we can discuss the interface here. Here's my proposal :)
Comment #3
fubhy CreditAttribution: fubhy commentedOkay, agreed with most of #2 although I am a bit defensive when it comes to making menu link hierarchies and basically every other hierarchical data structure that we have field-based (and even configurable-field based). I wonder how much that affects performance. I am also a bit worried about the baseFieldDefinition thing and how that would be reflected in the UI, etc. - No idea how you (field api) guys envisioned that but it does feel a bit weird to have entity-level code that depends on configured field structures. Might just be me though...
Other than that, I am very +1 on having a unified solution for hierarchical data structures in core including a lower level API / interfaces for that. I am not sure on the approach/decision for closure table vs. nested sets as I find the nested sets algorithm much harder to grok and did not yet take a look at how the D7 Tree module does the restructuring of the hierarchies upon insert/reordering of tree elements as that is a major pain-point with nested sets usually due to the lack of hierarchy integrity. A closure table feels much simpler to grok and implement although I do see potential problems with the size of the closure tables as they become pretty big rather quickly [O(n^2)] :/. Anyways, whatever we decide to provide by default should be pluggable/replacable so that @chx loves us all.
Everything else in your list seems reasonable to me. Seems like a good battle plan. When do we start?
Maybe, since TreeInterface is also very generic (as typed data), we could instead go with something more verbose and thereby prevent method name conflicts on our side?
?!
Comment #4
amateescu CreditAttribution: amateescu commentedComment #4.0
amateescu CreditAttribution: amateescu commentedUpdated issue summary with related issues.
Comment #5
yched CreditAttribution: yched commentedWhile I agree that TypedData::getParent() is too obnoxious, the right approach cannot be to rename it so that the treeInterface can take the method name and be the obnoxious one instead :-)
Secondary interfaces need to prefix their method names.
Other than that, not sure to what extent 5. is important for this plan, but I dont see it happening. Entity::baseFieldDefinitions() is about base fields, you won't be defining configurable fields in there.
Comment #6
amateescu CreditAttribution: amateescu commentedThe plan will change quite a bit after #2142549: Support multi-column field types in base tables :) It makes point 5. obsolete since the tree field can now be a base field (we didn't have this "luxury" in D7) and now I want the FieldItem class to implement TreeInterface, so we end up with something like: $menu_link(/$term/$comment)->getTree()->getParent().
Comment #7
larowlanbump - I think this is unblocked now?
Comment #8
Wim LeersHaving a
TreeInterface
would be amazing for things like Hierarchical Select… :)Comment #9
larowlanAlso, we could add LoraxInterface, you could only talk to TreeInterface via it.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedBump. Would be nice :)
Comment #11
BerdirNot seeing any activity on the whole tree stuff and menu went in a different direction. So here's a minimal patch converting it to an entity reference field without removing any of the weird stuff.
That should be enough to make it work for REST/default content.
Comment #13
ArlaAddressing the test fails.
array(array(target_id => 0))
toNULL
. @Berdir informed me that 0 is a special value for the term parent field. I worked around it here by setting an existing parent. We should probably define the behaviour regardingtarget_id => 0
in another issue.Comment #14
BerdirCan we add a @todo here to the other issue you opened? Because with the current behavior, we need a way to explicitly pass along 0 to update terms to no longer have a parent.
As mentioned, for this specific case, I'm wondering if we should simply make this a normal field, always load it. It slows down updating, but we added data tables and stuff, which is certainly the bigger problem for mass-updates of terms. And for loading, we have the cache now.
We possibly no longer need the custom validation.
What happens if you try to validate it with parent_id = 0? are we testing this?
Comment #15
ArlaRemoved the superfluous constraint+validator, added a
@todo
about #2327935: Allow empty entity IDs in EntityResolvers, and added a validation test for parent with id = 0.Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedLooks clean and proper. RTBC
Comment #17
alexpottCommitted 320ebd0 and pushed to 8.0.x. Thanks!