Problem/Motivation

All of the process plugins in Migrate Conditions are set to handle_multiples in the annotation.

This is so that the developer has the "widest view" possible of the pipeline value. If the pipeline value is a scaler, this doesn't affect anything. If the pipeline value is an array, one can easily evaluate conditions on the whole array (like contains or empty) or use the "Logical conditions" (and and or) or the "Array-handling conditions" (all_elements and has_element) to drill down into the source array as needed.

I think that's all fine.

However, the process plugins all rely on the base class declaration of the multiple() method, which determines how the NEXT process plugin will interpret the structure of the pipeline value. This means multiple is always set to FALSE.

This is probably not the best way to do things.

Proposed resolution

evaluate_condition: This always returns a boolean, so this one does not need to change, as multiple should always return false.
filter_on_condition: This should always return TRUE for multiple. We always have an array in and we always have an array out.
skip_on_condition: This should set multiple to TRUE if the input is an array and FALSE if the input is a scalar. That's what would happen if the skip wasn't there, so we should maintain that, right?
first_meeting_condition: This should set multiple to TRUE if the output is an array and FALSE if the output is a scalar. That would make it more like a fancy get, which I think is what we want.
if_condition: Hmmm....

Remaining tasks

Decide if this is ok, especially since I was stupid and made a beta release already.

User interface changes

API changes

multiple handling will be better

Data model changes

CommentFileSizeAuthor
#5 migrate-conditions--multiples--3293151--5.patch28.48 KBdanflanagan8
#5 migrate-conditions--multiples--3293151--5-FAIL.patch22.59 KBdanflanagan8
#2 migrate_conditions-multiple-3293151-2.patch5.29 KBdanflanagan8

Comments

danflanagan8 created an issue. See original summary.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Active » Needs review
StatusFileSize
new migrate_conditions-multiple-3293151-2.patch5.29 KB

Here's a patch with my proposed changes. It doesn't cause any tests to fail, which makes me more comfortable with the possibility of committing this to a beta. This doesn't change any intended (i.e. tested) behaviors.

codebymikey’s picture

codebymikey commented

I'm not sure what the potential side effects would be for existing migrations making use of these plugins.

I think an easy way to keep the old behaviour without any side effects on the 1.x release would be to let the developer specify what the expected cardinality is with an optional multiple configuration property - see migmag and migrate_devel.

Either that or we could save this for a major release or determine if a minor release is sufficient by updating the tests to include behaviour when other (single/multiple) process plugins are also used with it, and the expected behaviour when working with simple nested array value sources like:

source:
  plugin: embedded_data
  data_rows:
    -
      id: my_id
      field_to_test_with:
        - { value: [1,2], id: 1, ids: [1,1,en] }
        - { value: [1,2], id: 2, ids: [2,2,fr] }
  ids:
    id:
      type: string

I personally don't make use of these plugins with other process plugins, so I don't think this should affect me, however I do think backwards incompatible changes should typically result in a major release.

One thing to note though is that if we do go the multiple configuration route, then it'll probably have to stay in as an API even if the default cardinality changes in a future release.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Thanks for chiming in, @codebymikey! It's really helpful to get some community input here.

I think you're correct that our two options are:

1. Do this is a major release
2. Add a multiple configuration option like migmag

I'm leaning toward #1, but I'm in no rush. I'll sleep on it for awhile.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
StatusFileSize
new migrate-conditions--multiples--3293151--5-FAIL.patch22.59 KB
new migrate-conditions--multiples--3293151--5.patch28.48 KB

Here's a patch that adds a number of test cases intended to elucidate the current behavior with multiples and the proposed behavior with multiples. The second patch is the test patch with the proposed changes from #2 added.

The last submitted patch, 5: migrate-conditions--multiples--3293151--5-FAIL.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Version: 1.0.x-dev » 2.0.x-dev
Assigned: danflanagan8 » Unassigned
Status: Needs review » Fixed

I ended up deciding to add a 2.0.x branch where this work will go. Thanks for the input, @codebymikey!

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Status: Fixed » Closed (fixed)

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