As of today, destination plugin, table ( https://cgit.drupalcode.org/migrate_plus/tree/src/Plugin/migrate/destina... ), is unable to handle the following things.

Situations:

a.) What if your source id key, tid, is different with your destination table, eg. id key is id
--- This situation results in crash.
b.) destination table has existing records already but are not part of the migration source.
-- This results in wrongly overwrites.
EG. source has id 3, migrate new records to destination.groups.. source id 3 should be migrated as a new record groups.id = 6 because of auto-increment but right now.. it over-writes destination groups.id = 3.

mysql> SELECT id FROM destination.groups;
+----+
| id |
+----+
|  1 |
|  2 |
|  3 |
|  4 |
|  5 |
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chriscalip created an issue. See original summary.

chriscalip’s picture

This gist is an attempt to solve these problems.

https://gist.github.com/chriscalip/b24fc4dbecee1aa48a9ea7b8a87bab5d

C5table.php contains the migrate destination plugin c5table thats able to handle id_mapping and source_id_values and destination_id_values not in equal values.

migrate_plus.migration.group2model.yml showcases use of c5table and its configuration.

   id_fields_map:
      tid: id

This configuration handles source id key tid and its destination table id key id.

Unfortunately this initial attempt is only able to handle one sourceid eg.. migrate_map_{migration_name} only has sourceid1 and no other sourceidN

chriscalip’s picture

Issue summary: View changes
chriscalip’s picture

Issue summary: View changes
chriscalip’s picture

Issue summary: View changes
chriscalip’s picture

Issue summary: View changes
heddn’s picture

Status: Active » Needs work
Issue tags: +Needs tests

The gist attached doesn't extend Table so it is very hard to see what is the problem or the fix. Can we get some tests added here demonstrating the issue?

chriscalip’s picture

@heddn

Right, here's a more coherent example of that destination plugin table is not able to handle difference between source key and destination key.

Here's the migration.
https://gist.github.com/chriscalip/f2efaf73976b2ed42608aa8f3c35dc99

2949271-desc-group-table.png
Shows the destination table group, structure.

2949271-field-sources-termgroup.png
Shows the attributes of the datasource.

2949271-table-plugin-cant-handle-different-keys.png
Migration attempt results in crash.
Trying to insert source tid to destination id
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'tid' in 'where clause': SELECT 1 AS expression

heddn’s picture

This doesn't help me. Can we add a kernel test that demonstrates the issue?

chriscalip’s picture

sourceid1 is tid
destid1 is id

Destination plugin table is doing a check on TID, and is cracking because there is no TID column on destination table.

I have not put in the time and effort of doing Kernel Test for drupal ecosystem... d7 decision to stick with simpletest really burned me; i hesitate to deal with simpletest powered testing.

mhmd’s picture

I have the same issue any guide or help to fix it.

chriscalip’s picture

@mhmd

I had to create a custom destination plugin. The current version is not ready for the rigors of contrib patch; but hey it works.

mikeryan’s picture

Status: Needs work » Postponed (maintainer needs more info)

@chriscalip, can you post the .yml for your table migration?

chriscalip’s picture

@mikeryan

Link to yaml file.
https://gist.github.com/chriscalip/a0509ea657a56ffba1cec43d6fb7cf6e

This migrates node type asset to sql table "assets"
Primary key of nodes is nid. Primary key of sql table assets is id.

Trying to move issue forward. Will provide custom destination plugin if requested.

mikeryan’s picture

Status: Postponed (maintainer needs more info) » Active

Sorry I wasn't clear, I wanted to see the original YAML you were trying to get working with the table destination plugin as it is. We should patch that plugin rather than introduce a new one.

I see the problem that the Table destination import() does $row->getSourceIdValues() - yuck! Destination plugin looking directly at source data is all wrong, this is what the process pipeline is for. It should be getting the *destination* id, which should be mapped like:

process:
  id: tid

That's one big general flaw. Another is handling your specific case, where you don't want to explicitly set the ID, you want to let it be set by auto-increment. This is stretching the scope of the plugin a bit - it was really intended for a full table copy. But anyway, to handle this, it should recognize when the there is no id mapped and in that case do an insert() instead of merge().

