Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
If you are doing a migration and have an error, then it saves an ID mapping without a destination ID.
When you repeat that row, then the destination receives array(0 => NULL) instead of array(). Personally, I think that's a bad API and I'm not sure what the use case is for supporting that as it leads to bugs like this.
The mentioned method does this:
$old_destination_id_values ? reset($old_destination_id_values) : $this->getEntityId($row);
However, the first part is TRUE, sind the array is not empty, it has a NULL value. That result is that, depending on your migration, it's either silently creating a new entity and ignores the ID you passed in or fails with an error if there is no bundle key in $values.
Seems like a pretty fun way of killing your data, so setting to major.
Proposed resolution
Switch to reset($old_destination_id_values) ?: $this->getEntityId($row), so that it falls back to getting the ID from $row.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 2514212_8.patch | 4.32 KB | chx |
Comments
Comment #1
mikeryanLooks good, just needs a test.
Comment #2
alx_benjamin commentedComment #3
mikeryanStill needs a test.
Comment #4
chx commentedBased on the summary, the test should insert a row in the map table direct via $this->container->get('database')->insert() then it should run a migration for which the source id we saved above and then assert the destination id is correct
Comment #5
tduong commentedTried to work on the test but could not make it fail. test_only = interdiff.
Comment #6
berdirThe following test fails. I had to me two changes. a) set the status to needs update. Apparently that's the only one that's considered in the source. And I had to keep the node.
The reason for that is that my two examples in the issue summary (wrong ID or exception because of missing type) only happen when you do partial migrations without that data (without ID wouldn't work of course, but without type would, if you import additional data into nodes).
In this case however, the way it fails is that it tries to create a new node with the same ID and then explodes on the INSERT query.
We might want to move this to a separate test, maybe also work with a different source than nodes, not sure. Open to suggestions.
Comment #8
chx commentedI really like the test except a little detail: the migration re-run should be in a separate method.
Also, since I am changing it anyways, I am testing node 2 instead of node 1. I have a bad feeling about 1: if you were to cast NULL to 0 and add the default auto increment you'd still get to 1. Yes this is vague, not occuring etc. Still, costs us nothing.
I didn't attach an interdiff as it's much bigger than the patch itself.
Edit: I uploaded a test only to #2672416: test only for 2514212 ; ignore .
Comment #9
berdirThis looks good to me, we need an RTBC from someone who didn't work on the patch I guess.
Comment #10
benjy commentedLooks good to me.
Comment #11
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!