Explore storing distinct trees for each revision via a TREE ID column

I think the answer here is, yes - this is by design.

The nested set representation is primarily concerned with the default revisions.

Whilst this means you cannot view forward revision trees or backward revision trees, I'm not too concerned about that.

But I need to fully digest what you're proposing and the video before I comment fully.

There is logic to move sub-trees when an entity revision becomes the default, but it may not be working as expected.

I will try and write a kernel test to describe your scenario, as this will help my understanding (feel free to beat me to it).

another factor here, we use ER and not ERR field, so the parent can only ever refer to one revision, not a specific revision, which I think supports my case that this can only work for the default revision and not for any revision at given point in time.

I should point out, I'm not averse to this change, just that I doubt I'll have capacity to work on it.

It would require changes at three levels as follows:

• We'd need to subclass entity_reference_revisions field instead
• We'd need to update the nested set library to include the tree ID (note the tree ID could be the revision ID of the root drupal entity)
• We'd need to update the integration code and tests

I should note that whilst the linked detail includes a tree ID, we use one table per tree here, so perhaps we might not need changes to the library, although it would make views integration tricky

I think this failing test demonstrates what's going wrong here. The original parent still has reference to the old revision of the child, so on the Children tab of the original parent you still see the child.

That is the intended behaviour of the tree, but if it is showing on the children tab, then yeah - we need to filter out non default revisions from that page.

The idea being that if you have a forward revision (draft) that has a new parent, you should still find the default revision (published) against the old parent.

The idea being that if you have a forward revision (draft) that has a new parent, you should still find the default revision (published) against the old parent.

That sounds sensible, and a scenario whereby users would expect to see a node appear twice in the tree. (Hopefully, with the titles of the corresponding revisions, rather than necessarily the latest revision).

However, if I understand correctly, the video shows an example where the forward revision has been published, so we'd expect the previous revision to be unpublished and removed from the tree. (Presumably that's why you've suggested a filter for non-default revisions.)

Would I be right in thinking that a workaround would be to avoid creating a new revision at the same time as changing a parent? (I'm inferring that that's what's caused the "logic to move sub-trees when an entity revision becomes the default" to produce this unexpected result.)

No haven't sorry

\Drupal\entity_hierarchy\Form\HierarchyChildrenForm::form calls \Drupal\entity_hierarchy\Storage\EntityTreeNodeMapper::loadAndAccessCheckEntitysForTreeNodes which calls \Drupal\entity_hierarchy\Storage\EntityTreeNodeMapper::loadEntitiesForTreeNodesWithoutAccessChecks which has this code

$entities = $this->entityTypeManager->getStorage($entity_type_id)->loadMultiple(array_map(function (Node $node) {
      return $node->getId();
    }, $nodes));
    $loadedEntities = new \SplObjectStorage();
foreach ($nodes as $node) {
      $nodeId = $node->getId();
      $entity = isset($entities[$nodeId]) ? $entities[$nodeId] : FALSE;
      if (!$entity || ($entity->getEntityType()->hasKey('revision') && $node->getRevisionId() != $entity->getRevisionId())) {
        // Bypass non default revisions and deleted items.
        continue;
      }
      $loadedEntities[$node] = $entity;
      if ($cache) {
        $cache->addCacheableDependency($entity);
      }
    }

In this scenario $node is a nested set node from the tree and $entity is the Drupal entity.
We're calling loadMultiple which only returns default revisions and we're then dropping tree nodes if the revision ID doesn't match the default one.

This was changed in #2904307: Reorder children saves every revision needlessly in August 2017 which was after this was created

I suspect the issue regarding non default revisions being shown in the children tab no longer exists.

However, I'm still interested in the tree ID idea but on large sites the size of the tree and the number of rows to update can be problematic, so that would likely exacerbate the issue adding more records to the table.

I'm going to move this to a feature request to explore that.

I wonder if #3411981: CTE Rewrite makes any of this easier?