Add an explode/separator process plugin

I don't regard this as a migrate_plus issue. migrate core module doesn't provide Explode process plugin to explode multiple values from a single line in source, regardless whether that's XML, CSV or DB. Fortunately, it's very simple to create such a process plugin. (See an excellent example in this article - Migrating to Drupal 8, see Users section.)

In essence:

1. Create a file MY_MODULE/src/Plugin/migrate/process/Explode.php
2. Put in the following code:

/**
 * @file
 * Contains \Drupal\MY_MODULE\Plugin\migrate\callback\Explode.
 */

namespace Drupal\MY_MODULE\Plugin\migrate\process;

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

/**
 * @MigrateProcessPlugin(
 *   id = "explode",
 * )
 */
class Explode extends ProcessPluginBase {
  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    $result = explode($this->configuration['delimiter'], $value);
    return $result;
  }
}

3. In migration configuration file use the following:

my_dest_field:
  -
    plugin: explode
    delimiter: ','
    source: my_source_field

Re-assigning to new project name-space.

Splitting by a comma or other delimiter didn't make it into core yet. I'm moving this over to the core issue queue for the time being to see if we can still fit it into things at this late stage. If not, this is the type of thing that will get added to migrate_plus. To be clear, this isn't specific to CSV migrations, it is general.

In the mean-time, this can be done in a prepare row callback.

Retitling to reflect the current direction.

@mikeryan, should I make a patch for this with a test?

@quietone: well, someone should at some point, but with a week till release I wouldn't prioritize new features for 8.0.0 - my inclination is to put this into migrate_plus for now and aim to submit to core for 8.1.0. In general, I think at this point the priority should be trying to get the more significant "needs review" issues to RTBC, and triaging open bug reports to see if there's anything we really want to try to get fixed in the next week.

Ended up writing this while working on the D7 color and theme variable migrations.

