Extended callback process plugin to call functions with multiple parameters

+++ b/src/Plugin/migrate/process/CallbackPlus.php
@@ -0,0 +1,52 @@
+class CallbackPlus extends ProcessPluginBase {

This should extend the core callback method. I think.

+++ b/src/Plugin/migrate/process/CallbackPlus.php
@@ -0,0 +1,52 @@
+      $param_arr = is_array($value)? $value : array($value);

Let's pass these as config options to the plugin. Rather than source values.

And needs tests => Needs work.

1. This should extend the core callback method. I think.

   I would agree, however, we cannot reuse anything from the original Callback class. I don't know what's best practice in that case. I've extended the original Callback class anyway.

2. Let's pass these as config options to the plugin. Rather than source values.

   Done.

3. I've added the neccesary tests.

It turns out that validation of migration processes takes place in the construct of the class, as of 8.6.x. That means that we can actually extend the Callback class: we would reuse part of the construct.

1. +++ b/src/Plugin/migrate/process/CallbackPlus.php
   @@ -0,0 +1,65 @@
   + *   id = "callback_plus"
   ...
   + * The callback_plus process plugin allows processing using callback functions
   ...
   + *     plugin: callback_plus
   ...
   +class CallbackPlus extends Callback {
   ...
   +    $value = call_user_func_array($this->configuration['callable'], $parameters);
   

   Maybe this shouldn't be called callback then, if we are using 'call_user_func_array'.

2. +++ b/src/Plugin/migrate/process/CallbackPlus.php
   @@ -0,0 +1,65 @@
   +    if (!isset($configuration['parameters'])) {
   ...
   +    elseif (empty($configuration['parameters']) || !is_array($configuration['parameters'])) {
   

   !isset == empty, no? At least in this context. Maybe throw some tests at this to prove it.

I am wondering about the case where subject/source position is not last.

Example
1. ltrim(), here we need the subject as first argument and this will work with the patch.
2. Str_replace(), here subject should be the last argument and this will not work with current patch.

This addresses the feedback from #11 and #12 by changing the checks, renaming it to match the original plugin it's extending and by allowing the source to be used at any index of callback parameters. Will try this on my project now.

Hmm, wrong use of array_splice and I forgot to map the provided parameters.

Works like a charm on my end.

Some example usage for migration commerce promotions:

  offer/target_plugin_configuration:
    - plugin: 'get'
      source:
        - 'percentage'
        - 'constants/boolean_true'
        - 'constants/offer_conditions'
    - plugin: 'single_value'
    - plugin: 'callback_array'
      callable: 'array_combine'
      source_index: 1
      parameters:
        - 'constants/offer_target_plugin_configuration_keys'
    - plugin: 'callback'
      callable: 'serialize'

  conditions:
    - plugin: 'get'
      source:
        - 'constants/condition_target_plugin_id'
        - '@_condition_configuration'
    - plugin: 'single_value'
    - plugin: 'callback_array'
      callable: 'array_combine'
      source_index: 1
      parameters:
        - 'constants/generic_plugin_keys'

First, we should add some tests here.

1. +++ b/src/Plugin/migrate/process/CallbackArray.php
   @@ -0,0 +1,113 @@
   + *     source_index: 2
   

   Can we call this just index?

2. +++ b/src/Plugin/migrate/process/CallbackArray.php
   @@ -0,0 +1,113 @@
   + *     parameters:
   + *       - 'search'
   + *       - 'replace'
   

   I think we need some tests here. If these values are dynamic from the source plugin instead of just static values, then I don't think they will get swapped for anything dynamic.

3. +++ b/src/Plugin/migrate/process/CallbackArray.php
   @@ -0,0 +1,113 @@
   + *   id = "callback_array"
   

   Maybe call it call_user_func_array?

Re #16:

1. I'm even, I thought the combination of 'source' and 'source_index' looked nicer together.
2. I'm already running the plugin in our project and I can tell you it swaps out dynamic values perfectly. I basically just copied the relevant part from the Get plugin.
3. The Callback plugin represents call_user_func, so I figured we should call the representation of call_user_func_array CallbackArray

But yeah, needs tests.

17.1: We already use index in other core process plugins, so using index here would carry on with that tradition. See Extract as an example.
17.2: Great. But there's a method on the Row as of 8.7 that encapsulates all that same logic for you. See https://www.drupal.org/node/3001535. In which case, we'll want to add a TODO and open an issue to remove the extra code once 8.6 is sunset. And we still want tests on that feature.
17.3: sure enough, it does. Shall we go back to calling it callback_plus? Using the name of this module as part of the feature set?

Agreed on all 3 points. Also did not know about the issue you linked, that's really useful information.

I came accross this and in search for a wrapper to call_user_func. I looked through all the patches, and all the efforts to differentiate between source and parameters are really confusing. I'm not the expert for Migrate, but as transform() doesn't use $value by reference, it technically shouldn't matter if we pass additional (call_user_)function parameters also in the source array?

OTOH, by separating source and parameters, the syntax for this plugin has become confusing, the code has become longer, and in the end it does the same as the oneliner in #2 (ok, it's a 5-liner).

What's the reason to separate between source and parameters before stuffing them both into call_user_func_array()? And is this reason really worth all the additional code? I'd stick to KISS and go back to #2

Hi all,

thanks for the effort applied in this code, it's really useful for me. I just developed a similar code to resolve my own problem when I found it. So I merged both (my code and this patch) and now I think that it's more stable.

Following the last comments:

18.1: We already use index in other core process plugins, so using index here would carry on with that tradition. See Extract as an example.

I changed the var to use index in order to follow the traditional name in other parts of migrate and migrate+ as suggested.

18.2: Great. But there's a method on the Row as of 8.7 that encapsulates all that same logic for you. See https://www.drupal.org/node/3001535. In which case, we'll want to add a TODO and open an issue to remove the extra code once 8.6 is sunset. And we still want tests on that feature.

And it works perfect! I used it changing the original code.

18.3: sure enough, it does. Shall we go back to calling it callback_plus? Using the name of this module as part of the feature set?

I'm not sure about it, CallbackArray seems as good name as the other, you know that one of the most difficult thing in development is set names... ^^ I maintain for the moment CallbackArray. :)

And the last and important point, I added some Unit tests for this plugin.

Please, I'm happy if someone can review it.

Hi again,

A college discovered that the code doesn't work with constant. Some of the functions use constant to work and be configured (for example the function array_unique) and it's very useful allow do use it. I know that you can use the constant original value in place of the constant token, but it's not very intuitive and it's difficult to read the code.

I added the possibility of use constant in the definition, but with an extra config param allow_constant that have to be defined to make the replacement. I create the extra param because I think that is the best way to avoid problems with replacements not wanted.

I added 2 more unit tests case to manage this new param.

The tests are not running. For example, looking at the testbot results for the patch in #23, I see

Drupal\Core\Test\Exception\MissingGroupException: Missing @group annotation in Drupal\Tests\migrate_plus\Unit\process\CallbackArrayTest in /var/www/html/core/lib/Drupal/Core/Test/TestDiscovery.php:356

We need the tests to run (and pass), so I am setting the status to NW.

Sorry, I forgot to add the @group annotation.

Added all annotation to the test,

I just reran those tests, which had the same failures as the parent branches

From the issue summary:

I doubt that cores callback plugin could be extended that way.

Actually, I think it would be pretty easy! The trick is to use the "splat" operator, which turns an array into an argument list.

The implementation of the callback plugin is so simple:

  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    return call_user_func($this->configuration['callable'], $value);
  }

All we have to do is add a configuration option; for now, I am calling it "splat":

  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    if ($this->configuration['splat'] ?? FALSE) {
      return call_user_func($this->configuration['callable'], ...$value);
    }
    else {
      return call_user_func($this->configuration['callable'], $value);
    }
  }

Then the example from #2 becomes

source:
  constants:
    slash: '/'
process:
  destination_path:
    plugin: callback
    callable: ltrim
    splat: true
    source:
      - source_path
      - constants/slash

The examples in #15 are trickier. I think the first one could be done like this:

  target_values:
    - plugin: get
      source:
        - percentage
        - constants/boolean_true
        - constants/offer_conditions
  offer/target_plugin_configuration:
    - plugin: callback
      callable: array_combine
      splat: true
      source:
        - constants/offer_target_plugin_configuration_keys
        - '@target_values'
    - plugin: callback
      callable: serialize

My version is one more line, but I think it is easier to see what it is doing.

The second example from #15 becomes

  conditions_values:
    - plugin: get
      source:
        - constants/condition_target_plugin_id
        - '@_condition_configuration'
  conditions:
    - plugin: callback
      callable: array_combine
      splat: true
      source:
        - constants/generic_plugin_keys
        - '@conditions_values'

Caveat: I have not tested any of these examples.

On second thought, !empty(...) is more familiar than ... ?? FALSE.

For implementing the "splat" option, we could be shorter but perhaps less clear:

  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    $args = empty($this->configuration['splat']) ? [$value] : $value;
    return call_user_func($this->configuration['callable'], ...$args);
  }

Or we could save a couple of lines in the original suggestion:

  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    if (!empty($this->configuration['splat'])) {
      return call_user_func($this->configuration['callable'], ...$value);
    }
    return call_user_func($this->configuration['callable'], $value);
  }

