This patch introduces multiple handling:

  1. if the input is an array when none is requested, Get switches the pipeline to multiple mode.
  2. In multiple mode, if the process plugin can't handle arrays itself MigrateExecutable::processRow will iterate that array and call the plugin for each value.

This existed in Drupal 7 somewhat here and there but in a way that was specific to each "plugin" as it existed there. This is centralized and while makes the code a bit more complicated it makes writing migrations a lot simpler as we have already seen in role migrations (which pass with the sandbox, I'm working on getting the relevant pieces of sandbox into core).

Incidentally, this patch adds bypass support to the static map but I really don't want to go into hunk-by-hunk submission for core; I am already doing it for MigrateExecutable and it's a PITA.

CommentFileSizeAuthor
#9 2154209_9.patch15.41 KBchx
#9 interdiff.txt5.17 KBchx
#8 2154209_8.patch15.65 KBchx
#8 interdiff.txt5.46 KBchx
#6 interdiff.txt2.2 KBchx
#6 2154209_6.patch13.71 KBchx
#2 2154209_3.patch12.54 KBchx
mme.patch13.48 KBchx

Comments

Status: Needs review » Needs work

The last submitted patch, mme.patch, failed testing.

chx’s picture

chx commented
Title: MigrateExecutable cleanup; tests; multiple handling » Process refactor: multiple handling
Issue summary: View changes
StatusFileSize
new 2154209_3.patch12.54 KB
chx’s picture

chx commented
Issue summary: View changes
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/modules/migrate/lib/Drupal/migrate/Annotation/MigrateProcessPlugin.php
@@ -0,0 +1,30 @@
+   * Whether the plugin handles multiples itself

    Nitpick: missing "." 

  2. +++ b/core/modules/migrate/lib/Drupal/migrate/ProcessPluginBase.php
@@ -0,0 +1,21 @@
+
...
+    return FALSE;
...
+  public function multiple() {
...
+}
...
+  }

    Let's document that as it is the core of that patch. 

  3. +++ b/core/modules/migrate/lib/Drupal/migrate/ProcessPluginBase.php
@@ -0,0 +1,21 @@
+ * The base class for all plugins.

    Mabye: "The base class for all process plugins". 

  4. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/StaticMap.php
@@ -19,23 +18,33 @@
+        throw new MigrateException('Lookup failed.');
...
+        throw new MigrateException('Can not lookup without a value.');

    Work for a possible follow up: provide a little bit more context in the exception.

Status: Needs review » Needs work

The last submitted patch, 2: 2154209_3.patch, failed testing.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new 2154209_6.patch13.71 KB
new interdiff.txt2.2 KB

Looks like a bot fluke. Addressed issues. Seems the patch missed the interface change altogether.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you!

chx’s picture

chx commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new interdiff.txt5.46 KB
new 2154209_8.patch15.65 KB

Sigh.

chx’s picture

chx commented
StatusFileSize
new interdiff.txt5.17 KB
new 2154209_9.patch15.41 KB

Disregard #8 , better idea.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I like that we safe some bit of code here.

The last submitted patch, 8: 2154209_8.patch, failed testing.

chx’s picture

chx commented

I'd like to apologize to webchick for the disparaging comment in #8 -- her review actually made the code much better. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Cool, this addresses all of my feedback from IRC!

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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