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

Issue fork drupal-3124674

Command icon 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:

Comments

daffie created an issue. See original summary.

neslee canil pinto’s picture

Status: Active » Needs review
StatusFileSize
new6.99 KB
mondrake’s picture

mondrake’s picture

StatusFileSize
new928 bytes

@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

      // Add the original simpletest prefix so SQLite can attach its database.
      // @see \Drupal\Core\Database\Driver\sqlite\Connection::init()
      $connection_info[$target]['prefix'][$value['prefix']['default']] = $value['prefix']['default'];

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.

mondrake’s picture

StatusFileSize
new4.35 KB
new3.44 KB

Let's see this, as a first step to separate attaching the additional database from the constructor into a method of its own.

mondrake’s picture

StatusFileSize
new9.07 KB
new5.98 KB

Getting a bit more shape, with deprecation of ::setPrefix. It's replaced by a ::addPrefix method 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.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Postponed » Active

Changed to MR workflow; working on this.

mondrake’s picture

Title: Add functionality to the SQLite driver so that migration testing no longer uses per-table prefixing » Refactor functionality for the SQLite driver to stop migration testing usage of the 'extra_prefix' connection option
Issue summary: View changes
mondrake’s picture

Assigned: mondrake » Unassigned
Status: Active » Needs review
mondrake’s picture

Component: migration system » database system

Changing issue component to 'database system' since the patch is adding API to the database layer.

daffie’s picture

Status: Needs review » Needs work

Started 2 threads on the MR.

mondrake’s picture

Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Needs work

Replied to the threads on the MR.

mondrake’s picture

Status: Needs work » Needs review

Thanks for reviews, @daffie, made changes and commented on the MR.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

It 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.

mondrake’s picture

Status: Reviewed & tested by the community » Needs work
mondrake’s picture

Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Reviewed & tested by the community

I like the latest change. Now is the class variable attachedDatabases only used in the methods: __destruct(), attachDatabase() and getAttachedDatabases(). A big +1 for me.

Back to RTBC.

mondrake’s picture

Rerolled

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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.

  • alexpott committed e67aa82 on 9.3.x
    Issue #3124674 by mondrake, Neslee Canil Pinto, daffie: Refactor...
andypost’s picture

Thank you! It will help to related

Status: Fixed » Closed (fixed)

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