We discussed my suggestion from #27-28 at #3200725: [meeting] Migrate Meeting 2021-03-04 and the feedback was very positive, so I am moving this issue to the core queue and supplying a patch. I am not attaching an interdiff, since this is an entirely different approach.

A few changes from my original suggestion:

• Instead of using the "splat operator", I use call_user_func_array() if the new option is set.
• I call the new option array_args

The patch includes test coverage.

I have not yet updated the issue summary, so I will add the tag for an update.

The class comment used to start off,

 * Passes the source value to a callback.
 *
 * The callback process plugin allows simple processing of the value, such as
 * strtolower(). The callable takes the source value as the single mandatory
 * argument. No additional arguments can be passed to the callback.

I changed that to

 * Passes the source value to a callback.
 *
 * The callback process plugin allows simple processing of the value, such as
 * strtolower(). To pass more than one argument, create an array of the
 * arguments and set the array_args option.

Perhaps I should change it a little more.

Sorry, I attached a bad patch to the previous comment.

Disable spell check for one line.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Callback.php
@@ -10,11 +10,12 @@
+ * - array_args: Whether to interpret the source as an array of arguments.

Should this key be marked as optional here? Looks like array_args: (optional) Whether to interpret the source as an array of arguments. would be in line with other documentation.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Callback.php
@@ -38,6 +39,23 @@
+ * An example where the callback accepts more than one argument:
+ *
+ * @code
+ * source:
+ *   plugin: ...
+ *   constants:
+ *     date_format: 'Y-m-d'
+ * process:
+ *   destination_field
+ *     plugin: callback
+ *     callable: date
+ *     array_args: true
+ *     source:
+ *       - constants/date_format
+ *       - source_timestamp
+ * @endcode
+ *

