Move setting default target out of db_merge() and other deprecated db_* functions

I guess we have no work right here because this logic already implemented at \Drupal\Core\Database\Database::getConnection() method.

/**
   * Gets the connection object for the specified database key and target.
   *
   * @param string $target
   *   The database target name.
   * @param string $key
   *   The database connection key. Defaults to NULL which means the active key.
   *
   * @return \Drupal\Core\Database\Connection
   *   The corresponding connection object.
   */
  final public static function getConnection($target = 'default', $key = NULL) {
    if (!isset($key)) {
      // By default, we want the active connection, set in setActiveConnection.
      $key = self::$activeKey;
    }
    // If the requested target does not exist, or if it is ignored, we fall back
    // to the default target. The target is typically either "default" or
    // "replica", indicating to use a replica SQL server if one is available. If
    // it's not available, then the default/primary server is the correct server
    // to use.
    if (!empty(self::$ignoreTargets[$key][$target]) || !isset(self::$databaseInfo[$key][$target])) {
      $target = 'default';
    }

    if (!isset(self::$connections[$key][$target])) {
      // If necessary, a new connection is opened.
      self::$connections[$key][$target] = self::openConnection($key, $target);
    }
    return self::$connections[$key][$target];
  }

The target and key property of the connection instance can be set only once during ::getConnection call.

So we need to create some helper method to reset the connection property on the Connection instance level that will run the same code from getConnection method in Database class.

Raising to major as we discovered in #2873684: Replace all calls to db_select, which is deprecated there are BC implications here. See comments #28 onwards.

#2873684-28: Replace all calls to db_select, which is deprecated

The differing logic in db_merge() compared to all the other db_* functions has been in core since the original database layer patch landed in 2008.

Choosing deliberately to write to the replica connection would be a bug - it's a replica being replicated to.

Overriding that explicit choice then both feels unnecessary and potentially a bug too.

So I'm not sure we need to provide bc for the db_merge( ) behaviour at all.

#13 @catch thanks for feedback - that would cover anything 'writing' to the db, merge, insert, update, delete, upsert, truncate, opening transactions. How about 'reading' (select, query, queryTemporary, queryRange)? We may want to address query to replica directly as we are discovering in #2873684: Replace all calls to db_select, which is deprecated.

Wouldn't you need to use Database::getConnection() for those cases?

While 'replica' is a special case supported by core for a handful of select queries, isn't it up to the caller to determine which database they want to use? If custom code wants to write to a specific connection, be it 'replica' or 'custom' or 'remote', that is up to them.

While we support the 'target' parameter in $options in the db_* calls we have never pretended to support it in the OO world, so I don't see where we need backward compatibility, unless we are to replicate this apparent bug in db_merge() etc. However, we could try to be helpful by triggering a warning/error if someone does try to use 'target' when blindly using find and replace to convert e.g. db_select() to \Drupal\Core\Database\Connection::select().

edit: Connection::defaultOptions() says 'target' is supported in query options, but I don't see where this is actually implemented? By the time you have a Connection object open you cannot change the target?

   * Returns the default query options for any given query.
   *
   * A given query can be customized with a number of option flags in an
   * associative array:
   * - target: The database "target" against which to execute a query. Valid
   *   values are "default" or "replica". The system will first try to open a
   *   connection to a database specified with the user-supplied key. If one
   *   is not available, it will silently fall back to the "default" target.
   *   If multiple databases connections are specified with the same target,
   *   one will be selected at random for the duration of the request.

Or to expand a bit there are three possible cases:

1. You just want to read or write from the default connection.

2. You want to read or write from a custom connection which you know exists (an easy core example is a migration source database) via setting $key.

3. You want to read from a replica if it's available, but fall back to the default connection if it's not - what the fallback in Database::getConnection does.

So the question seems to be, should there be another mechanism other than Database::getConnection() to do #2/#3 in that list?

For #2 you could create a new service just for that connection and use that instead of @database and inject it.

For #3 is the issue that it's not injectable? But whatever we inject is going to have to call Database::getConnection() on the fly anyway, since that's what the container definition for @database does anyway. Some kind of factory that can be mocked which does this might be fine though, but I don't think it's a blocker to any of the deprecation issues.

I agree with #17 and #16. As @longwave says

isn't it up to the caller to determine which database they want to use?

Yep it is. So calling db_insert() with the replica db as an option is just wrong. It might be worth updating the CR to note this change but as @catch says

Choosing deliberately to write to the replica connection would be a bug

I think this issue should be "closed won't fix"

#16:

edit: Connection::defaultOptions() says 'target' is supported in query options, but I don't see where this is actually implemented? By the time you have a Connection object open you cannot change the target?

Yes you're right, that documentation is just completely wrong.

OK so I think there are two issues here, but both are pre-existing issues that shouldn't block deprecated code removals:

1. The documentation for Connection::query() and by extension Connection::defaultOptions() is wrong - we should open an issue to correct this.

2. There's no injectable way to use different database targets (for different database keys you can add a different connection as a separate service though) - we could possibly repurpose this issue for that.

Tagging as 'needs issue summary update' but for me it's fine if conversions use Database::getConnection() when they need to specify target.

Opened #3000931: Connection::query() does not support 'target' option

#21.2

You can only set a parameter before the container is compiled: not at run-time. To learn more about compiling the container see Compiling the Container.

https://symfony.com/doc/3.4/service_container/parameters.html

For me, it sounds like we need to define a service (something like @database.factory) that has the method that can generate the connection instance with the target that different to 'default'.

I suppose what will be left to do here then would be just removing the

if (empty($options['target'])) {
$options['target'] = 'default';
}

blocks from the alreAdy deprecated db_* legacy functions?

Edit - ignore, this is wrong.

I suggest to won’t fix this - we won’t be doing what the IS says, and we are already addressing in #3000931: Connection::query() does not support 'target' option what to do with the leftover code in the legacy functions.

Just reopen if you disagree.