Problem/Motivation

For some use-cases it is necessary to have different migrations that depend on each other to use different database targets.

The use-case I ran into was a D6 multi-site installation which shared a {users} table. Thus I had one database target for e.g. the node migration and a different target for the user migration.

Proposed resolution

Allow to specify the database target via a target key in the source plugin configuration of a migration.

Remaining tasks

User interface changes

None.

API changes

None (merely an API addition).

CommentFileSizeAuthor
#20 2336199-20.patch10.17 KBultimike
#20 interdiff.txt930 bytesultimike
#14 2336199-14.patch9.56 KBbenjy
#11 interdiff.txt1.01 KBbenjy
#11 2336199-11.patch9.56 KBbenjy
#9 interdiff.txt1.95 KBbenjy
#9 2336199-9.patch9.54 KBbenjy
#8 2336199-8-migrate-database-target.patch7.62 KBtstoeckler
#8 2336199-3-8-interdiff.txt7.17 KBtstoeckler
#3 2336199-3-migrate-database-target.patch758 byteststoeckler
#1 2336199-1-migrate-database-target.patch743 byteststoeckler

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review
StatusFileSize
new 2336199-1-migrate-database-target.patch743 bytes

I had something like this.

Thoughts?

Status: Needs review » Needs work

The last submitted patch, 1: 2336199-1-migrate-database-target.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2336199-3-migrate-database-target.patch758 bytes

Pretty please, Mrs. bot?!

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

Yeah that looks good to me.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Just for the record: @dawehner showed me that you can in fact specify the prefix part of the database info as a per-table array, which would allow to solve the use-case described in the issue summary in the database layer instead of the migration system.

It still makes sense to make this configurable, though, in my opinion. When sharing tables they might in fact reside in a different database alltogether, in which case this would be needed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Needs documentation somewhere - and unit tests?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

I sort of agree on both points in theory, but neither do I know where to document things like this nor how to write proper tests.

For the documentation: This is really a feature of SqlBase, but if we were to document it in that class I'm not sure how helpful that would be. I don't think many people look into that class. On the other hand this seems a little bit too specific for the general @migrate documentation overview.

Regarding tests: Since we no longer have a global $databases this should be possible in theory. So thinking out loud here how a test might look:
One would have to use $this->container->get('database')->addConnectionInfo() in the test to specify both a 'default' migrate database target and a 'something' target. Then one would need to instantiate a source plugin (maybe an existing one, or preferably a dedicated testing one) once with a 'target' configuration and once without. I haven't tried it but if Migrate utilizes the regular plugin factory mechanism that shouldn't be too hard. And then one would have to call $source_plugin->getDatabase() on the two different plugins and verify that the right one is returned.

Having thought about that that actually seems fairly doable, will take a stab at it. Hopefully later today, but maybe next week, so feel free to beat me to it!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2336199-3-8-interdiff.txt7.17 KB
new 2336199-8-migrate-database-target.patch7.62 KB 
e1d0b6e DEV-2336199: Add config schema for the 'target' key
bfedc98 DEV-2336199: Document the 'target' setting of SqlBase.

I started writing a test for this, but that's not as easy as I thought, because you cannot actually use the fake database backend with Database::getConnection(). Thus, we would actually need a database connection for running the test, which would rule out PhpUnit. Now we could write a KernelTestBase and somehow muck with the test database connections and re-use those or add an additional migrate prefix or whatever, but that feels fairly terrible in terms of hackiness, TBH. Not sure that's worth it for such a trivial test.

benjy’s picture

benjy commented
StatusFileSize
new 2336199-9.patch9.54 KB
new interdiff.txt1.95 KB

Wrote a web test.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Awesome, @benjy! I never would have thought of that. Very nice.

Setting this back to RTBC. I hope this doesn't count as me RTBCing a patch I worked on myself, because it was already RTBC before, and the remaining concerns have now been adressed.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2336199-11.patch9.56 KB
new interdiff.txt1.01 KB

Setting to RTBC as per #10 since I think @tstoeckler forgot :)

Patch attached just has some super minor doc changes and combines the getDatabase() call onto one line.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Oh god, yes that was rather stupid. Thanks again (!!) @benjy :-)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2336199-14.patch9.56 KB

Straight re-roll.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Status: Needs review » Reviewed & tested by the community

That's a pretty elegant test. Looks good to me.

-mike

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2336199-14.patch, failed testing.

Status: Needs work » Needs review

benjy queued 14: 2336199-14.patch for re-testing.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
migrate.source.d6_node:
  type: migrate_source

migrate.source.d6_node_revision:
  type: migrate_source

migrate.source.d6_user:
  type: migrate_source

These seem to have been left behind.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new interdiff.txt930 bytes
new 2336199-20.patch10.17 KB

Updated patch and interdiff attached.

-mike

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

This looks good, it seems that migrate_source is not longer used anywhere but it should probably still stay since there will be non sql sources in the future.

╰─➤  ls **/migrate*.yml | xargs grep -i migrate_source
core/modules/migrate/config/schema/migrate.data_types.schema.yml:migrate_source:
core/modules/migrate/config/schema/migrate.data_types.schema.yml:migrate_source_sql:
core/modules/migrate/config/schema/migrate.data_types.schema.yml:  type: migrate_source
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
core/modules/migrate/config/schema/migrate.source.schema.yml:  type: migrate_source_sql
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Yes, I think that makes sense. RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed afe174d and pushed to 8.0.x. Thanks!

  • alexpott committed afe174d on 8.0.x 
    Issue #2336199 by benjy, tstoeckler, ultimike: Added Allow to specify...

Status: Fixed » Closed (fixed)

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