\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
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff_7_14.patch | 529 bytes | ruloweb |
#14 | migrate_sql_map_doesn_t-2810907-14.patch | 1.27 KB | ruloweb |
#7 | migrate_sql_map_doesn_t-2810907-7.patch | 1.27 KB | heddn |
#5 | interdiff_4-5.txt | 514 bytes | heddn |
Comments
Comment #2
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedI 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.
Comment #3
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedComment #4
heddnHere'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.
Comment #5
heddnmikeryan had a suggestion in irc.
\PDO::FETCH_BOTH
should do the trick. Based on that, removing the BC tag.Comment #7
heddnInterdiff in #5 still works. But here's a valid patch.
Comment #10
heddnLots 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]
Comment #11
heddnComment #12
heddnTagging 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.
Comment #13
ruloweb CreditAttribution: ruloweb at Media.Monks commentedPatch #7 works ok for me, this is my migration config:
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:
and it works pretty ok.
Comment #14
ruloweb CreditAttribution: ruloweb at Media.Monks commentedWell, 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:
not sure what is the idea behind $scalar, but if I change FETCH_BOTH for FETCH_ASSOC it works.
Comment #15
heddnThis 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.
Comment #16
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedNoting 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.
Comment #17
ruloweb CreditAttribution: ruloweb at Media.Monks commentedMaybe we can leave FETCH_BOTH if the issue related to $scalar is solved #2767643: Scalar to array migration returns NULL
Comment #18
TrevorBradley CreditAttribution: TrevorBradley commentedI'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:
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):
Whereas Patch 14 removes the extra array values and is a lot cleaner:
(Both of these are simple explode > migration process plugin chains, without the iterator).
Comment #19
TrevorBradley CreditAttribution: TrevorBradley commentedA 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.
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.
Comment #20
heddnHere'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.
Comment #21
heddnClosing this down. A viable solution is available for this use case.