Problem/Motivation

While lookupSourceID uses the source ID properties as keys and the interface prescribes doing the same, getRowBySource keys by "source1" and such.

Proposed resolution

Add aliasing. Easy: remove ->fields('map'); and add $query->addField('map', $idmap_field_name, $source_field_name); to the loop next row.

Attempts were made to add aliasing as proposed but this lead to problems with the use of MySQL reserved words. This was found with the embedded source plugin which uses a key of 'key'. See #8.

Later, in #26, mikeryan pointed out that the use of standard name such as sourceid1 etc. makes it easy for generic tools since they will know what the keys are. Then this issue can be resolved by updating the interface documentation to make this clear. A following migrate meeting agreed to do this. See #27.

So the resolution here is to update the interface documentation, MigrateIdMapInterface

Remaining tasks

Update interface documentation, MigrateIdMapInterface

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx created an issue. See original summary.

phenaproxima’s picture

A first try. I have a feeling this will break the tests.

Status: Needs review » Needs work

The last submitted patch, 2: 2603124-2.patch, failed testing.

The last submitted patch, 2: 2603124-2.patch, failed testing.

The last submitted patch, 2: 2603124-2.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

quietone’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
2.45 KB

Started to reroll this and encountered a problem with MigrateEmbeddedDataTest. That test has source data with a data column named 'key'. Which is a reserved word and MySQL and can't be used as a column alias, which this patch does. Took a look at #2691797: Handle Migrate source IDs that are SQL reserved words and took the suggestion to add a suffix to the field name.

Like phenaproxima said above, this is a first go, and there will be failures.

Status: Needs review » Needs work

The last submitted patch, 8: 2603124-8.patch, failed testing.

quietone’s picture

This should be better.
The idMap property, 'last_imported' has been added to the method idMapDefaults. And the changed expected results are sorted to be in the same order as that which is returned by the modified queries.

quietone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: getRownBySource-2603124-10.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
14.27 KB
3.44 KB

Fixed MigrateRollbackTest.php and MigrateTaxonomyTermStubTest.php.

Status: Needs review » Needs work

The last submitted patch, 13: getRownBySource-2603124-13.patch, failed testing.

quietone’s picture

Oh, got distracted by the children and uploaded a test patch. Ignore patch in 13. Uploading the correct one.

Status: Needs review » Needs work

The last submitted patch, 15: interdiff-10-15.patch, failed testing.

quietone’s picture

FileSize
2.93 KB

And I named the interdiff wrong. Definitely time to stop for the day.

The last submitted patch, 15: getRowBySource-2603124-15.patch, failed testing.

quietone’s picture

Having difficulty testing this with MigrateUpgrade6Test.php. The test fails with 'MySQL server has gone away' error. But, if I reduce the number of migrations being run, it will run and report passing tests on those migrations. Let's see if it will pass everything.

The last submitted patch, 19: getRowBySource-2603124-19.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 19: interdiff-15-19.patch, failed testing.

quietone’s picture

FileSize
5.71 KB

Once again I did the interdiff incorrectly. Sigh.
Here is the correct one.

quietone’s picture

Status: Needs work » Needs review
FileSize
15.39 KB
1.1 KB

Don't know how I missed incrementing the index.

quietone’s picture

Right. Finally a successful reroll of the phenproxima's original patch. Ready for some feedback.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Needs review » Needs work

I keep saying when this comes up in the weekly meetings that I'm not a fan of changing this behavior, I should explain why... To my mind, the use of sourceid1/destid1 etc. as keys in the returned map data is preferable because the consumer then knows exactly what those keys are, which makes such things as general-purpose tools simpler. My preference would be to change the interface docs to reflect the actual behavior instead. That being said, I don't feel strongly about this - if others want to proceed, I won't stand in the way.

But, if this is to go ahead, adding an "_x" suffix to the field names also fails to match the documented interface, and seems really confusing. I would suggest, rather than setting the field names directly via the SQL query, retrieve the data as-is and then rewrite the results to use the actual field names without a suffix.

heddn’s picture

This was discussed in the migrate weekly call. We'd like to update the documentation to reflect the actual functionality. Then there'd be no BC break.

kekkis’s picture

Rerolled against 8.2.x using patch -p1 (getting fuzz), because the metadata, per #26, suggests that is the target.

With git apply the patch simply fails to apply, saying:

Checking patch core/modules/migrate/src/Plugin/migrate/id_map/Sql.php...
Checking patch core/modules/migrate/tests/src/Kernel/MigrateRollbackTest.php...
error: while searching for:
      $term = Term::load($row['id']);
      $this->assertTrue($term);
      $map_row = $term_id_map->getRowBySource(['id' => $row['id']]);
      $this->assertNotNull($map_row['destid1']);
    }

    // Rollback and verify the entities are gone.

error: patch failed: core/modules/migrate/tests/src/Kernel/MigrateRollbackTest.php:125
error: core/modules/migrate/tests/src/Kernel/MigrateRollbackTest.php: patch does not apply
Checking patch core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php...
Hunk #6 succeeded at 613 (offset 117 lines).
Checking patch core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php...
Checking patch core/modules/taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Updated IS and added tags.

heddn’s picture

Issue tags: +Migrate January 2017 Sprint
heddn’s picture

Issue tags: -Migrate January 2017 Sprint +Migrate January 2018 Sprint

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

A suggestion for updating the docs. I'm sure this needs more, what do you suggest?

quietone’s picture

Status: Needs work » Needs review
heddn’s picture

Status: Needs review » Reviewed & tested by the community

Proposed resolution is reasonable and one I support. LGTM.

larowlan’s picture

Version: 8.6.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed e35d086 and pushed to 8.8.x. Thanks!

  • larowlan committed e35d086 on 8.8.x
    Issue #2603124 by quietone: Sql::getRowBySource doesn't adhere to...
larowlan’s picture

Status: Fixed » Closed (fixed)

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