Calling EntityConfig::import() with multiple destination IDs fails

The ability to rerun and update migrations using the EntityConfigBase destination is not something currently supported by drupal core, but I think it would be useful to add in this ability if we can do so. We would need to add a good number of tests to make sure that this works, so I'm setting to NW for tests. Additionally, the getEntity method in the patch appears to just be a copy of the parent getEntity() method, which is incorrect for reasons I've noted below. I like the idea, but we need to prove that it works.

Wow, total fail to copy my notes on that last comment.. My apologies.

What I meant to say was that it appeared the parent method was copied with just the line changed that you needed changed. My point was that if you are going to have a config specific getEntity method, then the rest of the method should be written specifically for config entities. Specifically these notes (which I really thought I posted above)

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -162,6 +162,32 @@ class EntityConfigBase extends Entity {
   +      // Attempt to ensure we always have a bundle.
   +      if ($bundle = $this->getBundle($row)) {
   +        $row->setDestinationProperty($this->getKey('bundle'), $bundle);
   +      }
   

   Config entities don't have bundles. This is unnecessary

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -162,6 +162,32 @@ class EntityConfigBase extends Entity {
   +      // Stubs might need some required fields filled in.
   +      if ($row->isStub()) {
   +        $this->processStubRow($row);
   +      }
   

   Config entities can't be stubbed. This is unnecessary.

3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -162,6 +162,32 @@ class EntityConfigBase extends Entity {
   +      $entity->enforceIsNew();
   

   I don't think we need 'enforceIsNew' with config entities. enforceIsNew lets us set a id on a content entity and save it without the system thinking it's an update, config entities are defined by their id, so this shouldn't be necessary.

rerolled for 9.x. Also applied suggestions made in #6.

Regarding #9: I have rerolled the patch for 9.2.x and addressed the suggestion in #6. You are right in pointing that Test cases are still to be done. Will try to take it up if time allows me.

Imho we only have to override the inherited getEntityId() method, basically repeating what's already done for the initial migration (when the original destination IDs array is empty on import).

This actually breaks change tracking of migrations with config entity destination.

The ability to rerun and update migrations using the EntityConfigBase destination is not something currently supported by drupal core,

That's true. Although, why is core testing track_changes? I guess because it should be supported.

At least this suggests that: https://api.drupal.org/api/drupal/core%21modules%21migrate%21src%21Plugi...

Added a test, but it isn't explicitly testing \Drupal\migrate\Plugin\migrate\destination\EntityContentBase, Drupal\migrate\Plugin\migrate\destination\EntityFieldStorageConfig or Drupal\migrate\Plugin\migrate\destination\EntityFieldInstance: Its a functional testing of track_changes for field storage and field instance migrations.

I hope that it works for all of us, or at least it's a good starting point for a further improvement.

Cancelled the Mysql tests (let's first test with SQLite, it's much faster)!

Adding a fix only patch.

The fix in #19 is not BC-safe – but it reminds me of #3200936: DX: Block (config) destination shouldn't recalculate block config ID for block translations .

I was able to clean up the language override import as well (detecting of a translation was broken – the condition was wrong).

But the major update is that I added a test for EntityContentBase, which shows why this issue should be classified as bug.

Explaining the fix:

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -153,7 +153,7 @@ public function fields() {
   -    $entity_id = reset($old_destination_id_values) ?: $this->getEntityId($row);
   +    $entity_id = $this->getEntityIdFromRowOrDestination($row, $old_destination_id_values);
   

   This was the root of the issue: If there are more than one destination IDs, every subclass tried to load the entity with the very first one. Apparently was fine in 80% of the situations, but newer worked in case of field_config and field_config_storage destinations.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -174,6 +174,21 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +  /**
   +   * Gets the entity ID of the row.
   +   *
   +   * @param \Drupal\migrate\Row $row
   +   *   The row of data.
   +   * @param array $old_destination_id_values
   +   *   The previous destination ID values, if any.
   +   *
   +   * @return string
   +   *   The entity ID for the row that we are importing.
   +   */
   +  protected function getEntityIdFromRowOrDestination(Row $row, array $old_destination_id_values) {
   +    return reset($old_destination_id_values) ?: $this->getEntityId($row);
   +  }
   +
   

   ...so I moved the original $entity_id value assignment into this new protected method.

3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -282,9 +295,38 @@ public function rollback(array $destination_identifier) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function getEntityIdFromRowOrDestination(Row $row, array $old_destination_id_values) {
   +    return $old_destination_id_values
   +      ? static::joinIds($old_destination_id_values)
   +      : $this->getEntityId($row);
   +  }
   

   ...and I override it in EntityConfigBase.

4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -282,9 +295,38 @@ public function rollback(array $destination_identifier) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function getEntityId(Row $row) {
   +    $destination_id_key = $this->getKey('id');
   +    $destination_id = $row->hasDestinationProperty($destination_id_key)
   +      ? $row->getDestinationProperty($this->getKey('id'))
   +      : NULL;
   +
   +    if (!empty($destination_id)) {
   +      return $destination_id;
   +    }
   +
   +    $ids = $this->getIds();
   +    // Ids is keyed by the key name so grab the keys.
   +    $id_keys = array_keys($ids);
   +    // Set the ID into the destination in for form "val1.val2.val3".
   +    return $this->generateId($row, $id_keys);
   +  }
   

   I also "fix" the getEntityId() method – this ensures that we will have the right destination entity ID even when we have a preexisting configuration entity. Why this should be possible? Let me give an example!

   If you have a source Drupal 7 site with e.g. a field_image field with cardinality -1 (unlimited), then you are not able to migrate the data into your destination site: Standard profile provides its own field_image storage, but with cardinality set to 1. The most inconvenient part of the problem is that you only notice that something went wrong when you already migrated your data as well (if-you-ever-notice-it).

   If we ensure that every config entity destination is able to find a preexisting entity (please keep in mind that this is the case for node types, vocabularies, etc), then we can also prevent these cases!

5. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -195,18 +195,18 @@ protected function updateEntity(EntityInterface $entity, Row $row) {
        // This is a translation if the language in the active config does not
        // match the language of this row.
        $translation = FALSE;
   -    if ($row->hasDestinationProperty('langcode') && $this->languageManager instanceof ConfigurableLanguageManager) {
   -      $config = $entity->getConfigDependencyName();
   -      $langcode = $this->configFactory->get('langcode');
   -      if ($langcode != $row->getDestinationProperty('langcode')) {
   -        $translation = TRUE;
   -      }
   +    if (
   +      $row->hasDestinationProperty('langcode') &&
   +      $this->languageManager instanceof ConfigurableLanguageManager &&
   +      $row->getDestinationProperty('langcode') !== $entity->language()->getId()
   +    ) {
   +      $translation = TRUE;
        }
   

   I wasn't able to find out what should be done here, but the comment above helped A LOT.

   Notice the $langcode = $this->configFactory->get('langcode'); line! This was actually loading an immutable config. Do you know which config is expected here? I do not, I think core never had a configuration entity with langcode ID.

   But anyway, comparing this object never matches the value of the langcode destination property – which means that without fixing this logic, we are not able to fix this bug.

   So I went ahead and changed this condition. From now on, it will work what its comment describes.

The reason why you cannot see the first test's failure message in #23 is #3197324: Exception trace cannot be serialized because of closure.

But!
The second tests failure message clearly reflects the bug:

Expectation failed for method name is "load" when invoked zero or more times
Parameter 0 for invocation Drupal\Core\Entity\EntityStorageInterface::load('id1') does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'id1.id2'
+'id1'

I think I can remove the needs tests tag.

Patches in #23 can be applied on 9.2.x and on 9.1.x as well.

Re #30:

I'm able to get a meaningful message even without changing anything out of the kernel test: https://git.drupalcode.org/project/media_migration/-/blob/8.x-1.x/tests/...

But these are out of scope (and core won't solve that issue class by class imho).

(And I already mentioned it in #28)

#15: wow, I literally have nothing to remark about your test-only patch!

A few questions and nits on the complete patch:

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -179,12 +179,14 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   -  protected function getEntityId(Row $row) {
   -    return $row->getDestinationProperty($this->getKey('id'));
   +  protected function getEntityId(Row $row, array $old_destination_id_values) {
   +    return reset($old_destination_id_values) ?: $row->getDestinationProperty($this->getKey('id'));
   

   This is technically a BC break … 🙈
   Already fixed in #19 👍🥳

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -196,15 +197,19 @@ protected function updateEntity(EntityInterface $entity, Row $row) {
   -      $langcode = $this->configFactory->get('langcode');
   -      if ($langcode != $row->getDestinationProperty('langcode')) {
   ...
   +      $default_langcode = $this->languageManager->getDefaultLanguage()->getId();
   +      $langcodes_representing_default_translation = [
   +        $default_langcode,
   +        LanguageInterface::LANGCODE_NOT_SPECIFIED,
   +      ];
   +      if (!in_array($row->getDestinationProperty('langcode'), $langcodes_representing_default_translation, TRUE)) {
   

   This improves the config translation handling I think? 🤔

   I think to be able to change this we'll need explicit test coverage for this too. And arguably it is out of scope here? 🤔

   I can easily be convinced that this is necessary though, because core doesn't have a langcode config nor does there seem to be anything else trying to get it…

   Confirmed by #27.5 👏👏👏

3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -282,9 +300,33 @@ public function rollback(array $destination_identifier) {
   +    // Set the ID into the destination in for form "val1.val2.val3".
   

   Nit: s/in for form/in the form/

#19: nice — updating my review of #15 now :P

#23: This test also looks superb. Only a nit:

+++ b/core/modules/migrate/tests/src/Unit/destination/EntityConfigBaseTest.php
@@ -0,0 +1,211 @@
+    // followings are good to go.

🤔 Nit: not sure what "followings" means?

@huzooka I don't think I've ever seen a core patch of this test complexity be done so well on the first iteration. Impeccable work. Thank you for your excellent work!

I'd RTBC this, but I think this kind of change needs migration system maintainer review.

@codebymikey, no problem :)

Asking for subsystem maintainer review according to #32.

#34: d'oh — I forgot about that tag!

Time to start looking at this.

I started with the title. Can the title be changed to state what this is doing 'Allow EntityConfig migrations to track changes' or something. (It is rather harsh to claim this is broken when, at the time and as far as I recall, the migration of configuration was considered a once off).

Then I read the IS. The steps to reproduce says to install Migrate Tools, which is true. But it is also true that using Migrate Run or Drush 10.4+ will also allow one to reproduce the problem.

I then went to review the code.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -174,6 +174,21 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +   * Gets the entity ID of the row.
   

   This is the same summary as an existing method in Entity. I suggest adding an explanation of when and why to use or the other.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -195,18 +195,18 @@ protected function updateEntity(EntityInterface $entity, Row $row) {
   +    if (
   +      $row->hasDestinationProperty('langcode') &&
   +      $this->languageManager instanceof ConfigurableLanguageManager &&
   +      $row->getDestinationProperty('langcode') !== $entity->language()->getId()
   +    ) {
   

   The reformatting here makes it difficult to find the change. Plus the operators are at the end of the line when, typically, they are at the beginning of the line as has been done for the ternary operations elsewhere in the patch. The change is to use $entity->language()->getId() instead of $this->configFactory->get('langcode'). Why is this change needed?

3. +      $language_override = $this->languageManager->getLanguageConfigOverride($row->getDestinationProperty('langcode'), $entity->getConfigDependencyName());
   +      $language_override->set(str_replace(Row::PROPERTY_SEPARATOR, '.', $row->getDestinationProperty('property')), $row->getDestinationProperty('translation'));
   +      $language_override->save();
   

   If the name is not changed then there is only one line changed here and makes this is easier to review.

4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -257,6 +257,19 @@ protected function generateId(Row $row, array $ids) {
   +   * Generates an entity ID.
   

   The summary and the return description are identical to the summary for method generateId. I think at least the summary heres needs to be expanded to explain the difference.

5. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -257,6 +257,19 @@ protected function generateId(Row $row, array $ids) {
   +  protected static function joinIds(array $id_values) {
   

   Not a fan of the name, JoinIds. Yes, it is descriptive but makes me think of SQL joins. I couldn't think of a better name so browsed the config system and I saw buildCacheId. How about buildId?

6. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -282,9 +295,38 @@ public function rollback(array $destination_identifier) {
   +    // Ids is keyed by the key name so grab the keys.
   

   s/Ids is/IDs are/

I took a brief look at the tests and I did not see any testing for translated sources, which I looked for because of the changes to the if blocks at the beginning of \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::updateEntity. Did I miss something?

I hope to continue during this week.

Per #32 and #36

1. Changed the issue title as suggested
2. Updated the IS as suggested
3. Fixed the code as follow
   1. Did all asked naming and documentation changes
   2. Reverted the change about the langcode as it was not causing the issue in my case (I hope the testbot will agree). If I'm right, it might be moved in a follow-up.

My bad, those changes that I reverted were useful. Now migrate tells me that it updated the entities but it didn't...
I brought them back and made the minore changes asked in #36.

I did not expand tests to cover the translated source case (and I don't plan to, given my limited skills).

Since you bumped this issue back on my dashboard, there is a better chance of me addrssing it :)

I looked at the tests now. They look fine and appear to test what is needed (but it is late in the day for me). Just a few suggestions.

1. +++ b/core/modules/migrate/tests/src/Kernel/TrackFieldConfigChangesTest.php
   @@ -0,0 +1,402 @@
   +    'system',
   

   We normally alphabetize the $modules list.

2. +++ b/core/modules/migrate/tests/src/Kernel/TrackFieldConfigChangesTest.php
   @@ -0,0 +1,402 @@
   +    // Verify that when there are no changes on the source, nothing gets
   

   Expand the comment to make it clear that the destination is being changed.
   "Verify the when the destination configuration changes but the source does not change, that nothing is migrated."

3. +++ b/core/modules/migrate/tests/src/Kernel/TrackFieldConfigChangesTest.php
   @@ -0,0 +1,402 @@
   +    // Rerun migrations.
   

   Add a blank line here to separate the rerun from the initial migration.

4. +++ b/core/modules/migrate/tests/src/Kernel/TrackFieldConfigChangesTest.php
   @@ -0,0 +1,402 @@
   +   * Executes the given migrations with track_changes set to TRUE.
   

   s/migrations/migration/
   Because this only does one migration.

5. +++ b/core/modules/migrate/tests/src/Unit/destination/EntityConfigBaseTest.php
   @@ -0,0 +1,210 @@
   +use Drupal\Core\Config\Entity\ConfigEntityBase;
   

   We normally sort the list of use statements.

6. +++ b/core/modules/migrate/tests/src/Unit/destination/EntityConfigBaseTest.php
   @@ -0,0 +1,210 @@
   +        return $this->getMockForAbstractClass(ConfigEntityBase::class, [$values, self::ENTITY_TYPE_ID]);
   

   This will be easier to read if the array spans lines.

7. +++ b/core/modules/migrate/tests/src/Unit/destination/EntityConfigBaseTest.php
   @@ -0,0 +1,210 @@
   +    // Ensure that the preexisting config entity  has the expected initial
   +    // property value. (In this test, the source and destination IDs are the
   +    // same!)
   

   s/entity has/entity has/. The last sentence is difficult. I understand parentheses are used for extra information that may be useful but then the exclamation mark implies that this is import. All together it is confusing. So, remove the parentheses around the last sentence and change the exclamation mark to a period.

8. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -133,7 +133,7 @@ public function import(Row $row, array $old_destination_id_values = []) {
   +        // Set the ID into the destination in the form "val1.val2.val3".
   

   Out of scope.

Updated the patch in #38 to address the changes suggested in #41, thanks!

Track changes is a completely different thing in migrate world. Additionally, this isn't a bug. The config destination is currently specifically documented to not support updates through the migration system. Adding that functionality is a feature request, not a bug.

I have used the patch from #42 on Drupal 9.2.2 to update my migration 'upgrade_d7_field' and it processed right away instead of throwing lots of ''field_storage_config' entity with ID 'xyz' already exists errors. Although the specific update i was trying, didn't work for other reasons, the migration update worked!

Thanks!

@mikelutz,

What if I keep only EntityConfigBaseTest (so remove the extra change tracking test)? I strongly believe that this issue is about a bug (it is obvious for me).

But fixing the bug of EntityConfigBase has a side effect: change tracking will be possible for migrations using destination plugins with multiple destination IDs (which are extending EntityConfigBase).

Removed the track change test, and also an unneeded comment.

Seems like quite a few people/migrations would benefit from this. Lots of collaboration on this issue. Bumping for another round of review 🤞 (I can't RTBC this I think.)

Took a look again and found a few things.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -282,9 +293,38 @@ public function rollback(array $destination_identifier) {
   +    $destination_id_key = $this->getKey('id');
   +    $destination_id = $row->hasDestinationProperty($destination_id_key)
   +      ? $row->getDestinationProperty($this->getKey('id'))
   +      : NULL;
   

   Why not just $row->getDestinationProperty($this->getKey('id'))? It will return NULL if the property does not exist.

   If I am incorrect on that point then if $row->getDestinationProperty($this->getKey('id')) is changed to use use $destination_id_key it will be easier to read.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -282,9 +293,38 @@ public function rollback(array $destination_identifier) {
   +    $ids = $this->getIds();
   

   When is this block of code starting here code executed? The new test does not reach this.

3. 
   +++ b/core/modules/migrate/tests/src/Unit/destination/EntityConfigBaseTest.php
   @@ -0,0 +1,213 @@
   +   * Tests the initial and subsequent import of a config entity.
   ...
   +    // Test a subsequent import.
   

s/subsequent/update/
Because subsequent does not convey that what is being done is an update, not just running say 'drush mim' again.

I'd probably change the variables to use update instead of subsequent as well.

From #27,

If you have a source Drupal 7 site with e.g. a field_image field with cardinality -1 (unlimited), then you are not able to migrate the data into your destination site: Standard profile provides its own field_image storage, but with cardinality set to 1. The most inconvenient part of the problem is that you only notice that something went wrong when you already migrated your data as well (if-you-ever-notice-it).

The first thing that I notice here is that this in migrating to a Drupal destination site with the standard profile but migrations are to be run on a minimal install. However, I did try this on HEAD. I made a D7 site, standard install and change the image field of the article to be unlimited. Then I made a node that had three unique images. I then made a D9 sites, standard install, enabled migrate_drupal_ui and ran the migrations. The image field was correctly changed to unlimited and 3 images were correctly migrated to the node.

And finally what about a testing of a rollback?

Patch rerolled for 9.3.x

I ran into this issue without trying to use any contribs:

#3260352: Executing d7_entity_translation_settings and d7_language_content_comment_settings migrations in the calculated optimal order results in migration error

Rerolled patches in a new MR
Addressed some comments from #49 (points 1 and 3)

Hiding files now there is a MR

Just rerolled on latest 11.x.
This still needs point 2 from #49 to be fixed.

Patches for composer users <3