Update documentation for handling dots in the static_map plugin

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 last submitted patch, 23: 2827897-23.drupal.patch, failed testing. View results →

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!

quietone → closed merge request !7262

Automatically closed - issue fixed for 2 weeks with no activity.