Update documentation for handling dots in the static_map plugin

One workaround would be to first pipe the value through the machine_name plugin (but don't forget to rewrite the complete source value as the plugin will run the value over the transliteration service, lowercases it, replaces anything that's not a number or a letter with an underscore and removes duplicate underscores):

  field_cpu_bus_type:
    -
      plugin: machine_name
      source: BusType
    -
      plugin: static_map
      bypass: true
      map:
        'pcie_3_0': pcie_3_0
        PCI: pci
...

I added the workaround to the documentation.

It's important to note that the above workaround will not work if your static_map is using bypass and you need to preserve the dots. This is a problem when working with emails, since you will have no way to filter out some emails while preserving others.

For mapping (say) email addresses in a user migration, this did the trick:

namespace Drupal\whatever\Plugin\migrate\process;

use Drupal\migrate\MigrateExecutableInterface;
use Drupal\migrate\ProcessPluginBase;
use Drupal\migrate\Row;

/**
 * Perform custom value transformations.
 *
 * @MigrateProcessPlugin(
 *   id = "email_map"
 * )
 *
 * Map emails, because config keys can't contain a period.
 *
 * @see https://www.drupal.org/project/drupal/issues/2827897
 *
 * @code
 * mail:
 *   plugin: email_map
 *   source: mail
 * @endcode
 */
class EmailMap extends ProcessPluginBase {

  /**
   * Array of email values to map.
   *
   * @var string[]
   */
  protected $emails = [
    'from@example.com' => 'to@example.org',
  ];

  /**
   * {@inheritdoc}
   */
  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    return (isset($this->emails[$value])) ? $this->emails[$value] : $value;
  }

}

And in the yaml only

  mail:
    -
      plugin: email_map
      source: mail

Prefer to not do purpose-specific plugins, but it does the trick!

For any interested parties, I just added a new process plugin to Migrate Conditions that can be used to work around this static_map limitation pretty easily. The example is in the docs here.

It would look like this for the case in the IS.

field_cpu_bus_type:
  plugin: switch_on_condition
  source: BusType
  cases:
    -
      condition: equals(PCIe 3.0)
      default_value: pcie_3_0
    -
      condition: equals(PCI)
      default_value: pci
    -
      condition: default
      get: BusType

Slightly verbose, but not bad.

We discussed this at the 10/20/2022 migrate meeting and it looks like this is truly a bug in the migrate_plus module. The core migrate framework does not rely on config entities, migrate_plus does.

I'm attaching a couple patches here. The first adds a little test coverage to core to show that this bug does not exist in core. The second is a patch for migrate_plus that exposes the bug there. When I run it locally I get this error:

1) Drupal\Tests\migrate_plus\Kernel\MigrationConfigEntityTest::testImport
Drupal\Core\Config\ConfigValueException: foo.bar key contains a dot which is not supported.

/var/www/html/core/lib/Drupal/Core/Config/ConfigBase.php:211

Pretty damning!

Not sure if this issue should be moved to migrate_plus or if it should instead be closed, with a brand new issue on migrate_plus.

Sorry triggered tests, yes probably should move to the migrate_plus module

I was looking through the docblock on static_map today and noticed this hunk at the end:

 * Mapping from a string which contains a period is not supported. A custom
 * process plugin can be written to handle this kind of a transformation.
 * Another option which may be feasible in certain use cases is to first pass
 * the value through the machine_name process plugin.
 *
 * @see https://www.drupal.org/project/drupal/issues/2827897

I guess we should remove that since it's not true.

This report and that documentation were both probably true when they were written. Core migrate used to store migrations in config early in D8 development, but they were switched to plugins a long time ago.

@danflanagan8, thanks for the testing!

Let's use this issue to add the new test and update the documentation for StaticMap. The title needs an update too.

Cheers.

Keeping the test that shows this works, and changing the documentation in the plugin.

Thanks, @danflanagan8 and @mikelutz. The patch looks good.

Just to be sure that the test covers what we think it does, I hacked StaticMap and added

    if (is_string($value) && str_contains($value, '.')) {
      throw new MigrateException('Key contains a "." character.');
    }

As expected, the test failed with my new error message.

I am updating the title, as @quietone suggested in #21.

I will also add an issue for the migrate_plus module.

The patch didn't fail testing, The testing failed the patch. 😐

1. +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/external_translated_test_node.yml
   @@ -15,5 +15,6 @@ process:
   +      T.B.D.: un
   
   +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/external_translated_test_node_translation.yml
   @@ -19,6 +19,7 @@ process:
   +      T.B.D.: und
   

   I keep wondering why these have been changed. These test modules are not used for testing the static map plugin. I think these should be removed. Or have I missed something?

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
   @@ -133,10 +133,13 @@
   + * Note: While this plugin supports mapping from a string that contains a
   + * period, using this module with contrib modules which store migration
   + * configuration data as configuration entities will prevent this from working,
   + * as Drupal configuration entities may not contain keys which contain a period.
   + * Mapping from strings containing periods in conjunction with these contrib
   + * modules may require additional manipulation with custom or contrib process
   + * plugins.
   

   We have a pattern of adding documentation at the top of the plugin doc block, before all the examples so this should move to after the available keys section. See Substr.php.

   And since I've asked for a change we should also make this text easier to read. I had a go at using plain language as follows. This is only a suggestion.

    * This plugin supports mapping from a string that contains a dot. However, it
    * does now work when the migration is stored as a configuration entity. This is
    * because Drupal configuration entities may not contain keys which contain a
    * dot.
    * Some contributed modules convert migrations to configuration entities.
    * If you are using such a module you will need custom or contrib process
    * plugins to process the string.
   

Regarding @quietone's 27.1

I keep wondering why these have been changed

I added those changes to prove that a dot doesn't cause any problems when static map is used in yaml as opposed to simply called in a unit test. It looks like I edited those particular migrations because those are the test migrations that use static_map.

These test modules are not used for testing the static map plugin. I think these should be removed.

I agree with that. This is getting to be a pretty long time ago, but I don't think I ever intended for those changes to get committed. The main goal of my patches in #17 were to offer definitive proof that the problem is in migrate_plus rather than core. Using a dot in a migrate (yaml) plugin is fine, while using a dot in a migrate_plus migration config entity is not fine. I probably should have somehow indicated in the patch name that it was a "do-not-commit" patch or something. :)

I think we could revert those changes without replacement. I don't think we typically have tests for process plugin that would use a dedicated yaml, do we? I took a quick look right now and can't find any examples.

Convert to MR then made changes I suggested in #27 and @danflanagan8 agreed with in #28. I then made an attempt to simplify the new paragraph in staticmap.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Added nitpicky change for :void to test, didn't break anything.

Appears feedback has been addressed. Comment block reads well to me and leaning on quietone's expertise with the migration system.

I think the migrate_plus module should contain a fix for this as it is responsible for saving the static map to config. Essentially it is not be careful enough - as the comment says.

The change here for core looks good.

Committed and pushed 4da76f6b37 to 11.x and a1e704d78d to 10.3.x. Thanks!