Revisions on relations are not loaded correctly resulting in wrong data in includes

#33 Re-roll against 10.x.1 branch. Applied successfully, but custom command failing. required NW

I just had a look at #33 and I'm not sure why we are using

+use Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem;

Started to re-roll for 11.x but the changes to \Drupal\jsonapi\IncludeResolver::resolveIncludeTree from 🐛 ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type" Fixed have complicated this.

Re-rolled the patch against 10.2.x, still testing needs to be done on this re-rolled patch.

We can't reference classes from entity_reference_revisions in core, this needs a reroll starting from #21

Thanks @acbramley
I have re-rolled it from #21 against 10.2.x

I don't think the re-roll is that simple. Now we're only adding to the revision_ids if the is_subclass_of condition fails.

Opened MR against 11.x which is a re-roll of #21, but moves bits out of the is_subclass_of condition. This also applies to 10.2.x.

As mentioned in #45, the patch for v10.2.x in #44 doesn't cover both conditions in the if/else. Using that patch, I wasn't seeing the results I expected, so I made this patch that covers both conditions, and now I see the results I expected.

Hiding patches to avoid bot confusion.

Patch #48 works for me, but

list($target_type, $revision_type) = explode(':', $target_type_and_rev);

assumes that the $target_type_and_rev will always have 2 values. This is not the case, so you end up with tons of warnings>
I've changed it to

list($target_type, $revision_type) = array_pad(explode(':', $target_type_and_rev), 2, NULL);

and no more warnings.

Attaching the revised patch.

The line:

list($target_type, $revision_type) = array_pad(explode(':', $target_type_and_rev), 2, NULL);

throws a warning, I added '@' to suppress it since $revision_type is expected sometimes to be NULL.

Applied on Core 10.2.X

I rebased the MR and pushed a new commit with a different approach where I'm using the entity from the field item instead of loading it again from entity storage. Not sure if it has some downsides but it seems to work fine when testing manually.

This approach seems great! Thanks for moving this forward.

We do still need test. Probably best to look at JsonApiRegressionTest and then testDeepNestedIncludeMultiTargetEntityTypeFieldFromIssue2973681 which also creates some nested relations and tests those. It would just need to install content_moderation and make a forward revision.

Awesome, thanks!

Sorry for not addressing the needed tests. I'm not sure if test is needed since we're not changing any of core's behaviour, we're not expecting core's entity reference field to load a future draft(?) So it should already be covered by existing tests? My understanding here is that this allows modules like paragraphs to have a say in what revisions are loaded, but core should still works the same.

I could be wrong and I'd be happy to try and write a test for this. But I'd need some guidance on how to approach it. 🙇

DIsecting the change, you might actually be right. Instead of collecting ID's and then getting the entities we just reference the entity and create the list right away, doing away with the load multiple.

Technically its the same, there is also no risk involved with not load multiple imo since we know we have the entiy already. I cannot see how there could ever be a way to have different entities loaded with loadMultiple than we already know of in the (validated for existing) entity property.

This does assume the entity that is loaded is the correct revision, but that seems like ano brainer.

I want to RTBC, but:

1. IS needs update for new approach.
2. Need a change record describing the change

I've updated the proposed resolution. Assigning the issue to myself to draft a change record.

I've drafted a change record now. I'm no technical writer and haven't authored one before but hope it helps some what to drive this issue forward. 🙂

Thanks! I've updated the CR a little.

I prefer to test this, ive put some time into this to see if i could test it with a kernel test. Seems i can use the resolver properly, just need to do the second level of node and see if i can reproduce this. I keep getting an itch this needs testing.

Very rough work in progress.

class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {

  use ImageFieldCreationTrait;
  use ContentModerationTestTrait;
  use UserCreationTrait;

  public function testRelationshipVersionIncludes(){

    $this->assertTrue($this->container->get('module_installer')->install(['content_moderation'], TRUE), 'Installed modules.');


    $workflow = $this->createEditorialWorkflow();
    $this->addEntityTypeAndBundleToWorkflow($workflow, 'node', 'article');
    $this->type->getEntityType()->setHandlerClass('moderation', ModerationHandler::class);


    Role::create([
      'id' => RoleInterface::AUTHENTICATED_ID,
      'permissions' => [
        'access content',
        'view any unpublished content',
        'use editorial transition publish',
      ],
      'label' => 'Anonymous',
    ])->save();

    $this->setCurrentUser($this->user);

    $this->includeResolver = $this->container->get('jsonapi.include_resolver');
    $this->resourceTypeRepository = $this->container->get('jsonapi.resource_type.repository');

    $this->createEntityReferenceField(
      'node',
      'article',
      'field_entity_reference',
      'Related entity',
      'node',
      'default',
      ['target_bundles' => []],
      1
    );

    $node = Node::create([
      'title' => 'original node',
      'type' => 'article',
      'uid' => $this->user,
      'body' => [
        'format' => 'plain_text',
        'value' => $this->randomString(),
      ],
      'field_tags' => [
        ['target_id' => $this->term1->id()],
        ['target_id' => $this->term2->id()],
      ],
      'field_image' => [
        [
          'target_id' => $this->file->id(),
          'alt' => 'test alt',
          'title' => 'test title',
          'width' => 10,
          'height' => 11,
        ],
      ],
    ]);

    $nodeWithReference = Node::create([
      'title' => 'original node with reference',
      'type' => 'article',
      'uid' => $this->user,
      'body' => [
        'format' => 'plain_text',
        'value' => $this->randomString(),
      ],
      'field_tags' => [
        ['target_id' => $this->term1->id()],
        ['target_id' => $this->term2->id()],
      ],
      'field_image' => [
        [
          'target_id' => $this->file->id(),
          'alt' => 'test alt',
          'title' => 'test title',
          'width' => 10,
          'height' => 11,
        ],
      ],
      'field_entity_reference' => [
        ['target_id' => $node->id(), 'entity' => $node],
      ],
    ]);
    $nodeWithReference->save();

    $node->set('title', 'updated node');
    $node->setNewRevision(TRUE);
    $node->set('moderation_state', 'draft');
    $node->save();

    $nodeWithReference->set('title', 'updated node with reference');
    $nodeWithReference->setNewRevision();
    $nodeWithReference->set('moderation_state', 'draft');
    $nodeWithReference->save();

    $parentVid = $nodeWithReference->get('vid')->value;
    $childVid = $node->get('vid')->value;

    $resource_type = $this->container->get('jsonapi.resource_type.repository')->get('node', 'article');
    $resource_object = ResourceObject::createFromEntity($resource_type, $nodeWithReference);
    $includes = $this->includeResolver->resolve($resource_object, 'field_entity_reference');

    $jsonapi_doc_object = $this
      ->getNormalizer()
      ->normalize(
        new JsonApiDocumentTopLevel(new ResourceObjectData([$resource_object], 1), $includes, new LinkCollection([])),
        'api_json',
        [
          'resource_type' => $resource_type,
          'account' => NULL,
          'include' => [
            'field_entity_reference',
          ],
        ]
      );
    $normalized = $jsonapi_doc_object->getNormalization();



    // Todo:

    // Node 1  (static) -> reference node 2 -> reference node 3
    // Update node 3 to have a working copy.
    // Ask for working copy. Might need

  }
}

I worked on this during DrupalCon Singapore. I started work on a having mock entity reference revisions field type since entity reference revisions module is in contrib in git merge request. My test is just a mock placeholder. I am finished for today.