Fixed the namespace.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
@@ -0,0 +1,41 @@
+    if (is_string($value[0])) {

+++ b/core/modules/migrate/tests/src/Unit/process/ExplodeTest.php
@@ -0,0 +1,81 @@
+    $source = ['color_bartik_stylesheets'];

Why do we check value[0], that should be just $value? And then $source wouldn't be an array.

Yea, your right.

Making a small change. I don't see why sprintf is used there (maybe copy/paste error). And I think this needs to go to 8.1.x.

Migrate is marked as experimental, I don't see why this has to wait until 8.1

I've looked over the code. It looks solid.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
@@ -0,0 +1,41 @@
+      throw new MigrateException(sprintf('%s is not a string', var_export($value, TRUE)));

Are we sure we want to use var_export? Couldn't that return nasty things?

@heddn: That is a general pattern in all the migrate plugins.

It isn't that common in Migrate. And I think we'd be better off not introducing more occurrences.

I don't see a problem with var_export, what do you suggest instead? Happy with print_r as well for me.

Either is fine, but SafeMarkup::checkPlain() the values before inserting them into the exception.

Why would we do that? checkPlain is for escaping a string that is going to be rendered in a HTML document, we're throwing an exception. If something else is catching that exception and printing it to the page, that's their problem.

Doesn't watchdog grab those? So it would be printing that all to the screen for us. Does it do the checkPlain()?

If watchdog is printing it to the screen, then watchdog will be escaping it.

In that case, I'm probably being paranoid.

We should add multiple true, because that other process plugins in the pipeline should be able to iterate over the exploded values

+++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
@@ -0,0 +1,46 @@
+  public function multiple() {
+    return TRUE;
+  }

This would should be in the annotation, and we should add a new test if we do in-fact need it.

hi, here's a patch based off of #26 with the annotation, I'll get to work on the test now.

Forgot to remove ::multiple() in the class.

Hey all, so the currently existing suite still passes, what else might need to have coverage here?

Add a unit test that tries to adjust a value into a multi-valued result and see if a following process plugin uses each individual value. Rather than on the original full string.

Hi, sorry but I have no idea what I'm doing here. I've got this going so far --

<?php
  /**
   * Test handle_multiples
   */

  public function testProcessesMultiples() {
    $configuration = [
      'delimiter' => '|',
      'process' => [
        'bar' => [
          'plugin' => 'machine_name',
          'source' => 'value'
        ]
      ]
    ];
    $this->plugin = new Explode($configuration, 'explode', [], $this->moduleHandler->reveal(), $this->migrationPlugin->reveal());
    $source = 'this|that|the other';
    $result = ['this', 'that', 'the_other'];

    $row = new Row(array(), array());

    $transformed_value = $this->plugin->transform($source, $this->migrateExecutable, $row, 'bar');
    $this->assertSame($result, $transformed_value);
  }

which fails --

Failed asserting that Array &0 (
    0 => 'this'
    1 => 'that'
    2 => 'the other'
) is identical to Array &0 (
    0 => 'this'
    1 => 'that'
    2 => 'the_other'
)

So obviously I'm missing how to configure this test to chain processes together. I've been in the IteratorTest for most of the afternoon and am not able to tease apart how to set this up correctly.

I'm sorry, conceptually I know what needs to be done but I have no idea how to actually implement this test (the setup of these pipelined transformations).

@jgrubb, for your test to pass the last item in the array 'the other' need to match. Try changing 'the other' to 'the_other' in $source. Also, instead of making a new row, you can use $this->row.

Can anyone provide a use case where multiple is needed? If so, what would a migration template look like that uses the multiple feature?

Looking at the docs, for ::multiple() and handle_multiple they have different goals.

In this case I would say ::multiple() needs to be implemented and return true because explode always returns an array and chained plugins in the pipeline need to know.

If the plugin should handle_multiple is a different question. Are we expecting an array of strings to be exploded?

Patch in #26 looks like the right approach to me.

I fully agree with #36 from @esclapes. The correct code is to define multiple(), since this function outputs multiple values (and does _not_ handle them).

This patch extends #26 to include a limit 'field', so that this can be used without problem in limited multi-value fields.

I've simplified the unit test somewhat based on the simpler TestConcat, and added a simple version of the process chaining between explode and concat.

It might be useful to have a better migration simpletest where we can define a full migration, and chain these values properly, but that's outside the scope of this issue, since that simpletest would focus on the proper handling of the multiple() = TRUE and handles_multiple plugins.

+++ b/core/modules/migrate/tests/src/Unit/process/ExplodeTest.php
@@ -0,0 +1,112 @@
+class TestExplode extends Explode {
...
+  public function setDelimiter($delimiter) {
...
+  public function setLimit($limit) {

Do we need a stub class, public function __construct(array $configuration, $plugin_id, $plugin_definition) the parent constructor definition looks simple enough that we could just pass in the values and set the config at the same time? See ExtractTest for example.

Rest looks good, RTBC once we've removed the stub class for testing.

Removing the stub classes as per #38 + minor drupalcs warning.

Great, looks good to me.

This is exactly what I need for an upcoming import.

If I want to implement this before this gets into dev (or, not using the latest dev), do I just need the latest patch in #39?

@jenstechs, yes. Make sure your version is the same as shown at the top of this issue.

Committed 4e39681 and pushed to 8.0.x and 8.1.x as migrate is experimental and this new feature does not introduce any risk to non-experimental code. Thanks!

@heddn btw exception messages are not supposed to use the markup or translation tools at all. Anything that displays them has to (and does) handle this.

Explode had its own docs already, where the configurable delimiter is documented. Migrate source CSV had a configurable delimiter also; as it's underling php CSV also supports it. Please check out on their respective handbook pages

Hi @heddn thanks for this info. It answers my question which I asked here. My only question is whether, in the config file, to use the syntax recommended in comment #3 above in the config file?.

Wrong version.