I personally think we could come up with a better example here. This specific case could easily be handled with the format_date plugin, right?

Is there a use case we could present that can not be accomplished with another core plugin?

One of the tests added for this new feature uses rtrim as the callable function. Is there a good example with rtrim as the callable?

@danflanagan8:

Thanks for taking a look!

#32: Yes, I will add "(optional)" in the next patch.

What do you think about the code comment (#29)?

#33: What do you think of replacing both the rtrim test and the example in the doc block with something like this? (needs testing)

source:
  constants:
    slash: '/'
process:
  field_link_uri:
    - plugin: callback
      callable: rtrim
      source:
        - url
        - constants/slash

@benjifisher,

I love the new rtrim example. That's really easy to follow and totally practical. If your updating tests, I would be interested in seeing a test where the function takes three parameters just so there's some more variety in the tests. I know there's an str_replace plugin in migrate_plus, but I see str_replace as a good callback with three arguments to test.

Regarding the comment in #29, I don't have a big problem with it. If I were forced to make a change, perhaps I wouldn't mention "creating" an array because that might make it seem more complicated than it is. I would describe "passing" an array.

* Passes the source value to a callback.
*
* The callback process plugin allows simple processing of the value, such as
* strtolower(). To pass more than one argument, <strong>pass an array as the source</strong>
* and set the array_args option.

As long as array_args is set I think the array is interpreted as args whether it's something like this:

  source: my_array

or

  source:
    - value_1
    - value_2

In the first case, maybe you already have the array and nothing needs to get created. In the second case, I don't really feel like I'm "creating" an array. "Creating" makes me feel like it should be a separate step or something. But maybe I'm reading too much into the word create.

The attached patch implements the changes discussed in #32-35.

As a bonus, the example using rtrim() does not have a string that will make the spell checker unhappy.

I still have to update the issue summary.

I didn't review the code in detail, but quickly checked the latest patch and noticed we already have test coverage and examples 💪
I also like the idea, and I could probably write fewer custom plugins if I had this feature before.

This is gonna be a new feature, which means we need to create a change record. (moving to Needs Work for that)

I'm also wondering if it makes sense to create a follow-up to deprecate existing process plugins in favor of Callback.

#36 looks really good to me.

@danflanagan8:

Thanks for the encouragement!

@Matroskeen:

Thanks for the nudge to write a change record. I borrowed a couple of examples from the documentation block: here is the draft change record.

@benjifisher thanks, CR looks good to me. I'll let someone else review and, hopefully, RTBC.

This works great for me. I was able to replace a usage of the `str_replace` plugin from `migrate_plus` with the config structured like the sample below:

my_processed_string:
  plugin: callback
  callable: str_replace
  array_args: true
  source:
    - constants/search
    - constants/replace
    - my_original_string

The migration I ran had other instances of the `callback` plugin with single argument functions. None of that broke or required any changes.

Setting as RTBC.

I just added #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest for the unrelated test failure. Let's see if there is any response on that issue before we ask the testbot to reconsider.

If the testbot just switched to DST, then a lot of tests will fail, and we do not want to queue them all up at once.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest was committed. Ordered retest and put this issue back to RTBC per #42.

I really like the solution here - using the existing callback process plugin is very elegant. The think that's got me pondering is whether the option array_args is well named. I'm bit fearful of painting a bikeshed but at first glance this is a bit ambiguous. I think what we're really doing here is unpacking the source. I made the change locally and to me it reads better because it reflects that this is about the source and reflects what happens to it. Also the language of unpacking is directly from PHP - see https://3v4l.org/khmSR

As usual, naming things is the hardest part! While I see that "unpack" is used in this situation with arrays, I also see that there's a completely unrelated built-in php function called unpack: https://www.php.net/manual/en/function.unpack.php. I wonder if that would cause any confusion.

I think @benjifisher originally used the term "splat" which exists in other languages. I'm not seeing it when I search the php manual.

I'm going to throw out an idea that I don't necessarily like, but occurred to me at one point. The new boolean determines whether the plugin should leverage call_user_func or call_user_func_array. It would make a lot of sense to call the new boolean call_user_func_array such that call_user_func_array: true would do the new thing with multiple args.

The reason I don't like that is that it's not abstract in any way. If for some reason the underlying code changed, then that parameter might get confusing. Or maybe the plugin is so strongly tied to call_user_func and call_user_func_array that there's no real need to abstract anything.

@danflanagan8 in the implementation in #47 call_user_func is used in both instances. And unpacking is what PHP calls it... see https://www.php.net/manual/en/migration56.new-features.php#migration56.n... and https://3v4l.org/khmSR ... obvs it is amusing the PHP anchor uses splat - but I think we should go with docs.

I first came across the term "splat" in a StackOverflow answer. Looking it up, I found the same page mentioned in #49:

Arrays and Traversable objects can be unpacked into argument lists when calling functions by using the ... operator. This is also known as the splat operator in other languages, including Ruby.

I originally thought of implementing this feature with call_user_func(...$value). I used "splat" in some comments here as a working name for the new option, but never thought it was actually a good name. I decided to use the older implementation, call_user_func_array($value).

I am happy to use something other than array_args. I will ask for comments on Slack.

@alexpott I totally missed the change from using call_user_func_array to using call_user_func with .... I retract my comment!

@danflanagan8, I think you have that backwards. Was I ambiguous?

@benjifisher I was referring to @alexpott making the change away from call_user_func_array to unpacking in the patch in #47, not to the opposite change that you made in #29. So we're back to where you started, I guess!

benjifisher asked in #migration about the name of the new property which is unpack_source in the patch in #47. Both unpack and unpack_source work for me. I like the brevity of unpack and in context I can't think of what else it would refer to other than the source but then I am used to mucking around with process plugins. The extra clarity of unpack_source is likely to help others and that basis I lean to unpack_source.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Callback.php
@@ -38,6 +40,25 @@
+ *     slash: '/'

Does not need to be quoted.

Since array_args doesn’t hold any source and just a boolean that indicates whether to use “unpacking” or not, I would vote for
unpack_source option.

Thanks for making callback process plugin more useful!

On the option name, both something simple as single_argument or the suggested unpack_source sound good.

@@ -63,6 +84,12 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
    * {@inheritdoc}
    */
   public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
+    if (!empty($this->configuration['unpack_source'])) {
+      if (!is_array($value)) {
+        throw new \InvalidArgumentException('When "unpack_source" is set, the source must be an array.');
+      }
+      return call_user_func($this->configuration['callable'], ...$value);
+    }
     return call_user_func($this->configuration['callable'], $value);
   }

