Problem/Motivation

Some problems with the fallback mechanisms in SqlBase::getDatabase():

  1. If the global fallback_state_key is set, it overrides explicit configuration on the specific source plugin - a fallback should be prioritized below explicit configuration.
  2. Per #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled, the use of global fallbacks (fallback_state_key or a 'migrate' connection) with migrate_drupal (or potential contrib/custom modules of similar design) is problematic. In particular, setting a fallback to a database intended for use with specific migrations affects other migrations which are not explicitly configured with a database connection.
  3. In the class documentation, fallback_state_key is not documented, nor is the priority of the potential means of configuration. In addition, the way the documentation is structured encourages using the 'migrate' connection, where it should favor explicit configuration.

Proposed resolution

  1. Use explicit configuration ahead of fallback_state_key in getDatabase().
  2. Rewrite the docs to deprecate/discourage fallback_state_key and the fallback to an implicit 'migrate' connection, and fully document the configuration priorities.

Remaining tasks

  1. Write a patch, starting from the relevant bit of #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled.

User interface changes

N/A

API changes

fallback_state_key will now actually be a fallback when plugin-specific configuration is not present. This would only affect someone who was using fallback_state_key and expected that it would override migration-specific configuration, which seems rather unlikely (in practice, fallback_state_key is only used by the upgrade UI and is only present during the actual migration process).

Data model changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

So, fallback_state_key doesn't appear to be tested at all, might be a good idea to do that...

Status: Needs review » Needs work

The last submitted patch, 2: fix_sqlbase_fallback-2830031-2.patch, failed testing.

The last submitted patch, 2: fix_sqlbase_fallback-2830031-2.patch, failed testing.

mikeryan’s picture

OK, let's try again.

mikeryan’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
@@ -11,13 +11,35 @@
+ * 4. Otherwise, if a connection named 'migrate' exists, that is used as the
+ *    database connection.

A reminder for the next iteration:

5. Otherwise, RequirementsException is thrown.

That actually won't be true until #2830036: MigrationPluginManager::getDefinitions() blows up in node derivers gets in, so technically postponed on that, but leaving open for now to add tests.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Needs work » Needs review
FileSize
3.62 KB

Rerolling - still needs tests.

mikeryan’s picture

Status: Needs review » Needs work
mikeryan’s picture

OK, this should be ready for manual review once the bots pass it...

mikeryan’s picture

Added draft change record.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

phenaproxima’s picture

+++ b/core/modules/migrate/tests/src/Kernel/SqlBaseTest.php
@@ -23,11 +24,23 @@ class SqlBaseTest extends MigrateTestBase {
+    Database::addConnectionInfo($key, $target, Database::getConnectionInfo('default')['default']);

Having a target and key of 'default' and 'default', respectively, is a pretty common configuration. I think, for stronger assertions, we should randomly generate both if possible. Unless I'm missing something?

mikeryan’s picture

@phenaproxima: The key and target we're testing are in $key and $target - we need a real db info array to pass to addConnectionInfo(), so we use ['default']['default'] because we know that exists.

joelpittet’s picture

Should/could the CR discuss how and where database_state_key should be defined?

phenaproxima’s picture

Re #14:

The key and target we're testing are in $key and $target - we need a real db info array to pass to addConnectionInfo(), so we use ['default']['default'] because we know that exists.

Can we add a comment to that effect?

Also, +1 to @joelpittet's comment in #15. The CR should document everything, because the whole fallback system is confusing (and it's really kind of a BC layer anyway, because the SQL source plugins should be, and always should have been, using dependency injection). I also think the CR should include code samples, as this is the sort of thing that is difficult to explain with documentation alone.

phenaproxima’s picture

Assigned: phenaproxima » Unassigned

Unassigning.

mikeryan’s picture

Added a comment, and expanded the CR.

heddn’s picture

Assigned: Unassigned » heddn

I'll take this one on for reviewing.

heddn’s picture

Status: Needs review » Needs work

Setting to NW for clarification on first point.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
    @@ -15,13 +15,36 @@
    + * 1. If the source plugin configuration contains a key 'database_state_key',
    

    'database_state_key' isn't mentioned in the summary above. Is this an omission?

  2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
    @@ -15,13 +15,36 @@
    + * It is strongly recommended that database connections be explicitly defined
    + * via 'database_state_key' or in the source plugin configuration. Defining
    + * migrate.fallback_state_key or a 'migrate' connection affects not only any
    + * migrations intended to use that particular connection, but all
    

    Defining in 'source plugin configuration' means setting the 'key' for each migration.

    We cannot mention migrate_plus groups specifically here, but can we mention that other methods do exist in contrib? And these can help inject some of this into the mix for us.

heddn’s picture

Assigned: heddn » Unassigned
mikeryan’s picture

Status: Needs work » Needs review
FileSize
8.09 KB

'database_state_key' isn't mentioned in the summary above. Is this an omission?

No - the list above is of database configuration which can appear either directly in the source plugin, or in the state referenced by key 'database_state_key'. Expanded that comment a bit to hopefully clarify.

We cannot mention migrate_plus groups specifically here, but can we mention that other methods do exist in contrib? And these can help inject some of this into the mix for us.

Not sure how to do this here, or if this is really the place to document that...

heddn’s picture

I wasn't sure about 20.2 and I tend to agree it isn't really possible. And the other nit is caught, so this looks now. Added an interdiff between 18 & 22.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Only found one minor thing but this would be important for understandability :)

+++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
@@ -15,13 +15,37 @@
- * Sources whose data may be fetched via DBTNG.
+ * Sources whose data may be fetched via a DBTNG connection.

Let's not use DBTNG, this is the only place in core referring to DBTNG (uppercase). Also only 4 mentions of dbtng (lowercase) and no other mention in any other case:

$ git grep -i DBTNG
core/modules/migrate/src/Plugin/migrate/source/SqlBase.php: * Sources whose data may be fetched via DBTNG.
core/modules/views/src/Plugin/views/query/Sql.php:   * Query tags which will be passed over to the dbtng query object.
core/modules/views/src/Plugin/views/query/Sql.php:   * Internally the dbtng method "where" is used.
core/modules/views/src/Plugin/views/query/Sql.php:   * Internally the dbtng method "having" is used.
core/modules/views/tests/src/Kernel/Plugin/JoinTest.php:    // Build the actual join values and read them back from the dbtng query
heddn’s picture

Status: Needs work » Needs review
FileSize
8.08 KB
646 bytes

Fixes #24.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Nice. Pre-RTBC.

Gábor Hojtsy’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Now looks good to me, thanks.

Committed 68d8cd2 and pushed to 8.4.x. Thanks!

Also updated the title of the change record and updated the affected version number to 8.4.x.

  • Gábor Hojtsy committed 68d8cd2 on 8.4.x
    Issue #2830031 by mikeryan, heddn, phenaproxima: Fix SqlBase fallback...

Status: Fixed » Closed (fixed)

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