How best to persist source database credentials?

I'm going to raise this to a migrate critical, since it seems like it's required prior to calling Migrate "supported" in D8 core.

Right off the bat I would rule out options #2 and #4. The UX of #2 is so abysmal that most users will probably hate us, and writing to settings.php is not merely gross, but super-annoying and potentially dangerous (permissions issues). In my mind, only #1 and #3 are viable options.

On balance, #3 (state) seems like the best option to me. It cannot be persisted across environments, true, but at least it won't be stored in plain text for all to see. I don't think it's much to ask for people to re-enter their DB credentials when switching between environments -- they only have to do it once per environment and there's a good chance that, most of the time, their credentials will differ across environments anyway. I think state offers us the best balance between UX and (such as it is) security concerns, and I'd be willing to sacrifice portability for those.

#3 isn't a bad option although we still end up with plain text credentials in the database.

I actually think a combination of #2 and #4 could be great. Eg, you can enter the details in the form, but, if the user has put an entry in their settings file then that takes precedence. We don't write to settings.php, they can simply be put there by a developer? That works well for developers who often have settings.local.php files anyway per environment.

This is where we start to intersect with #2552791: MigrateSqlSource should use dependency injection. It should be Migrate Upgrade's job to persist the source DB credentials as it sees fit, and it should be the thing responsible for passing a fully initialized Connection object to the source plugins. Ideally, I'd like to see all of this done in the same patch.

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -58,20 +58,43 @@ public function __toString() {
   +        // @todo: Inject the state service.
   +        $database_state = \Drupal::state()->get($this->configuration['database_state_key']);
   

   Are we going to have a follow-up issue for injecting this dependency?

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -58,20 +58,43 @@ public function __toString() {
   +        if (isset($database_state['target'])) {
   +          $target = $database_state['target'];
   +        }
   +        else {
   +          $target = 'default';
   +        }
   +        if (isset($database_state['key'])) {
   +          $key = $database_state['key'];
   +        }
   +        else {
   +          $key = 'migrate';
   +        }
   

   Just for brevity's sake, can these use ternary syntax instead of if/else blocks?

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -58,20 +58,43 @@ public function __toString() {
   +        if (isset($this->configuration['target'])) {
   +          $target = $this->configuration['target'];
   +        }
   +        else {
   +          $target = 'default';
   +        }
   +        if (isset($this->configuration['key'])) {
   +          $key = $this->configuration['key'];
   +        }
   +        else {
   +          $key = 'migrate';
   +        }
   

   Ditto here.

   Actually, these two code blocks (using state or $this->configuration) are awfully similar -- seems to me they could be refactored into a single method.

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -34,10 +37,31 @@
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration = NULL) {
   +    return new static(
   +      $configuration,
   +      $plugin_id,
   +      $plugin_definition,
   +      $migration,
   +      \Drupal::state()
   +    );
   

   \Drupal::state() should be $container->get('state').

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -61,47 +85,44 @@
   +   * @param $database_info
   

   Needs a type hint and documentation.

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -61,47 +85,44 @@
   +  protected function setUpDatabase($database_info) {
   

   Should be type hinted.

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
   @@ -51,8 +52,8 @@
   @@ -89,6 +90,7 @@ public static function create(ContainerInterface $container, array $configuratio
   
   @@ -89,6 +90,7 @@ public static function create(ContainerInterface $container, array $configuratio
          $plugin_id,
          $plugin_definition,
          $migration,
   +      \Drupal::state(),
          $container->get('entity.manager')
   

   $container->get('state')

5. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/d6/Drupal6SqlBaseTest.php
   @@ -68,8 +68,9 @@ class Drupal6SqlBaseTest extends MigrateTestCase {
   +    $state = $this->getmock('Drupal\Core\State\StateInterface');
   

   Er...should be getMock() :)

Hot diggity daffodil! This looks great to me. Presuming the PostrgeSQL and SQLite bots approve, methinks this is RTBC.

Great work. This solution indeed seems inline with how we handle sensitive data in other parts of the system.

Committed and pushed to 8.0.x. Thanks!