Method \Drupal\migrate\Plugin\migrate\process\MigrationLookup::transform()
has two different return values:
- If the lookup succeeds it returns the result of
\Drupal\migrate\Plugin\MigrateIdMapInterface::lookupDestinationId()
- If the lookup fails and stubbing is allowed it returns the result of
\Drupal\migrate\Plugin\MigrateDestinationInterface::import()
The first is documented as:
* @return array
* The destination identifier values of the record, or empty on failure.
The second as:
* @return mixed
* The entity ID or an indication of success.
In practice the import() call always returns an array containing the entity id as the array key. While lookupDestinationId returns values numerically indexed.
This leads to the problems described in #2810907: Migrate SQL Map doesn't get array keys for compound keys
That ticket was apparently closed because it has workarounds for 2 situations:
- no stubs are present (you can use keys '0' and '1')
- all referenced content is stubbed (you can use keys 'id' and 'revision_id')
I'm opening this ticket for the case where you have both stubs and real content. You'll get the error 'Array index missing, extraction failed.'
Comment | File | Size | Author |
---|---|---|---|
#37 | 2890690-37.patch | 65.7 KB | yogeshmpawar |
#32 | 2890690-32.patch | 65.9 KB | quietone |
#31 | interdiff.2890690.27-31.txt | 8.72 KB | mikelutz |
#31 | 2890690-31.drupal.MigrateLookup-plugin-has-inconsistent-return-values.patch | 65.95 KB | mikelutz |
Issue fork drupal-2890690
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedAdding related issues.
Comment #3
quietone CreditAttribution: quietone as a volunteer commentedFound another one.
Comment #6
heddnI fully understand the issue here. If we could provide a test of this, I think the fix proposed is a really great suggestion.
Comment #7
heddnAs a work around, can we use callback process plugin and call array_shift/array_pop?
Comment #8
mikelutzI don't think so. At minimum it would trigger a warning, since callback uses call_user_func, which passes parameters by value, and array_shift/pop expect references.
Comment #10
jibranThis is blocking progress on #2911244: Field collections deriver and base migration.
Comment #11
mikelutzI'm curious as to what would break if the plugin simply returned an array with the results returned twice, under both keys. Struggling with a BC way to fix this other than adding a new optional config parameter to select the return keys, and leaving the current behavior alone if it is absent.
Comment #12
mikelutzrebase against new 8.7
Comment #13
mikelutzLet's try this again using the correct keys......
Comment #14
mikelutzAnd one other option with a new configuration value instead.
Comment #15
mikelutzAlright, 13 breaks too much, not surprised, so let's throw 14 out for feedback.
Comment #16
joachim CreditAttribution: joachim as a volunteer commentedThis should be defined as optional, partly for BC, and also because it's easier to simply omit something rather than declare FALSE.
Should also say what it's keyed by if you request associative.
(Can't wait for https://www.drupal.org/project/drupal/issues/2937177 to get in and remove the need for issets() all the time!)
Comment #17
mikelutzIn regards to both points, the issue is that (at least the way I structured this patch) omitting the configuration key is different than setting it to false. Omitting the key retains the current functionality (associative array if a stub is created and indexed if the lookup succeeds), while setting it to false returns an indexed array in either case.
I'm not a fan of this for several reasons, but I'm completely open to a better way to preserve BC while improving the functionality. In a perfect world, the plugin should return an associative array by default, and the extra configuration shouldn't be necessary, but I can't see a way to do that without breaking BC.
Another option would be to have two new configuration bools, force_associtive and force_indexed. This would allow the legacy behavior if both were false, but you have to deal with the error case where someone sets them both true.
You could have a configuration option that takes one of three predefined string keys, like return_array: associative|indexed|legacy. That effectively follows the same logic path as this patch, but perhaps with a slightly better DX. You could set the default to legacy and deprecate it, which basically returns us to this patch, where the param isn't really optional, because omitting it returns the current broken behavior that we want to get away from.
Plus all of this only comes into play if there are more than one destination id anyway, since (in yet another inconsistency), this plugin returns an array if there are multiple keys, and a single value if there is only one. Fortunately, you tend to know what your migrations destination ids are when you call this and admittedly, in the common case of just getting an entity_id back it's nice to be done in your process without going through an extract. But I wonder with new content moderation and workspaces initiatives, if it's not going to be more common to need an entity_id and revision id back.
Finally, we could combine this along with #2891073: MigrationLookup doesn't create stub when there's multiple migrations., #2890844: Migration Lookup plugin does not return multiple values when matched, #2804893: MigrationLookup process plugin not extensible, and possibly support for a lookup by migration tag, which I've also wanted to add, and create a brand new functional migrate_map_lookup plugin that works in a sane way and deprecate migration_lookup. Obviously a fair amount of work to update all the usages and tests in core, and I hate having 2 deprecated migrate_lookup plugins, but it might be worth it to have a consistent return array
I'd love to come up with a long term plan, I'm happy to try to do some work on it if we can figure out where we want to end up.
Comment #18
heddnDiscussed in migrate maintainers meeting with @phenaxproxima, @mikelutz, @maxocub, @quietone and @heddn. We suggested we'd like to use a config option for the process plugin to always return as a associative array and if that option isn't set, to trigger_error.
Comment #19
mikelutzComment #20
mikelutzComment #21
mikelutzWell, lets see how bad this is....
Comment #23
mikelutzThis fixes one oops...
Comment #24
mikelutzI don't know that this is as cut and dried as I thought it would be. I was working on the assumption that all stubs return an associative array, but that isn't true. I'm not even sure that any core destination returns an associative array. most destinations extend off of the Entity destination, which returns [$entity_id]. ERR in contib returns an associative array of id and revision id, but that's the only one I see.
There's definitely a core issue here where the return value of stubbed and lookup values are different, but I'm suddenly less certain of the best way to resolve the discrepancy. I do like the way the migrations look when I switch them over to expecting a keyed array back from this plugin. it adds a little to the migration yml, but I think it becomes readable and more obvious what values you are looking at
At the end of the day, migrate_lookup doesn't get to control what DestinationPluginInterface::import() returns, we almost need to create the stub, and then call lookup destination ids again to guarantee the return values are indistinguishable, which may be what I have to do..
Comment #25
colanLet's see what the testbot says.
Comment #26
mikelutzI've found several things failing since I posted that one. Not much point in running the testbot against it until I get a new version.
Comment #27
mikelutzComment #30
mikelutzA few more fixes..
Comment #31
mikelutzBah, wrong files...
Comment #32
quietone CreditAttribution: quietone as a volunteer commentedNeeded a reroll
Comment #35
esolitosI've been using this patch, only for the change on class
SubProcess
, which seems to work as expected.Very quick review of the other parts.
There's a typo in a few places:
return_associatve
should bereturn_associative
Indentation issue.
Comment #36
yogeshmpawarComment #37
yogeshmpawarRerolled the patch & comments addressed in #35
Comment #39
jibranThe following point about the patch is not a bug but an enhancement so it might seem out of scope for the issue but I'd like to hear some thoughts on this and we can always create a follow-up.
For me, the overall change makes sense here but there is a disconnect between the input and the output.
MigrateIdMapInterface::lookupDestinationId()
allows associate array for IDs and currently there is no way to send the associated values array usingMigrateLookup
plugin toMigrateIdMapInterface::lookupDestinationId()
. If we allowing retruned value to be an associate array then we should also allowMigrateLookup
plugin to pass associate array for IDsMigrateIdMapInterface::lookupDestinationId()
using$this->configuration
or$value
.Comment #40
mikelutzFYI, most of this is getting abstracted/refactored away in #3004927: Create Migration Lookup and Stub services
Comment #45
tvb CreditAttribution: tvb commentedMigration from D7 site with translated nodes and
no_stub: true
:MigrationLookup returns:
Not sure whether this is a same, related or child issue.
A quick fix for this case (but not a solution) is to always return reset($destination_ids) in MigrationLookup.
Comment #50
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedI did a best effort rebase and committed it to a new MR. A lot changed since 8.8 so some things might not be right, but let's see what the test are saying. I tested manually with a custom migration and that seems to be working.
Comment #51
mikelutzWe aren't going to make changes here, closing this in favor of #3246666: Deprecate migrate_lookup and replace with cleaner process plugin
Comment #54
quietone CreditAttribution: quietone commented@DieterHolvoet, thanks for updating the patch but I don't see why a new MR was created after a migrate maintainer set the issue status to "won't fix". Credit stays because of the work in good faith on the initial patch update.
@Rajeshreeputra, this issue has a status of "won't fix". Adding an MR is of no help and causes unnecessary testing. Therefor, credit has been removed per How is credit granted for Drupal core issues.