The various kinds of plugins should not talk to each other. MigrateExecutable weaves them together but they don't talk to each other. #2598696: Rollback should not delete uid 1 leaks Sql idMap specific implementation details into the destination "sourceid1" and $id_map['rollback_action']. sourceid1 is not something the idMapInterface guarantees to exist. I am a bit baffled by how the usage here because lookupSourceID will key by the id propery instead of "sourceid1" so I have no idea how that even leaked. That's another bug, whatever leaks sourceid1 from Sql idmap needs to be fixed.
To simplify:
core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php:103: if (isset($id_map['sourceid1'])) {
shouldn't be in DestinationBase.
I do not understand what's happening but such leaks need to be plugged.
If need to get the destination plugin involved then have the MigrateExecutable pass the data much like it does on import.
But I will be honest and say: I do not fully grasp why did destination got involved in the first place. There's no destination plugin overriding setRollbackAction. So... why is this in destination?
Comments
Comment #2
webchickStill don't understand what was architecturally broken about the previous patch, and the issue title/summary here sadly don't help.
Comment #3
chx commentedComment #4
chx commentedComment #7
heddnNeeds an issue summary update.
Comment #8
chx commentedComment #10
mikeryanThe issue summary is still not entirely clear. As far as 'sourceid1' goes, in my mind, sourceid* and destid* are part of the idmap interface - if we were to implement an idmap with a non-SQL store, we would still use those names for the key fields, so consumers of the id map data know what the field names are.
Yes, it is less than ideal that the destination is referencing the ID map directly. The destination does in some instances need to know if it's dealing with a previously-imported entity, so to remove this we would need to have some way to determine this (i.e., having MigrateExecutable figure it out and pass the information in). This is not a priority at this point, particularly with Migrate at beta-experimental stability - downgrading to normal. If someone can address this without breaking BC, patches welcome.
Comment #14
heddnYes, we do need to decouple these things. But the current approach is good enough for D8. If/when we do fix this it will probably through an incubated contrib project. So marking this as won't fix.