One suggestion, I am wondering why we need the two cases.
i.e. creating an array of one element if the option is unset, and always call it the same way.

I am guessing that not enforcing the array is about being back-wards compatible, so I guess the option is needed for that differentiation.
Not sure about the process, but we may want to just change the input to be required as an array in the future.

@marvil07 imagine the callback was array_unique and you wanted it to filter an array of items - it that instance automatic unpacking would break. That's another about unpack_source - it is very explicit about what is going to occur when the callback is processed.

@alexpott, indeed, I forgot about array operations, thanks for pointing that out!
That means the option is truly needed.

Applying the patch from #47 failed, likely b/c of the unrelated/duplicated hunks on core/modules/field/tests/src/Functional/FieldHelpTest.php.
Looking at git history, it seems like it is colliding with the change from commit 45e5a8aedb0c73beb7f44f96f5bc9647ba1d4a95.
Hence, I am marking as NW.

I have tested the changes locally with a simple migration on a custom module, attaching it here for reference.
The changes work as expected!
Also, please notice that the last patch changes the option to be unpack_source which seems to be the consensus.

I would RTBC, but that is pending on a re-roll to remove the mentioned two conflicting hunks, to be removed, so NW.

I will update the patch now that we have settled on a name for the option.

@marvil07:

The patch in #47 included an unrelated change to core/modules/field/tests/src/Functional/FieldHelpTest.php. It is a bit of relief to see that I am not the only one who occasionally makes a goof like that.

