Use entity reference for taxonomy parents

I've been talking to @catch about this for a while, I guess now I have to get to work :)

Even 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.

1. Rename \Drupal\Core\TypedData\TypedDataInterface::getParent() to something else (getParentStructure(), getParentDataType(), getParentObject() ?)
2. Introduce a TreeInterface with basic methods for working with hierarchical data
3. Make TaxonomyTerm implement this interface and refactor taxonomy_get_tree() to its methods
4. Repeat for MenuLink (this allows us to kill/move all menu links-related code from menu.inc) and Comment
5. Introduce the ability to define default configurable (Field API) fields in baseFieldDefinitions()
6. Port the Tree module to D8 - this should be somewhat easy because it's based on Entity reference and not many things changed
7. Profit?

While "typed data guys" need to agree and maybe work on step 1), we can discuss the interface here. Here's my proposal :)

Okay, 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?

interface TreeInterface {
  function getTreeParents();
}

?!

Updated issue summary with related issues.

While 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.

The 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().

bump - I think this is unblocked now?

Having a TreeInterface would be amazing for things like Hierarchical Select… :)

Also, we could add LoraxInterface, you could only talk to TreeInterface via it.

Bump. Would be nice :)

Not 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.

Addressing the test fails.

1. The reserialization in EntityTest will transform array(array(target_id => 0)) to NULL. @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 regarding target_id => 0 in another issue.
2. The target_type defaults to node or user, so we need to specify it in baseFieldDefinitions().
3. Validating an invalid parent field now gives two violations, so I added a temporary workaround for that, which is explained in the patch. On the other hand, maybe we should simply remove TermParentConstraint[Validator]? It seems to be equivalent to ValidReferenceConstraint[Validator] in this context.

1. +++ b/core/modules/hal/src/Tests/EntityTest.php
   @@ -93,13 +93,21 @@ public function testTerm() {
   +    // If a parent is not set, the test fails because target_id => 0 is
   +    // reserialized to NULL.
   +    $term_parent = entity_create('taxonomy_term', array(
   +      'name' => $this->randomMachineName(),
   +      'vid' => $vocabulary->id(),
   +    ));
   +    $term_parent->save();
   

   Can 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.

2. +++ b/core/modules/taxonomy/src/Tests/TermValidationTest.php
   @@ -62,7 +62,12 @@ public function testValidation() {
   -    $this->assertEqual(count($violations), 1, 'Violation found when term parent is invalid.');
   +    // @todo There are two violations because the TermParentConstraint, added to
   +    // the parent field in Term::baseFieldDefinitions(), overlaps with the
   +    // ValidReferenceConstraint specified by EntityReferenceItem. Resolve this
   +    // in https://drupal.org/node/2023465.
   +    $this->assertEqual(count($violations), 2, 'Violation found when term parent is invalid.');
        $this->assertEqual($violations[0]->getMessage(), format_string('%id is not a valid parent for this term.', array('%id' => 9999)));
   +    $this->assertEqual($violations[1]->getMessage(), format_string('The referenced entity (%type: %id) does not exist.', array('%type' => 'taxonomy_term', '%id' => 9999)));
   

   We possibly no longer need the custom validation.

   What happens if you try to validate it with parent_id = 0? are we testing this?

Removed 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.

Looks clean and proper. RTBC

Committed 320ebd0 and pushed to 8.0.x. Thanks!