Problem/Motivation
In #3106531: Notify in Status Report that per-table database prefixes are no longer supported, and will throw errors in Drupal 10.0 we prepared the ground for removing per-table prefixing and the 'prefix' connection key in its array form. In the meantime, an @internal 'extra_prefix' connection option is still in use for temporary tables and migration testing.
Only in 2 migration testing methods the 'extra_prefix' connection option is still used: Drupal\Tests\migrate\Kernel\MigrateTestBase::createMigrationConnection() and Drupal\Tests\migrate\Kernel\Plugin\MigrationPluginListTest::testGetDefinitions(). This needs to be removed.
Proposed resolution
Remove the usage of the 'extra_prefix' connection option, and replace it with a direct method in the database API to attach additional databases to the active connection.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3124674
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3124674-add-functionality-to
changes, plain diff MR !1217
Comments
Comment #2
neslee canil pintoComment #3
mondrakePostponed on #3106531: Notify in Status Report that per-table database prefixes are no longer supported, and will throw errors in Drupal 10.0.
Comment #4
mondrake@Neslee Canil Pinto your patch is removing entire test or methods, which is not what we are trying to achieve here. What we are trying to achieve is to get rid of
and replace with something else to be still determined.
However, it looks like at least one of the two cases is actually not relevant for the test, at all, in
Drupal\Tests\migrate\Kernel\Plugin\MigrationPluginListTest.So maybe that one yes, can be just removed.
Comment #5
mondrakeLet's see this, as a first step to separate attaching the additional database from the constructor into a method of its own.
Comment #6
mondrakeGetting a bit more shape, with deprecation of
::setPrefix. It's replaced by a::addPrefixmethod that only works with scalar values - no longer need to pass an entire array, that would be an exception to be managed via::attachSchema.Stopping here now, until parent is committed.
Comment #9
mondrakeChanged to MR workflow; working on this.
Comment #11
mondrakeComment #12
mondrakeThe MR contains some code taken from #3200743: Decouple identifier management from database Connection, introduce an IdentifierHandler class.
Comment #13
mondrakeComment #14
mondrakeChanging issue component to 'database system' since the patch is adding API to the database layer.
Comment #15
daffie commentedStarted 2 threads on the MR.
Comment #16
mondrakeComment #17
daffie commentedReplied to the threads on the MR.
Comment #18
mondrakeThanks for reviews, @daffie, made changes and commented on the MR.
Comment #19
daffie commentedIt all looks good to me.
The new method is marked as internal, therefor no CR is needed.
There is already a lot of testing for the new method.
For me it is RTBC.
Comment #20
mondrakeComment #21
mondrakeComment #22
daffie commentedI like the latest change. Now is the class variable
attachedDatabasesonly used in the methods:__destruct(),attachDatabase()andgetAttachedDatabases(). A big +1 for me.Back to RTBC.
Comment #23
mondrakeRerolled
Comment #24
alexpottCommitted e67aa82 and pushed to 9.3.x. Thanks!
I'm not that wild about the no-op \Drupal\Core\Database\Connection::attachDatabase() method but it's hardly even going to be used and is marked @internal and it does allow us to make headway in removing per table prefixing without having to support extra_prefix.
Comment #26
andypostThank you! It will help to related
Comment #28
wim leersThis caused a significant regression: #3260111: Regression AND bug: SQLite source and id map tables aren't joinable anymore, but SqlBase::mapJoinable() is unaware of this.