Fix \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::updateEntity() so that config translation migrations can be rolled back

Let's investigate where this is happening.

Here's a potential fix. I'm not sure we should be create config overrides if we don't have a property or translation to set.

@daffie that exception is now unreachable - I left it in #5to prove the change fixes the issue. Here's a patch without the exception.

I have suspicions that this code is still not correct though. It's pulling random info from the destination and doing stuff with it. Can we be sure that translation and property always mean what we want them to mean? This code was introduced by #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields

We don;'t need a comment in the code about $row->hasDestinationProperty('property') && $row->hasDestinationProperty('translation') - we're adding them because we depend on them in the block below.

I think we should try to fix this in a better way here.

Okay this is a bit of a mess. The following code in HEAD \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::updateEntity()

    // 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;
      }
    }

IS working out if we are creating a translation... and if it thinks we are - ie. the config entity langcode does not match the config langcode it is going to create a config language override by doing:

    if ($translation) {
      $config_override = $this->languageManager->getLanguageConfigOverride($row->getDestinationProperty('langcode'), $config);
      $config_override->set(str_replace(Row::PROPERTY_SEPARATOR, '.', $row->getDestinationProperty('property')), $row->getDestinationProperty('translation'));
      $config_override->save();
    }

This relies on the destination having a 'property' and a 'translation' property.

HOWEVER... elsewhere in the code for example in the caller of EntityConfigBase::updateEntity() we do:

    $entity = $this->getEntity($row, $old_destination_id_values);
    // Translations are already saved in updateEntity by configuration override.
    if (!$this->isTranslationDestination()) {
      $entity->save();
    }

So we should be calling $this->isTranslationDestination() in \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::updateEntity(). But this is going to break things because not all migrations for config translations have this set properly. Compare core/modules/config_translation/migrations/d6_field_instance_label_description_translation.yml (has it) with core/modules/config_translation/migrations/d7_field_instance_label_description_translation.yml (does not).

This is a bug issue because if translations: true is not set correctly we'll be doing unnecessary entity saves and \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::rollback() will not work as expected.

The patch attached will show us how broken all of this is.

Here are some more migrations that are not configured correctly and can't be rolled back.

I think we need to call $this->setRollbackAction($row->getIdMap(), $rollback_action); inside the if ($translation) { bit in \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::updateEntity()

Hmmm that does not fix the problem. But if we don't do this then how can we be sure that the config translation is deleted on rollback...

This is looking pretty good now I think. We have way better test coverage we've fixed a bug in rollbacks and tested it. Nice work everyone!

+++ b/core/modules/config_translation/migrations/d6_taxonomy_vocabulary_translation.yml
@@ -25,6 +25,7 @@ process:
 destination:
   plugin: entity:taxonomy_vocabulary
   destination_module: config_translation
+  translations: true
 migration_dependencies:

Do we need a CR for this? It's kinda true atm (rollback does not work on config translation migrations without this) but may have been missed if people copied and pasted from many of the core examples.

@quietone thanks for the CR.

I'm not sure about

Before this change the property was not set and rolling back the migrations caused an Undefined array key warning.

I think that before this change \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::rollback() would not rollback config translations if translations: true was not set because \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::isTranslationDestination() would return FALSE if the migration was missing it.

@quietone thanks for confirming. I've tweaked the CR to contain the important information for anyone who has custom configuration translation migrations and to be as clears as possible what needs to be done - while not getting into the weeds. If people want to know the details as to why they can read the discussions on this issue.

I think this issue is ready now :)

After discussing this with @catch I've merged the tests from #3227660: MigrateExecutable::rollback incorrectly assumes MigrateMapInterface::getRowByDestination() returns an array with 'rollback_action' key and slightly improve on both issues logic of checking the result of ... going for:

        $map_row = $id_map->getRowByDestination($destination_key);
        if (isset($map_row['rollback_action']) && $map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {

isset($map_row) will not error for $map_row = FALSE.

Crediting the people who worked on that issue and will mark that one as a duplicate.

@huzooka the code

if (isset($map_row['rollback_action']) && $map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {

checks for an array key - it also happens to be a bit more robust if $map_row happens to be false. There's no need to use array_key_exists() here because if $map_row['rollback_action'] is equal to NULL then.... oh well... const ROLLBACK_DELETE = 0; and $map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE. That sucks... $map_row['rollback_action'] = NULL will return TRUE atm. We need to stop using == everywhere. See https://3v4l.org/628ck

Personally I think we should not care about $map_row['rollback_action'] = NULL. The looseness in this code atm is a bug creator.

@huzooka it's not forced by the interface only by documentation - we have far too many places in core where documentation != reality. If it was public function getRowByDestination(array $destination_id_values): array; then sure but something may have extended \Drupal\migrate\Plugin\migrate\id_map\Sql in custom and is still returning FALSE, NULL or whatever.

These bugs are intimately linked. We can't fix config translation rollback without fixing the bug in \Drupal\migrate\MigrateExecutable::rollback() as well. And I think not being able to rollback a migration is a way more significant bug than a PHP notice.

Updating the issue summary so we're clear about what is being fixed here.

@longwave I agree. Let's make it way more explicit and actually test this behaviour properly.

Better prophecy.

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -313,7 +313,7 @@ public function rollback() {
-        if ($map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {
+        if (!isset($map_row['rollback_action']) || $map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {

This is the new version of the contentious if. It will not emit an error if $map_row is an array without a rollback_action or $map_row is FALSE... and it behaves the same way - i.e. if if can't determine a rollback_action it assumes the ->rollback() should be called.

We could add a comment like:

        // The default behaviour if $map_row['rollback_action'] is not set or
        // NULL is to assume that $destination->rollback() should be called.

But given this is now !isset($map_row['rollback_action']) || I think it is explicit and obvious enough. Plus we have tests.

Here's evidence that the new formulation works the same way as the old - https://3v4l.org/hGIMP - with we less PHP notices... see https://3v4l.org/Lf6NF for only the new version.

If $map_row is FALSE we will get a deprecation on PHP 8.1 because as pointed out a few times that's against the interface documentation so I think that is okay. It won't be broken and if you have custom code that triggers this you should fix it.