Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Some problems with the fallback mechanisms in SqlBase::getDatabase():
- 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.
- 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.
- 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
- Use explicit configuration ahead of fallback_state_key in getDatabase().
- 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
- 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
Comment | File | Size | Author |
---|---|---|---|
#25 | interdiff_22-25.txt | 646 bytes | heddn |
#25 | 2830031-25.patch | 8.08 KB | heddn |
Comments
Comment #2
mikeryanSo, fallback_state_key doesn't appear to be tested at all, might be a good idea to do that...
Comment #5
mikeryanOK, let's try again.
Comment #6
mikeryanA 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.
Comment #8
mikeryanRerolling - still needs tests.
Comment #9
mikeryanComment #10
mikeryanOK, this should be ready for manual review once the bots pass it...
Comment #11
mikeryanAdded draft change record.
Comment #12
phenaproximaSelf-assigning for review.
Comment #13
phenaproximaHaving 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?
Comment #14
mikeryan@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.
Comment #15
joelpittetShould/could the CR discuss how and where
database_state_key
should be defined?Comment #16
phenaproximaRe #14:
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.
Comment #17
phenaproximaUnassigning.
Comment #18
mikeryanAdded a comment, and expanded the CR.
Comment #19
heddnI'll take this one on for reviewing.
Comment #20
heddnSetting to NW for clarification on first point.
'database_state_key' isn't mentioned in the summary above. Is this an omission?
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.
Comment #21
heddnComment #22
mikeryanNo - 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.
Not sure how to do this here, or if this is really the place to document that...
Comment #23
heddnI 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.
Comment #24
Gábor HojtsyOnly found one minor thing but this would be important for understandability :)
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:
Comment #25
heddnFixes #24.
Comment #26
phenaproximaNice. Pre-RTBC.
Comment #27
Gábor HojtsyNow 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.