So, what we would need here is a patch to the Table plugin fixing these issues (I think they're closely enough related to do in one go).

chriscalip’s picture

Right, gotten used to the maturity of core destination plugins. Handles auto-increment and usable by derive process plugins like migrate_lookup. Besides conditional merge and insert; this plugin also could use ability to deal with multiple source ids.

EG.
example 1: sourceid1 is LANG, sourceid2 is NID and destid1 is id.
example 2: sourceid1 is NID and destid1 is id.

mhmd’s picture

@chriscalip, Thanks for your reply your plugin c5table works fine with me to migrate to custom entity

https://gist.github.com/chriscalip/b24fc4dbecee1aa48a9ea7b8a87bab5d

You saved my time.

webflo’s picture

Status: Active » Needs review
FileSize
1.19 KB

I tried to address the feedback from @mikeryan in #15 and focused on getting rid of $row->getSourceIdValues()

The idFields config of the destination plugin describes the table structure of the destination. I extracted the ids based on this information. It should support direct mapping (full database copy) and rename of columns, since everything can be mapped via the process step in migrate.

It does not support auto-increment values atm, but we are in a better place since it does not rely on the source data anymore.

heddn’s picture

Status: Needs review » Needs work

Any chance for a test? Great work in #18.

webflo’s picture

FileSize
2.09 KB

I tried to make auto_increment values work.

webflo’s picture

FileSize
2.1 KB
+++ b/src/Plugin/migrate/destination/Table.php
@@ -121,12 +126,26 @@ class Table extends DestinationBase implements ContainerFactoryPluginInterface {
+        if (isset($fieldInfo['auto_increment']) && $fieldInfo['auto_increment'] === TRUE && !$row->hasDestinationProperty($field)) {

Its not possible to use 'auto_increment' here. This leads to an invalid sql table definition for the migrate map.

esolitos’s picture

Status: Needs work » Reviewed & tested by the community

Patch from #21 seem to work as expected, no issues encountered (at the least for my use case).

heddn’s picture

Status: Reviewed & tested by the community » Needs work

We refactored the tests for table over in #3006094: Add migrate source for sql table. This should be an easy thing at this point to add a test of this functionality. Back to NW for tests.

mikeryan’s picture

Assigned: Unassigned » mikeryan
mikeryan’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.63 KB
heddn’s picture

Somewhere in the IS we mention this should also support cases where there is already data in the destination table. Can we add that as a test scenario?

mikeryan’s picture

@heddn: Well, the description for that is:

EG. source has id 3, migrate new records to destination.groups.. source id 3 should be migrated as a new record groups.id = 6 because of auto-increment but right now.. it over-writes destination groups.id = 3.

That's actually the expected behavior - if there's an explicit ID, then that's what's written.

heddn’s picture

But what if there is already data in the destination. Do we expect it to overwrite? Is there any way to /not/ overwrite the destination?

mikeryan’s picture

That's a larger question than is relevant here - in the migration ecosystem in general, if a destination plugin is presented with an explicit ID, it is expected to write that ID (overwriting if it already exists, or inserting if it doesn't). Hence this plugin's use of merge(). If there were to be a "use-this-ID-if-it's-not-already-used-but-generate-a-new-one-if-it-is" option, that would be an entirely new feature request...

Remember, the initial issue here was because the table plugin was automatically saving the source ID to the destination ID, which this patch fixes to work like "normal" source plugins - overwrite when there's an explicit ID, and generate an ID when there's not. And the patch already tests both scenarios - MigrateTestBase::testTableUpdate tests that overwriting happens when there's an explicit ID, and MigrateTableIncrementTest::testTableDestination() tests that IDs are generated when there's not.

mikeryan’s picture

Assigned: mikeryan » Unassigned
heddn’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine.

  • heddn committed 92f54a9 on 8.x-5.x authored by mikeryan
    Issue #2949271 by webflo, mikeryan, chriscalip, heddn: Add feature...
heddn’s picture

Status: Reviewed & tested by the community » Fixed
Chris Matthews’s picture

Version: 8.x-4.x-dev » 8.x-5.x-dev
Parent issue: » #3095532: Plan for Migrate Plus 5.x Full Release

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jedihe’s picture

For anybody using 8.x-4.2: a migration where values for one of the fields used for id_fields was transformed via process plugins was failing to rollback for me (migrated records were not removed, as expected).

Having a look at migrate_map_my_migration table, I noticed that the source IDs were used also for the destination columns, which was the reason for rollbacks not working.

Once I applied #25 on top of 8.x-4.2, I was able to get proper destination IDs set in migrate_map_my_migration, which then got the rollback working fine. Also, existing records in the destination table were properly preserved after rolling back.