Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
In #2746541: Migrate D6 and D7 node revision translations to D8, the entity-translations-revision impedance mismatch between D8 and prior versions was addressed. Impressive care was taken to not break BC in any way.
But a different class of use cases, migrating D8 entity source to custom destination, broke.
Steps to reproduce
- Use D8.8
- Have a migration
- with arbitrary destination
- and a "entity:*" source
- Upgrade to D8.9
On drush migrate:import ..., we get something like
In Connection.php line 701:
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'sourceid3' in 'fiel
d list': INSERT INTO {migrate_map_omm_collmex_users} (source_ids_hash, sour
ceid1, sourceid2, sourceid3, source_row_status, rollback_action, hash) VALU
ES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placehol
der_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_place
holder_5, :db_insert_placeholder_6); Array
(
[:db_insert_placeholder_0] => ccbecd3359774c9adf6a0dcf313a1662064ed9742
9da3b5ee5fd85686ce64e6d
[:db_insert_placeholder_1] => 0
[:db_insert_placeholder_2] => 1
[:db_insert_placeholder_3] => de
[:db_insert_placeholder_4] => 2
[:db_insert_placeholder_5] => 0
[:db_insert_placeholder_6] =>
)
In Statement.php line 59:
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'sourceid3' in 'fiel
d list'
(Note: Setting $settings['migrate_node_migrate_type_classic'] = TRUE; like indicated in the RN does not help so setting major.)
Proposed resolution
TBD
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3184627
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:
changes
Comments
Comment #2
geek-merlinComment #3
geek-merlinOK, patch flying in to issue fork, let's see what the bot says.
Comment #5
geek-merlinOK it looks like my initial assumption is vaild and this hunk of the D8 ContentEntity Souce plugin was changed along the lines of D6 and D7 plugins, but is used nowhere. It adds revision key to getIds() but does NOT return revisions (so an entity update with a new revision would create a new target object, so this is wrong). I removed the test for that errroneous hunk too.
I added a test that spots the BC break.
As source revisions might be useful as option, i opened #3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier as a followup.
Comment #6
benjifisherUnless the current behavior is completely broken, I do not think we can just remove the lines that set
$ids[$revision_key]. That is how it works in Drupal 9.0 and 9.1, and we cannot break that in 9.2.It would be great to have more explicit steps to reproduce than "Have a migration". As we say in the #migration Slack channel, Show Me The YAML (SMTY)! Once we have that, we should be able to add a failing test, which will prove that there is actually a problem here.
If I understand correctly, the only way to reproduce this bug is by starting with a migration on a site running Drupal 8.8; run the migration (or maybe
drush migrate:statusis enough) so that the map table is created; and then upgrade Drupal. Perhaps a better fix would be to add an update function. We have done that in a similar situation in a contrib module, maybecommerce_migrate.Comment #7
quietone CreditAttribution: quietone as a volunteer commentedIn #2746541: Migrate D6 and D7 node revision translations to D8 \Drupal\migrate_drupal\Plugin\migrate\source\ContentEntity::getIds was modified to add the vid for revisionable entities but no hook update was done to alter existing migrate map tables. That should be easy to fix but I am not sure how to right a test for it. I will have to look for an existing update test.
Comment #8
geek-merlinThanks @benjifisher&@quietone for chiming in here!
Re-reading my IS, there's missing a lot that was "self-evident" for me at that moment, but now not even to me anymore:
We're using this on a sustainable food cooperative where members can choose and update their monthly payment and deposit.
(Yes it is monolingual.)
So whenever a user updates their data, it will be synced to an external payment helper service:
https://gitlab.com/geeks4change/modules/omm_collmex/-/blob/master/migrat...
When things broke, i also thought "hook_db_update_N". But only for 5 seconds.
As this is a user migration, not user-revision, we need an updated user to be mapped to the same destination.
In other words, this is the test:
- create a user
- run the migration, expect "Created 1 updated 0 ignored 0 users"
- run the migration, expect "Created 0 updated 0 ignored 1 users"
- update the user
- run the migration, expect "Created 0 updated 1 ignored 0 users"
I'm quite confident that this test will break with current code.
In old D8.8 code the ID is "en:1", so after a user update "en:1" gets updated.
(That's the point of the mapping table in the first place.)
In current code, the ID is "en:1:1", and after user update, the ID is "en:1:2" (a new revision).
So migrate thinks this is a new object, creates it (and if configured, deletes the first).
BUT otoh, testing that "IDs=id+langcode" proves the same. This is in current MR.
Please also note my MR in #3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier that adds revision_id to IDs ONLY if revisions are queried. I think this is the right thing to do but needs maintainer decision if we include it here, do it as followup or not at all.
Does this clarify?
Comment #9
benjifisherThanks for pointing out the use case of a recurring migration. Not all migrations are "one and done".
Thanks also for the link to your repository. That satisfies the standard SMTY request. We should be able to use a simplified version as the basis of a failing test.
To be clear, you have now identified two problems caused by the unintended BC break:
content_entitysource plugin creates a row for each revision, and there is no way to get a row for each entity.Please clarify:
When you write "en:1", do you mean that you have two columns in the mapping table (sourceid1 and sourceid2, corresponding to langcode and uid) with values "en" and "1"? I hope you mean that and not a single column where the values are concatenated.
Assuming that you do mean two columns, I think that adding an update function would work to fix (1) but not (2).
It seems to me that the proposed resolution here would trade one BC break for another. That is not satisfactory. I think we should go straight to #3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier instead of postponing that issue on this one. That seems like the only way to avoid BC problems.
If that is acceptable, then we can close this issue and continue the discussion on #3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier.
Comment #10
geek-merlin> When you write "en:1", do you mean that you have two columns in the mapping table (sourceid1 and sourceid2, corresponding to langcode and uid) with values "en" and "1"? I hope you mean that and not a single column where the values are concatenated.
Yup!
> If that is acceptable, then we can close this issue and continue the discussion on #3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier
Yes i prefer that too. Great to rock that with you.
Comment #11
geek-merlinOK merged IS into #3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier
Comment #12
geek-merlin@benjifisher: The other issue is NR now!
#3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier