Allow single scalar value in EntityReferenceRevisions data type plugin

+++ b/src/Plugin/DataType/EntityReferenceRevisions.php
@@ -114,6 +114,10 @@ class EntityReferenceRevisions extends EntityReference {
+    elseif (is_scalar($value)) {

It is very common that revision id and entity id it not the same. I think it makes sense to set only the target_revision_id and let the the field compute the target_id by itself. The revision is unique anyways.

The properties are wrong, traget_id and target_revision_id is the proper name.

We cleaned up lots of duplicated code but yes, I guess the target property plugin is still remaining.

Note that storing the target_id does have some benefits. Core currently has no multi loading nor caching of any sort for loading revisions.

I noticed that field_collection attempts to optimize that by defaulting to load the current revision first and only fall back to the specific revision if it is in fact not that. Of course, it would be even better to support those two things in core ;)

RTBC can not be accepted without any statement. Also i see open items in discussions above.

- Reformatted the method use a switch statement instead of a string of elseifs.
- Added handling for missing target_id or target_revision_id when we receive an array
- When both target_id and target_revision_id are specified, validate that they both refer to the same entity before setting the target_id property.

@todo:
- Validating sane values for entity ID and revision ID should really happen in a constraint.
- Inject the entity type manager and logger
- Tests

sometimes integers are mis-identified as strings. Updated patch to handle it.

whoops, used the $entity to work out the entity type, when entity failed to load.

Trying to raise prio here once to try creating some focus.
We are preparing the next release and i would love to get this in.

As this adds specific code for new cases, we need tests for those.

+++ b/src/Plugin/DataType/EntityReferenceRevisions.php
@@ -93,31 +94,111 @@ class EntityReferenceRevisions extends EntityReference {
+        if (isset($value['target_id']) && !empty($value['target_id'])) {

This assumes the keyed value passed in here is target_id or target_revision_id. But if you are doing a migration, as is mentioned in the OP, the value provided back from a paragraphs migration is actually id. This is because the "key" according to paragraph's entity definition is 'id'.

I'd almost prefer using nixing this approach entirely and add a dedicated paragraphs destination class that returns a properly indexed set of values that are usable with the migration process plugin.

#2644634-11: Missing Migration Destination in 8.* has some documentation on how to interact with this thing via a migration. I'll leave it up to you guys to decide if there are use cases (outside of migrations) that require the need to create an ERR with a scalar value. But if there aren't such use cases, I'd recommend marking this "won't fix" and point to the docs on how to migrate paragraphs.

I'm 400% in favor of a migration destination as the solution to the migration part of the problem... I actually wrote a destination plugin myself, first. BUT

1) Why make a destination specific for Paragraphs? Every line of that destination plugin solves for the general ERR migration use case. The ERR module page says we have aspirations outside of Paragraphs, like inline entity form and field collections. If that's the case, ERR should be concerned about migration in the general case, and should take over that patch.

2) If there is a legitimate need for ERR to accept a simple, rather than compound, value - and I would argue that there is - then the simple value fix also solves a lot of ERR migration cases, without writing a destination class.

Further to point 2: anything written for pre-ERR versions of modules like Paragraphs expects to be able to pass an entity ID. The ability to accept a simple value is therefore a significant improvement in backwards (and cross-) compatibility. I Am Not A Maintainer for this module, but I strongly suggest this is a use case worth considering.

This needs review (and a decision) from a maintainer. Marking as such.

Linking to paragraph's migrate issue.

1. +++ b/src/Plugin/DataType/EntityReferenceRevisions.php
   @@ -93,31 +94,111 @@ class EntityReferenceRevisions extends EntityReference {
   +      case 'object':
   ...
   +      break;
   

   Indentation.

2. +++ b/src/Plugin/DataType/EntityReferenceRevisions.php
   @@ -93,31 +94,111 @@ class EntityReferenceRevisions extends EntityReference {
   +          $entity = $entity_storage->load($value['target_id']);
   ...
   +            if (!$entity) {
   +              throw new \InvalidArgumentException('Value is not a valid entity ID.');
   ...
   +            if (!$entity) {
   ...
   +              \Drupal::logger('entity')
   +                ->error('%type %id is invalid. Ignoring the entered entity ID.', array(
   ...
   +              \Drupal::logger('entity')
   +                ->error('%type %id does not contain revision %vid! Ignoring the entered entity ID.', array(
   

   I would stop earlier if $entity load fails or if it doesn't match. And fail hard instead of just logging an error.

3. +++ b/src/Plugin/DataType/EntityReferenceRevisions.php
   @@ -93,31 +94,111 @@ class EntityReferenceRevisions extends EntityReference {
   +      case 'integer':
   +        $this->revision_id = (int) $value;
   

   After so much spaghetti of loading, here you just consider this a revision_id and skip setting the entity id...
   You don't even try to load it and check if it exist.

I think if you really want to support both cases, we should introduce a helper
protected function loadEntityBy($id, $revision_id)
And it should allow both $id and $revision_id be empty. Internally you do housekeeping (loading, check if entity id matches id, fail hard if something doesn't match) and at the end assign both consistently.

This leads me back to tests. Before, loading was streamlined and simple. Now you add many conditions and thus special cases.
If we want to support those, we want to add tests that trigger failing conditions and check them.

No patch here. :-)

Thinking about the Scalar case:
If we want to support being a similar target like EntityReference is, we should treat a Scalar similarly. In that case it would be the Entity ID and we would resolve to the default revision.

@@ -93,31 +94,111 @@ class EntityReferenceRevisions extends EntityReference {
We are subclassing EntityReference here, so we should not behave differently with a value passed in.

So how would we solve the migration target case if we stay strict?

In case of a UUID, we could try to support both cases... (if the revision has an own)

@miro_dietiker thanks for checking in, and good point about subclassing. We should assume the scalar value is entity ID, and lookup the default revision. Attached hctom's snippet as a patch. Yes, we should inject the service properly, but we don't do it elsewhere in this class either.

The migration problem is a separate issue. But if Migrate returns only one of a set of compound keys, that's a Migrate bug for sure. Scanning through the code it looks like it does a straight up DB query, which would return [sourceid1 => 123, sourceid2 => 456] . Let's look into this deeper in #2809793: EntityReference migrate destination.

TIL that typed data plugins can't receive services through dependency injection. #2053415: Allow typed data plugins to receive injected dependencies

Still, calling the global container inline is not nice. Patch re-rolled with entity storage moved into its own property.

Removing "needs tests" tag because we removed the complexity that required it in the first place. Rewriting the description for clarity.

Fixed broken constructor type hints in #27.

Update ticket status.

Nice, but we still need tests. :)

The patch above is failing for me in EntityReferenceRevisionsItem->onChange(), where it wants the revision and target id. It fails with null value around ~213.

#2843278: Fatal error on migrate-import was closed as a duplicate of this one. If the issues are really exactly the same, then a test for this issue can be found in comment #8 of the duplicate issue.

I'd be willing to bet the reason for errors here is one should use the solution adopted in #2809793: EntityReference migrate destination. I'd suggest this get closed as duplicate to it even, but I think that the "bug" folks are seeing is coming from upstream in something like Feeds. The real fix would be to pass in both pieces of info from Feeds, like migrate does.

@heddn From what I can tell it seems like migrations are also affected by this issue. Specifically I'm seeing the exact same behavior as described in #2903495: Migration won't work if we are migrating a translation. when attempting to migrate paragraph translations. That issue was closed as duplicate of this one, which is why I'm commenting here.

Rerolled #29 since the patch didn't apply due to changes introduced by #2673076: Investigate loading pattern of field_collection.

Patch #37 made it possible for me to migrate paragraph translations. Thanks!

Patch #37 is working fine, now I could migrate my paragraphs without problems

The default entity reference field supports scalar values so I guess that this is how this should also work.
In my case, I ran across the issue by running behat tests where the entity could not be passed as a value.

I can confirm that the patch resolves the issue and I am also attaching a test for this. I did a small refactor in one of the test classes too to make it more readable and I am attaching a test-only file as well.

Oops I removed a leftover comment.

Found this wile testing a form with IEF and ERR, patch in #42 solves the issue!

Error: Call to a member function getRevisionId() on null in Drupal\\entity_reference_revisions\\Plugin\\Field\\FieldType\\EntityReferenceRevisionsItem->onChange()

RTBC+. The last patch fixes a couple warnings/errors for me when trying to write a paragraph ID to a paragraph field in a custom migration:

[warning] Illegal string offset 'target_id' EntityReferenceRevisions.php:116
 [warning] Illegal string offset 'target_revision_id' EntityReferenceRevisions.php:116
 [warning] Illegal string offset 'target_id' EntityReferenceRevisions.php:120
 [warning] Illegal string offset 'target_revision_id' EntityReferenceRevisions.php:121
 [error]  Error: Call to a member function getRevisionId() on null in Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem->onChange() (line 216 of /[..]/web/modules/contrib/entity_reference_revisions/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php) #0 /[..]/web/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php(132): Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem->onChange('entity', false)

RTBC+ (since 2020 in my use case)

Posted a review.