The attached patch makes the same changes as #47 (replacing array_args with unpack_source) without the accidental change to FieldHelpTest.php. I also removed the unnecessary single quotes (#54).

I made corresponding changes to the issue summary. I tested locally with callback_test as in the issue summary.

I will also update the change record.

@benjifisher thanks for fixing my goof - definitely not my first time :)

Crediting everyone who's contributions to the issue have affected the current patch.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Callback.php
   @@ -38,6 +40,25 @@
   + *   plugin: ...
   

   Every time I look at this I want it to be something else. I know the ellipsis is correct but can it be 'foo'?

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/Callback.php
   @@ -63,6 +84,12 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +        throw new \InvalidArgumentException('When "unpack_source" is set, the source must be an array.');
   

   Process plugins should throw a MigrateException not an InvalidArgumentException. We want to control the processing in MigrateExecutable and record messages etc etc. Referring to Extract for an example, let's use this.

        throw new MigrateException(sprintf("When 'unpack_source' is set, the source must be an array., instead it was of type '%s'", gettype($value)));
   

I read the test and it does test the new functionality with a likely popular and simplistic case. Does it need more complex cases as well. I am not sure.

@quieone, I had the same though about adding a test with more and/or more complicated arguments. I wonder what we would think about something like this as an additional line in the data provider:

'str_replace' => ['str_replace', [['one', 'two'], ['1', '2'], 'One, two, three!'], '1, 2, three!'],

It is different from the others in that 1) the function uses three arguments and 2) the arguments are a mix of arrays and strings. I think both of those would be nice coverage to add.

Made changes as per comment #63.

I just thought of another use case. We already have the str_replace plugin, which is a wrapper for str_replace() or preg_replace(), but I want to put preg_match() in a pipeline with skip_on_empty.

@ravi.shankar:

If we change the exception we throw, then we also have to update the test. If you can set up your local environment to run tests, then it is good practice to test any tests that your patch changes before uploading your patch to an issue.

@quietone:

The Callback class already throws InvalidArgumentException in its constructor. So I wonder whether we should fix that in this issue or keep the exception from #60 and change all of them in a follow-up issue.

Looking at other process plugins, I see that MigrateException is used all over the place, including one constructor: MachineName.

I also searched the other process plugins for InvalidArgumentException, and found only one use, where it was being caught, not thrown. Based on that, I am going ahead and updating the constructor here.

While looking through the other process plugins, I noticed that the @throws annotation was added inconsistently. Let’s create a follow-up issue for that. Is it appropriate to add it to MigrateProcessInterface, or should we add it to the implementations that explicitly throw an exception?

@danflanagan8:

Thanks for the specific suggestion. The nice thing about provider callbacks is that it is so easy to add test cases.

@benjifisher, Thanks for the changes.

I am pleased to see the additional test. The changes to the test and the change to using a MigrateException in the transform look good to me.

I didn't look at the constructor until now and I agree that it should be changed but if I understand scoping then it should be a follow up. I did a bit of research and found that the patch that added the InvalidArgumentException originally used a MigrateException but heddn requested it be changed in 2936463#27. I think we should ask him about that.

