Problem/Motivation

When importing data using the Migrate module and the concat process plugin (http://cgit.drupalcode.org/drupal/tree/core/modules/migrate/src/Plugin/m...), if the data source has empty values, the delimiter is included anyways.

Example:

id value_1 value_2
1 'Value 1' 'Value 2'
2 'Value 1'
3 'Value 2'
4

If I'd wanted to have a migration that used concat to join value_1 and value_2 I'd have these settings:

my_field:
    plugin: concat
    source:
      - value_1
      - value_2
    delimiter: ' & '

Which will result in the following data:

id join value
1 'Value 1 & Value 2'
2 'Value 1 & '
3 ' & Value 2'
4 ' & '

I don't think 2, 3 and 4 are the desired result.

Proposed resolution

Add an array filter to strip out empty values when concat'ing. I don't see a use case of wanting these empty values, but this could be optional.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

pcambra created an issue. See original summary.

pcambra’s picture

Status: Active » Needs review
FileSize
710 bytes
pcambra’s picture

Issue summary: View changes
Jo Fitzgerald’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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).

pcambra’s picture

Thanks for the tags Jo, obviously this needs tests and a decision on the allow_empty option, I'll wait for a component maintainer to chime in before investing in adding those.

quietone’s picture

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.

Jo Fitzgerald’s picture

  • 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).

The last submitted patch, 7: 2913428-7-test_only.patch, failed testing. View results

heddn’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs review » Reviewed & tested by the community

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.

quietone’s picture

Status: Reviewed & tested by the community » Needs work

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.'

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
642 bytes
5.56 KB

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

heddn’s picture

Status: Needs review » Reviewed & tested by the community

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

quietone’s picture

@Jo Fitzgerald, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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?

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

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

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
5.35 KB
6.17 KB

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?

heddn’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 16: 2913428-16.patch, failed testing. View results

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.