Field storage migration fails on re-import

Needs tests, but first I just want to get some feedback.

should a new approach be taken? Seemed to cause some kernel and functional test failures.

should a new approach be taken? Seemed to cause some kernel and functional test failures.

I see failures where it tries to serialize the container.
This is probably obfuscating something else.

This is probably obfuscating something else.

Actually.. my code does not check whether $old_destination_id_values is empty!
So I should be able to fix it.

I just found that these arrays are sometimes like [NULL, NULL].

This needs tests. But a review is not wrong.

Will post in #migration channel

I stopped by to say I have this issue with other types of migrations (view modes, field instances) -- looks like y'all already accounted for that, though! -- but, issue could use a title and summary update, I think? (but I'm too anxious to add that tag myself :) )

ANYWAY. Thank you for the fix!! -- it applies cleanly and does the trick on Drupal core 10.2.0, btw (I haven't tried on 11.x).

Dang. I had a whole thing typed up here, but I must not have hit submit. Here goes trying to remember what I was going to post...

This is a bug, NW for tests, but I'm not certain if this is the right approach here.

So the core EntityConfigBase expects the entity id key (generally id for configs) to contain the whole id. for a field storage entity, that would look like "node.field_text' we have 12 plugins that extend EntityConfigBase for various reasons. The three addressed in the MR are the only ones affected by this bug, as they override the getIds() method to change the destination plugin's ids and map table storage such that the individual parts of the config id are separate properties in the migration (i.e entity_type: node, field_name: field_text)

And as mentioned above, and fixed in this patch is the issue that the base getEntity method (from \Drupal\migrate\Plugin\migrate\destination\Entity) can't handle that, it tries to load an entity using only the first element of the ids array.

So there are two interesting things to note here. The first is EntityBaseFieldOverride which needs to do a similar thing but does it in a different way. Instead of overriding getIds, it overrides getEntityId instead. I got a bit into the weeds with trying to understand it, but it seems like this works for base_field_override, and might have worked for field instances because of the FieldConfigBase id() method, but it won't work in general.

The main thing that give me pause is this bit of EntityConfigBase's import method:

    if (count($ids) > 1) {
      // Ids is keyed by the key name so grab the keys.
      $id_keys = array_keys($ids);
      if (!$row->getDestinationProperty($id_key)) {
        // Set the ID into the destination in for form "val1.val2.val3".
        $row->setDestinationProperty($id_key, $this->generateId($row, $id_keys));
      }
    }

So, EntityConfigBase is smart enough to know that if a subclass has overridden the getIds() method, then it needs to implode the non langcode ids with a '.' separator and load it into the 'id' property so the config can be created with the right id, but it doesn't do the same with the old_destination_id_values before calling getEntity(). So my gut says we should fix this in EntityConfigBase, either by manuipulating old_destination_id_values before calling getEntity(), or more likely by overriding getEntity and putting the logic there (That way subclasses could still override getEntity with the original old_destination_id_values if they wanted to.

NW to refactor this into EntityConfigBase for any subclass that overrides getIds() and for tests.

I know this is awaiting a refactoring but meanwhile I had to move ahead with a migration and made a patch from the current merge request. To say others step I have attached it here.