In reviewing the patch I found this concerning the change to MigrateException.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Callback.php
@@ -51,10 +73,10 @@ class Callback extends ProcessPluginBase {
+      throw new MigrateException('The "callable" must be set.');
...
+      throw new MigrateException('The "callable" must be a valid function or method.');

The coding standard examples for exception messages have the property/field in single quotes. "The 'callable' must be a valid function or method.". Let''s change them to that format.

Now to consider the question about adding @throws to MigrateProcessInterface. I've been dabbling in the coding standards issues recently and fixes are done by sniff, not individual cases or modules. I don't think we should make an issue to fix this.

Actually, on further thought, we can fix the exception in the constructor and the @throws in a single issue. One that is scoped to ensure that core process plugins are throwing MigrateExceptions and to update the docs in \Drupal\migrate\Plugin\MigrateProcessInterface::transform. What do you think?

@quietone:

Thanks for putting some thought into this.

One reason I prefer to change the exceptions in the constructor as part of this issue is that we have a single testCallbackExceptions(), with a data provider, that tests the exceptions from both the constructor and transform(). If we throw MigrateException in transform() and InvalidArgumentException in the constructor, then we have to make that test a little more complicated. That is not a problem if we really want to have two different exceptions, but if we plan to end up with consistent exceptions then it seems like more churn than it is worth.

As I said in #68, I am also willing to throw InvalidArgumentException in transform(), and make no changes to the constructor, and then change all of them in a follow-up issue.

The rational for invalid argument is because that is what we have in the constructor. We have an invalid argument. Actually, I prefer invalid plugin exception. Which I think was also added. The object is a Singleton after construction. We don't want to effect flow here. We want to inform the developer they need to fix things. Move things that never change, things that are validation of configuration to constructor. Because you can't change config. So no need to check it in every invocation of the plugin. Just check it once at construction and be done.

@heddn, thanks for explaining!

I think the right thing to do is to stay within the scope of this issue. The attached patch reverts the changes to the constructor and makes the test a little more complicated. As long as I am doing that, I think it is in scope to add a doc block to the test.

By "invalid plugin exception" do you mean InvalidPluginDefinitionException? If we want to throw that in the constructor, then we can change it in a separate issue, and consider the MachineName process plugin at the same time.

I reviewed that patch and could only find the following small suggestions.

1. +++ b/core/modules/migrate/tests/src/Unit/process/CallbackTest.php
   @@ -33,15 +34,48 @@ public function providerCallback() {
   +      'date format' => ['date', ['Y-m-d', 995328000], '2001-07-17'],
   +      'rtrim' => ['rtrim', ['https://www.example.com/', '/'], 'https://www.example.com'],
   +      'str_replace' => ['str_replace', [['One', 'two'], ['1', '2'], 'One, two, three!'], '1, 2, three!'],
   

   I think it would be easier to read if these were multiline.

2. +++ b/core/modules/migrate/tests/src/Unit/process/CallbackTest.php
   @@ -33,15 +34,48 @@ public function providerCallback() {
   +    $value = $this->plugin->transform($args, $this->migrateExecutable, $this->row, 'destination_property');
   

   $value is not used and can be removed.

Regarding the exceptions, maybe a topic for a meeting? Just thinking out loud.

@quietone, thanks for the suggestions. The attached patch addresses both points in #74.

BTW, I came up with another application. My site has a List(text) field with three possible values. My source is a CSV file with a column for each of the three values. The following snippet does it.

  list_text_filter:
    plugin: get
    source:
      - key_1
      - key_2
      - key_3
  field_list_text:
    - plugin: callback
      callable: array_combine
      unpack_source: true
      source:
        - constants/list_text_values
        - '@list_text_filter'
    - plugin: callback
      callable: array_filter
    - plugin: callback
      callable: array_keys

@benjifisher, thank you. Nice example too! That should be included on the 'nifty' process pipelines examples page. Wasn't there a plan to make such a page?

I re-read the change record and added link to the rtrim() documentation as suggested by danflanagan8 in a comment. The IS is correct as well.

Onward to RTBC.

There is already such a page! I added the example in #75 as Example: combine boolean fields into a List(text) field.

Oops, I did not mean to change the status.

Committed and pushed a3ca88ebbf to 9.3.x and 50d267d893 to 9.2.x. Thanks!

@benjifisher, thanks for the link. I was on the parent page and missed it! The page is now bookmarked.