Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Considering how to easily provide specific source data for testing (discussion starting at https://www.drupal.org/node/2535458#comment-10175438), an ArraySource that was just like EmptySource except permitting injection of source data (defaulting to the single empty row EmptySource now has) would be useful. It may even be useful in real migration scenarios, for importing data that can be determined at configuration time (as opposed to coming from a dynamic source such as a SQL database.
Comment | File | Size | Author |
---|---|---|---|
#24 | create_new_source-2544690-24.patch | 10.46 KB | mikeryan |
Comments
Comment #1
phenaproximaI'm not yet certain how I feel about using such a source in real migrations, but for testing, I can imagine this being quite useful. +1.
Comment #2
mikeryanI'm thinking "array" isn't right here - actually, the simplest way to specify hard-coded data for testing would be directly in the source schema. E.g.
Thoughts on what the best name for a source plugin that works this way would be? Using 'embedded_data' for the moment, as you can see above...
Comment #3
mikeryanComment #4
mikeryanHere we go - the EmbeddedSourcePlugin lets you put source data directly into the plugin configuration. I've switched MigrateEventsTest to use that instead of defining a custom source plugin just to deliver test data.
Comment #5
mikeryanBlocks #2522012: Remove broken idlist handling, replace with more robust exception handling , which should use this for tests. Since that is Major and Migrate-critical, this should be as well.
Comment #6
benjy CreditAttribution: benjy commentedDon't mind the name happy to go with that. The only other suggestion i had was something just like ConfigurableTestSource
Can we also remove EmptySource now?
Interesting test, had me confused at first, looked like something from the ban module.
Comment #7
mikeryanInteresting question - I ran across at least one non-test usage of migrate's EmptySource that I didn't quite understand (which naturally I can't find at the moment). migrate_drupal's EmptySource extends that one as md_empty, and is easier to find usages for. I'm not sure it's that simple, but I could look at replacing the empty stuff (and extending the md_empty version from EmbeddedDataSource) tomorrow.
Yes, borrowing the ban module's destination to test here, but I'm having second thoughts about that, it would be better to not have a dependency on an unrelated module.
Thanks!
Comment #8
mikeryanSo, with this all that's left of migrate_events_test module is the DummyDestination, which would also be useful here instead of borrowing the ban module. Perhaps we should add the destination-side equivalent of EmbeddedDataSource, a simple destination that just copies the incoming data somewhere we can easily validate its arrival.
Comment #9
phenaproximaLet's create a destination plugin that simply slaps $row->getSource() into somewhere temporary (cache, state, key-value storage, /dev/null, temp file...?) Then we can have a useful source and destination pair for testing purposes.
But maybe we should do that in a follow-up issue to prevent this one from getting too big and confusing. MigrateEmbeddedDataTest can use the null destination for now; in fact, I wonder why we're even bothering to test the destination. EmbeddedData is a source plugin, so why aren't we testing that functionality only?
A few other thoughts:
I was thinking that this should be a sequence of mappings so that each data row is an array. As far as I can tell, this defines each row as a string, but that's not a very accurate approximation of how normal Migrate sources work. (If I'm misinterpreting the config schema here, disregard my blathering.)
Can we call the config key
data
orrows
or something, rather thanembedded_data
? I find that name confusing. (It's OK for the plugin ID, but it's a strange config key.) If we rename it, let's also change the $embeddedData property to match.Can we use gettype() here to get the field type? It returns 'string' for strings and 'integer' for integers, which matches what other sources return. Alternatively, it might be handy to allow the plugin config to specify the ID keys directly, and have this auto-detection code as a fallback. Something like:
Comment #10
mikeryanI'll reroll the test to just grab the source plugin and verify it delivers the expected data. Let's deal with a new destination in a separate issue.
That's defining each row as a sequence of strings.
OK, I'll go with rows.
gettype('1') == 'string'
I had that pegged for a follow-up once we had a use case, but I could do that now.
Thanks!
Comment #11
mikeryanOK, here it is with a much nicer test. Interdiff might be too much to be useful, but here it is anyway...
Comment #12
mikeryanWith requested comment change.
Comment #13
phenaproximaLooks good to me.
Comment #14
alexpottIt kind of scares me that embedded data is an regular migration source plugin - do we have use-cases outside of tests?
Comment #15
phenaproximaI do not believe we have a use case outside of tests -- yet -- but there are a few places it might be useful (it's more straightforward, for instance, than using the EmptySource and a bunch of constants, which IIRC is done by a few real migrations).
As for it being a regular source plugin, I'm not sure what else it would be? We already have a "null" destination plugin which sends incoming rows to the Big DIMM in the Sky, and that is a "regular" destination plugin which is a) part of Migrate API and b) so far unused outside of testing. What are you envisioning?
Comment #16
mikeryanI think this will be useful outside of tests - I've seen plenty of cases where some small amount of fixed (for a given project) data is imported using migration, so that it can be referenced by other migrations (in D8 parlance, with the 'migration' process plugin). A "Channels" vocabulary is one such use case I've seen multiple times. With this plugin, it'd be simple to have a UI tool to enter a few rows of data that can be baked into a migration configuration.
Comment #17
mikeryanE.g., channels.yml:
Comment #18
phenaproximaGood call @mikeryan. It's quite possible that you might want, in some cases, to define arbitrary source data in a migration. It's an edge case, but combined with the usefulness for testing purposes I see no reason this should not go ahead.
Comment #19
webchickBack to alexpott.
Comment #20
phenaproximaWith the advent of the event system, the idlist functionality (#2522012: Remove broken idlist handling, replace with more robust exception handling ) is something we'll soon be able to implement in contrib, which is where it rightfully belongs. So I no longer think this qualifies as Migrate-critical.
Comment #21
phenaproximaDe-escalating.
Comment #23
phenaproximaComment #24
mikeryanThe test for the interruption patch was leveraging the source plugin provided for the events test (and modified it to provide more data), which rather confused things. So, with this version we're have both the events and interruption tests leveraging this new source plugin rather than having to define a custom one to provide them with test data.
Note that at the moment #2522012: Remove broken idlist handling, replace with more robust exception handling is creating its own source plugin to provide it with test data - if we can get this patch it, then I'll update that to make use of the embedded_data source plugin. If the idlist patch gets in first, we'll want to update this patch to remove the custom source plugin in favor of this one.
Comment #25
phenaproximaLook good to me, this does.
Comment #26
alexpottThanks for answering the question. I can see the use-case and think I've done something similar myself. Committed 427d080 and pushed to 8.0.x. Thanks!