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
Comment | File | Size | Author |
---|---|---|---|
#36 | 2603124-36.patch | 746 bytes | quietone |
#28 | interdiff-2603124-23-28.txt | 5.28 KB | kekkis |
#28 | getRowBySource-2603124-28.patch | 15.38 KB | kekkis |
#23 | interdiff-19-23.txt | 1.1 KB | quietone |
#23 | getRowBySource-2603124-23.patch | 15.39 KB | quietone |
Comments
Comment #2
phenaproximaA first try. I have a feeling this will break the tests.
Comment #7
mikeryanComment #8
quietone CreditAttribution: quietone as a volunteer commentedStarted 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.
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedThis 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.
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedComment #13
quietone CreditAttribution: quietone as a volunteer commentedFixed MigrateRollbackTest.php and MigrateTaxonomyTermStubTest.php.
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedOh, got distracted by the children and uploaded a test patch. Ignore patch in 13. Uploading the correct one.
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedAnd I named the interdiff wrong. Definitely time to stop for the day.
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedHaving 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.
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedOnce again I did the interdiff incorrectly. Sigh.
Here is the correct one.
Comment #23
quietone CreditAttribution: quietone as a volunteer commentedDon't know how I missed incrementing the index.
Comment #24
quietone CreditAttribution: quietone as a volunteer commentedRight. Finally a successful reroll of the phenproxima's original patch. Ready for some feedback.
Comment #26
mikeryanI 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.
Comment #27
heddnThis 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.
Comment #28
kekkisRerolled 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:Comment #31
quietone CreditAttribution: quietone as a volunteer commentedUpdated IS and added tags.
Comment #32
heddnComment #33
heddnComment #36
quietone CreditAttribution: quietone as a volunteer commentedA suggestion for updating the docs. I'm sure this needs more, what do you suggest?
Comment #37
quietone CreditAttribution: quietone as a volunteer commentedComment #38
heddnProposed resolution is reasonable and one I support. LGTM.
Comment #39
larowlanCommitted e35d086 and pushed to 8.8.x. Thanks!
Comment #41
larowlan