Concat process plugin should filter empty values

Firstly, this will need a test to demonstrate the problem (and prove that the patch fixes it). Secondly, perhaps we should add "allow_empty" to the plugin configuration (probably set to false by default) for those (rare) cases when I user might want ' & Value 2' (or similar).

Yea, Jo Fitzgerald is on it, as usual. Everything in #4 sounds like a good implementation, with the addition of updating the API documentation will need updating.

• Added allow_empty to the plugin configuration (and relevant documentation).
• Added tests (and tidied up the existing tests in the process which I think is acceptable as part of this issue considering the amount the tests are changing).

Looks good to me. Docs are added and tests too. I don't think we need a CR for the very minor API addition. This can probably make its way into 8.4, since migrate is still beta/experimental. So I've switched back versions. And adding a test for good measure.

Really sorry to set this back to NW. Using the structure 'if set' was pointed out in another migration issue (which of course I can't find) so let's use it here.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Concat.php
@@ -16,6 +16,8 @@
+ * - allow_empty: (optional) Whether empty values should be included in the

It would be better and be consistent with the other process plugin docs to use something like; '(optional) If set, empty values will be included in the output. Defaults to FALSE.'

Updated the comment for consistency, as suggested by @quietone.

Docs fixes added in response to #10. Back to RTBC.

@Jo Fitzgerald, thanks!

I wonder if changing the default behaviour is a good idea - someone's migrations could be relying on this? Maybe we should swap the configuration default?

Also migrate is beta and this plugin is part of the API so this needs a change record.

I have attempted to address @alexpott's very valid argument in #14. I have corrected that oversight, but in doing so I have come across a problem: when exclude_empty is TRUE zero values are excluded (when they shouldn't be, imo). Any suggestions?

Honestly, this whole piece of work seems a little like we could either 1) move it to contrib - migrate_plus is a good landing place or 2) leave it to a custom extention. Where in core do we need this functionality? Can we get a convincing argument why filtering empty is needed? we can always run array_filter with a callback process plugin beforehand, no?

@Jo Fitzgerald
we could use strlen as second paramater of array_filter, as suggested in a comment of php.net: https://secure.php.net/manual/en/function.array-filter.php

$value = array_filter($value, '\Drupal\Component\Utility\Unicode::strlen');

I just changed that in attached patch

Although initially my reviews suggested that I agree with this change, after all it seems quit logical. However, I am no longer convinced it should be in core. It would be a nice addition to migrate_plus, giving folks an option and could reduce the number of processes in some pipelines.

First, the concat process plugin implements implode. Once you know that you will expect implode not some variation. Now, the name concat is somewhat unfortunate where implode seems to be the best choice but I don't know the history. And after all, there is explode.

Two, it isn't needed in core. And as heddn points out in #17 there are ways to filter out the empty strings if that is what is needed.

Three, changing the default behavior of concat is likely to cause grief as alluded to by alexpott in #14.

After thinking about it some more, if we moved to contrib... What would we name this thing? Process plugins work best when they do single chunks of work. And then chain them. Perhaps that's what should be done here. Just use callback with array filter. And this get closed won't fix?

Re tagging as feature rather than task.

Good point about the name.
We could add documentation showing how to use concat with array_filter somewhere in https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins. Would that be sufficient?

With the array_filter callback solution, we can't choose the second parameter to pass to array_filter.

And we need it to avoid the problem of zero values

Then perhaps we should add an array_filter process plugin to migrate_plus?

With such a plugin, we could solve the problem, think I.

I can provide a patch (and an issue in migrate_plus project) if you want

Reviewing the issue there seems to be consensus to move this to Migrate Plus. This came after a comment by alexpott that the patch in #14 would break existing migrations. While a later patch, #20, may have fixed that it was after heddn suggested, #17 that this be moved to Migrate Plus. He pointed out that this isn't needed by core, there is a workaround and no one has presented a argument why this is needed. The following comments support the move. The final discussion was to create an array_filter process plugin instead of changing concat.

An issue needs to be made in Migrate Plus and then this becomes a won't fix.

Discussed this at the migrate meeting today, #3160932: [meeting] Migrate Meeting 2020-07-30.

The desired result can be achieved by using a pipeline of callback, with array_filter before the concat to obtain the desired result. That kind of processing is what the pipeline is for and it means there isn't a need for change to concat. Creating an array filter process plugin can also be avoided if the callback process plugin is improved. There is an issue in Migrate Plus for that, #2882276: Extended callback process plugin to call functions with multiple parameters.

@pcambra, thank you for suggesting this change to concat and for the patch. Also to jofitx and SylvainM for adding patches and to alexpott for the review that started a rethink of this issue.

Here is an example using the pipeline described above in #34:

my_field:
  -
    plugin: callback
    callable: array_filter
    source:
      - value_1
      - value_2
  -
    plugin: concat
    delimiter: ' & '