Pipeline using explode and extract fails [#2675156]

Problem/Motivation

While working on #2620364: Variable to config: theme settings [d7] I discovered that the extract plugin fails when the previous plugin returns multiple true. When that happens, extract tries to extract on a scalar and an exception is thrown.

The part of the process pipeline with the problem is the conversion of the D7 theme setting name to the D8 configuration name. Seemed simple enough but it doesn't work.

process:
  # build the configuration name from the variable name, i.e.
  # theme_bartik_settings becomes bartik.settings
  configuration_name:
    -
      plugin: explode
      source: name
      delimiter: _
    -
      plugin: extract
      index:
        - 1
    -
      plugin: concat
      source:
        -
        - constants/config_suffix

This pipeline works if both extract and get use 'handle_multiples = TRUE'. Note that #2500495: Upgrade path for Color 7.x uses this same pattern.

Data model changes

Proposed resolution

Add 'handles_multiples = TRUE' to the extract annotation.

Remaining tasks

User interface changes

N/A

API changes

Comment	File	Size	Author
#11	2675156-11-test-only.patch	2.99 KB	maijs
#11	8.2.x: PHP 5.5 & MySQL 5.5 20,890 pass, 3 fail
#11	2675156-11.patch	3.56 KB	maijs
#11	8.2.x: PHP 5.5 & MySQL 5.5 20,892 pass
#10	2675156-10-migrate_extract_process_handle_multiple.patch	585 bytes	mitrpaka
#10	8.2.x: PHP 5.5 & MySQL 5.5 20,908 pass
#6	2675156-6-migrate_extract_process_handle_multiple.patch	518 bytes	rocketeerbkw
#6	8.1.x: PHP 5.5 & MySQL 5.5 18,535 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

24 February 2016 at 22:15

quietone created an issue. See original summary.

Comment #2

quietone CreditAttribution: quietone as a volunteer commented 24 February 2016 at 22:17

Title:

Set handle_multiples true in Extract destination plugin

» Set handle_multiples true in Extract process plugin

Correct title, s/destination/process.

Comment #3

quietone CreditAttribution: quietone as a volunteer commented 24 February 2016 at 22:23

Issue summary:	View changes
Related issues:		+#2675164: Pipeline using explode and concat fails

Comment #4

quietone CreditAttribution: quietone as a volunteer commented 1 March 2016 at 08:08

Title:

Set handle_multiples true in Extract process plugin

» Pipeline using explode and extract fails

Hopefully, a more informative title.

Comment #5

1 March 2016 at 08:08

Version:

8.0.x-dev

» 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

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

Comment #6

rocketeerbkw CreditAttribution: rocketeerbkw commented 21 April 2016 at 22:12

Status:

Active

» Needs review

File	Size
2675156-6-migrate_extract_process_handle_multiple.patch	518 bytes
8.1.x: PHP 5.5 & MySQL 5.5 18,535 pass

I understand why extract needs to handle multiples but I don't know what it has to do with the get process plugin.

Comment #7

benjy CreditAttribution: benjy at PreviousNext commented 17 June 2016 at 02:03

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

Comment #8

chx CreditAttribution: chx at Smartsheet, Migrate Rocks commented 1 September 2016 at 15:10

The patch is correct as far as I can see but yes needs tests.

Comment #9

1 September 2016 at 15:10

Version:

8.1.x-dev

» 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

Comment #10

mitrpaka CreditAttribution: mitrpaka as a volunteer commented 22 October 2016 at 13:29

File	Size
2675156-10-migrate_extract_process_handle_multiple.patch	585 bytes
8.2.x: PHP 5.5 & MySQL 5.5 20,908 pass

Testing if needs re-roll. It seems that target file has slightly changed. Patch updated.

curl https://www.drupal.org/files/issues/2675156-6-migrate_extract_process_handle_multiple.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   518  100   518    0     0    982      0 --:--:-- --:--:-- --:--:--   982
patching file core/modules/migrate/src/Plugin/migrate/process/Extract.php
Hunk #1 succeeded at 14 with fuzz 1.

Comment #11

maijs CreditAttribution: maijs as a volunteer and at Wunder commented 25 October 2016 at 13:21

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

File	Size
2675156-11.patch	3.56 KB
8.2.x: PHP 5.5 & MySQL 5.5 20,892 pass
2675156-11-test-only.patch	2.99 KB
8.2.x: PHP 5.5 & MySQL 5.5 20,890 pass, 3 fail

A test that verifies multiple value handling has been added. The patch with a test only should fail.

Comment #12

25 October 2016 at 14:08

Status:

Needs review

» Needs work

The last submitted patch, 11: 2675156-11-test-only.patch, failed testing.

Comment #13

maijs CreditAttribution: maijs as a volunteer and at Wunder commented 25 October 2016 at 21:14

Status:

Needs work

» Needs review

Test-only patch failed, but the other patch in #11 needs review.

Comment #14

iMiksu

Finland

CreditAttribution: iMiksu at Wunder commented 27 October 2016 at 14:36

Tests is looking good for me. Also together with the fix :)

Comment #15

iMiksu

Finland

CreditAttribution: iMiksu at Wunder commented 27 October 2016 at 14:37

Issue tags:

+DCampBaltics

Comment #16

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented 27 October 2016 at 20:06

Status:

Needs review

» Reviewed & tested by the community

Looks good, thanks!

Comment #17

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 28 October 2016 at 05:36

Status:

Reviewed & tested by the community

» Fixed

Committed and pushed 0aed30b to 8.3.x and 8dd44b1 to 8.2.x. Thanks!

Comment #18

28 October 2016 at 05:37

alexpott committed 0aed30b on 8.3.x

Issue #2675156 by maijs, rocketeerbkw, mitrpaka, quietone: Pipeline...

Comment #19

28 October 2016 at 05:37

alexpott committed 8dd44b1 on 8.2.x

Issue #2675156 by maijs, rocketeerbkw, mitrpaka, quietone: Pipeline...

Comment #20

11 November 2016 at 05:44

Status:

Fixed

» Closed (fixed)

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

Comment #21

quietone CreditAttribution: quietone as a volunteer commented 15 November 2016 at 21:52

Always nice to see more testing.

However, the process pipeline in the test uses extract followed by extract, when the issue is about explode followed by an extract. And it seems the original problem is not fixed because #2827656: Once a process plugin returns multiple values, all following plugins are expected to handle multiple values was opened.

Comment #22

maijs CreditAttribution: maijs as a volunteer and at Wunder commented 16 November 2016 at 08:20

I agree that the test does not cover the whole explode-then-extract process, but the issue and the fix certainly does cover the immediate problem with the extract process plugin where it wasn't even able to catch multiple values at all. I propose to limit the scope of this issue only to process plugin while dealing with a more systematic problem with multi-value handling in #2827656.