\Drupal\migrate\Plugin\migrate\id_map\Sql (correctly) supports save and lookup by compound row identifiers, ie ['nid'=> 123, 'vid'=>456] . But lookupDestinationIds() returns an array of identifier values without their keys, ie [123, 456].

Since the values are meaningless without their keys, and since the equivalent lookupSourceId() method returns a keyed array, we should make lookupDestinationIds() return a keyed array too.

Without adding the key, it forces people to do unholy things like the following to re-add the proper destination index back in. What should be 3 or 4 lines in yaml turns into a page. And yes, every line of the following is needed without this getting fixed.

  field_paragraphs:
    plugin: iterator
    source: foo_sources
    process:
      target_id:
        -
          plugin: migration
          source: source_key
          migration: migrate_foo_paragraphs
          no_stub: true
        -
          plugin: extract
          index:
            - 0
      target_revision_id:
        -
          plugin: migration
          source: source_key
          migration: migrate_foo_paragraphs
          no_stub: true
        -
          plugin: extract
          index:
            - 1
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ohthehugemanatee created an issue. See original summary.

ohthehugemanatee’s picture

I feel like we need a policy decision here.

Fixing this introduces either a BC break, or a new method ->lookupDestinationIdsWithKeys() that breaks consistency with the rest of the API in the interface. Looks like the only place this is used is in migrate module, if tha tmakes any difference.

ohthehugemanatee’s picture

Issue tags: +Migrate BC break
heddn’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.25 KB

Here's a patch that kicks this off. This will break BC. See #2644634-11: Missing Migration Destination in 8.* as an example. However, I don't see any way around fixing the DX except if we break BC.

heddn’s picture

mikeryan had a suggestion in irc. \PDO::FETCH_BOTH should do the trick. Based on that, removing the BC tag.

Status: Needs review » Needs work

The last submitted patch, 5: migrate_sql_map_doesn_t-2810907-5.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
1.27 KB

Interdiff in #5 still works. But here's a valid patch.

The last submitted patch, 4: migrate_sql_map_doesn_t-2810907-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: migrate_sql_map_doesn_t-2810907-7.patch, failed testing.

heddn’s picture

Lots of errors, the handfull I looked at where because we are doing a *lot* of array comparisons on destination ids. And they don't match any more. Because the destination surfaces twice, instead of just in the numeric indexed results. Is that a bad thing? I don't know. I'd appreciate feedback from others before I put in the effort to fix 100 failing tests. Below is a sample of one of the many failures. The actual is kinda what I'd expect and the work of fixing 100+ tests would be to adjust all the expectations.

Expected: [0 => 101]
Actual: [0 => 101, 'nid' => 101]

heddn’s picture

Issue summary: View changes
heddn’s picture

Tagging as DX (and marking as contrib blocker). Contrib blocker is marked because I want to signify this doesn't effect core, only contrib. All core field types do not support a composite relationship. It isn't a hard contrib blocker, but it sure makes life very painful for ERR.

ruloweb’s picture

Patch #7 works ok for me, this is my migration config:

  field_case_description:
    plugin: iterator
    source: case_descriptions
    process:
      target_id:
        -
          plugin: migration
          source:
            - exhibit_id
            - position
          migration: spin_casedescription
        -
          plugin: extract
          index:
            - id
      target_revision_id:
        -
          plugin: migration
          source:
            - exhibit_id
            - position
          migration: spin_casedescription
        -
          plugin: extract
          index:
            - revision_id

My source is a composite value, but the key updates here are that stub is not disabled and the index on extract plugin can be keys now, I think that numeric Ids like the original example would work too, because it returns [0, id, 1, revision_id].

I'm able to create stubs for common field references too (file fields). Im working with an entity/paragraph sctructure like:

- entity
    - Text field 
    - File field multiple
    - Paragraph field multiple
      - Text field
      - Text field

and it works pretty ok.

ruloweb’s picture

Well, actually some references wont work with #7, because in

Drupal\migrate\Plugin\migrate\process\Migration:transform();

$value is scalar (just the source ID), but $destination_ids is an array [0, id] and this condition wont pass:

    if ($destination_ids) {
      if ($scalar) {
        if (count($destination_ids) == 1) {
          return reset($destination_ids);
        }
      }
      else {
        return $destination_ids;
      }
    }
  }

not sure what is the idea behind $scalar, but if I change FETCH_BOTH for FETCH_ASSOC it works.

heddn’s picture

This was discussed at the weekly migrate meeting and decided that we think we can do the work without affecting BC. However, it was decided to reach out to alexpott and see if there were any concerns with the large number of test changes that would get introduced with this change.

ohthehugemanatee’s picture

Noting for clarify #12 - it's correct that this doesn't affect d2d migrations in core. But compound keys are a migrate (core) functionality, which currently breaks the Migrate (core) process plugin.

ruloweb’s picture

Maybe we can leave FETCH_BOTH if the issue related to $scalar is solved #2767643: Scalar to array migration returns NULL

TrevorBradley’s picture

I'd like to make case for patch #14.

See https://www.drupal.org/node/2825565#comment-11772010 for reference.

I'm attempting to import multiple paragraphs into a node. Without having the destination keys returned, the migration is very difficult, requiring a custom plugin to rewrap the data appropriately, followed by a very complex iterator process. Patch #14 simplifies this procedure significantly, reducing the problem to:

uuid: 88b8d4b8-0887-4dbf-884e-1d2483c3f2c0
id: import_content_test_1
label: Import Content Test 1

source:
  plugin: csv
  path: 'private://import_csv/content_test_1.csv'
  header_row_count: 1
  keys:
    - id
process:
  title: title
  body: body
  field_paragraph_test_1:
    -
      plugin: explode
      source: paragraph_test_1
      delimiter: ,
    -
      plugin: migration
      migration: import_paragraph_test_1
      no_stub: true
    -
      plugin: iterator
      process:
        target_id: id
        target_revision_id: revision_id
  type:
    plugin: default_value
    default_value: content_test_1
destination:
  plugin: entity:node

It's not perfect - My final iterator plugin is required because paragraph's keys of id and revision_id don't match up with the Paragraph reference keys of target_id and target_revision_id, but it's significantly easier than having to reference them by number. I suspect this is an Entity Reference Revisions problem though, making the choice in entity_reference_revisions.schema.yml to not match the target field names. It's not a huge deal.

With patch 7, the imported data is much uglier (but would still work):

    [destination:protected] => Array
        (
            [title] => Title 2
            [body] => Body 2
            [field_paragraph_test_1] => Array
                (
                    [0] => Array
                        (
                            [id] => 78
                            [0] => 78
                            [revision_id] => 80
                            [1] => 80
                        )
 
                    [1] => Array
                        (
                            [id] => 79
                            [0] => 79
                            [revision_id] => 81
                            [1] => 81
                        )
 
                )
 
            [type] => content_test_1
        )

Whereas Patch 14 removes the extra array values and is a lot cleaner:

    [rawDestination:protected] => Array
        (
            [title] => Title 2
            [body] => Body 2
            [field_paragraph_test_1] => Array
                (
                    [0] => Array
                        (
                            [id] => 82
                            [revision_id] => 84
                        )
 
                    [1] => Array
                        (
                            [id] => 83
                            [revision_id] => 85
                        )
 
                )
 
            [type] => content_test_1
        )

(Both of these are simple explode > migration process plugin chains, without the iterator).

TrevorBradley’s picture

A follow-up from #2825565: Migrate Process: Importing Multiple Paragraphs:

There's a much cleaner way to run iterator for mapping arrays than the original post, without using indexed keys.

  field_paragraph:
    -
      plugin: migration
      source: foo_sources
      migration: migrate_foo_paragraphs
      no_stub: true
    -
      plugin: iterator
      process:
        target_id: '0'
        target_revision_id: '1'

Note that this requires one of my patches from #2767643: Scalar to array migration returns NULL , so that both '0' (target_id) and '1' (target_revision_id) are returned.

Maybe this isn't quite as horrible as it looks.

heddn’s picture

Status: Needs work » Postponed

Here's why '0' is necessary, look at 'get' process plugin on line 35. It treats them as different. Next question, does the yml parsing engine in drupal, auto-correct '0' => 0 on export after importing it as '0'?

I'm going to a wait a couple days and close this issue out as won't fix. I think we've learned enough about paragraphs migrations that it won't be necessary.

heddn’s picture

Status: Postponed » Closed (won't fix)

Closing this down. A viable solution is available for this use case.