Connection::query() does not support 'target' option

I guess it should also be removed from Connection::defaultOptions.

Shall we also add a conditional deprecation that triggers if something is still eventually passing the 'trigger' key to the $options array of Connection::query?

On it

Initial patch, will fail with deprecations from testing legacy db_* functions.

Now changing the deprecated db_* functions to remove the $options['target'] key before invoking OO API.

I cannot understand the failures in Search tests here. Will investigate further, any pointer appreciated.

For the moment, added a deprecation tests for the deprecation of the 'target' key usage in Connection::query.

Added link to issue with discussion and fixed linting

- As db_ wrappers still need this option we should unset it and continue select target through option
- throwing error still needed to notify about useless/non-working option passed and legacy needs clean-up (very helpful to catch bugs)

Great discussion and tl'dr from @catch makes sense to move to this issue summary and update links if needed, IMO no CR needed

Failed tests shows that we need to finish conversion of db_* leftovers first

#14 thanks for fixing my c/p error :)

#16 well tests should not fail since we are removing the 'target' key in all db_* functions before calling the OO code in this patch already. I can not understand where the key is still leaking in from.

#18 thanks @longwave. I think that will lead to WSOD on sites that are using modules still passing the target key, no? Deprecation is less intrusive as it will only fail in testing.

Your patch actually may be giving hints as to why #14 is failing. Here's a new patch following on #14.

#20 does not make sense, it's removing the key just before invoking the check that it is there. See this one.

I bet some code using to pass options[target] to injected database that's why it fails

Found a couple of possible culprits. Patch on the way.

CommentStatistics and Plugin/Search/NodeSearch are passing 'target' => 'replica' directly to OO methods.

We can't solve that with a different database connection though, because even when replica is specified it needs to fall back to default.

So this either means changing these places to use Database::getConnection() or adding an injectable service (that would just delegate to Database::getConnection() at least for now.

How about the approach like this? Any patch updates are welcome.

I would suggest to do #30 in a separate issue (it's a prerequisite), and postpone this on that one. This one would prevent further bugs.

Filled #3001216: Use the database.replica service where appropriate and postponing on it this issue.

#29 seems likely, I think it's a more common pattern at this point to shift listing queries to Solr (which will be faster than a query run against a replica would be). I have worked on one site that was running some read queries against replica, but it was on 6.x so couldn't actually use this feature anyway.

+++ b/core/modules/comment/src/CommentStatistics.php
@@ -52,20 +67,24 @@ class CommentStatistics implements CommentStatisticsInterface {
+    $this->connectionFactory = $connection_factory;
     $this->database = $database;
...
+    $this->replicaDatabase = $this->connectionFactory->getDatabaseConnection('replica');

main question here is do we need to make connection in constructor or not! and any reson to store connections when we have factory injected?

We definitely still need to

1. remove the wrong documentation
2. adjust the legacy db_* functions to use database.replica where needed
3. unset the 'target' key before invoking the OO API
4. and I'd still keep the @trigger_error when query() is invoked with a 'target' key in $options, it would be useful to prevent regression and help contrib to comply

I will make a reroll from #26.

EDIT - point 2 is unnecessary as in database.inc we are using static calls and not the service.

Here's the reroll.

Re-queued

+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
@@ -431,7 +431,7 @@ public function testDbTruncate() {
-   * @expectedDeprecation Passing a 'target' key to Connection::query $options argument is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, get the target connection via Database::getConnection() passing the target in input and execute query() on it. See https://www.drupal.org/node/3000931
+   * @expectedDeprecation Passing a 'target' key to Connection::query $options argument is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, get the target connection via Database::getConnection() passing the target in input and execute query() on it. https://www.drupal.org/node/2993033

"See" is gone for some reason?

"See" is gone for some reason?

Yes, effects of carpal tunnel syndrome on copy/paste operations :)

Thanks, this should do.

Thanks @longwave! Let's keep it simple

Updated IS

RTBC +1 just minor nit

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -600,6 +593,9 @@ protected function filterComment($comment = '') {
+      @trigger_error('Passing a \'target\' key to Connection::query $options argument is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, use Database::getConnection($target)->query(). See https://www.drupal.org/node/2993033', E_USER_DEPRECATED);

+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
@@ -428,6 +428,15 @@ public function testDbTruncate() {
+   * Tests deprecation of the $options 'target' key in Connection::query.
...
+   * @expectedDeprecation Passing a 'target' key to Connection::query $options argument is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, use Database::getConnection($target)->query(). See https://www.drupal.org/node/2993033

should we use full namespace for connetction & `query()`?

Thanks, addressed #48 and #49.

Looks good

+++ b/core/includes/database.inc
@@ -93,11 +91,9 @@ function db_query($query, array $args = [], array $options = []) {
+  return Database::getConnection($target)->queryRange($query, $from, $count, $args, $options);

@@ -130,11 +126,9 @@ function db_query_range($query, $from, $count, array $args = [], array $options
+  return Database::getConnection($target)->queryTemporary($query, $args, $options);

@@ -158,10 +152,9 @@ function db_query_temporary($query, array $args = [], array $options = []) {
+  return Database::getConnection($target)->insert($table, $options);

@@ -185,12 +178,9 @@ function db_insert($table, array $options = []) {
+  return Database::getConnection($target)->merge($table, $options);

@@ -214,10 +204,9 @@ function db_merge($table, array $options = []) {
+  return Database::getConnection($target)->update($table, $options);

@@ -241,12 +230,9 @@ function db_update($table, array $options = []) {
+  return Database::getConnection($target)->delete($table, $options);

@@ -270,10 +256,9 @@ function db_delete($table, array $options = []) {
+  return Database::getConnection($target)->truncate($table, $options);

@@ -301,10 +286,9 @@ function db_truncate($table, array $options = []) {
+  return Database::getConnection($target)->select($table, $alias, $options);

I think \Drupal\Core\Database\Connection::*() needs to deprecate the target option too. Atm we've only done ::query() but all the other methods should have this. Potentially this deprecation could live in \Drupal\Core\Database\Query\Query::__construct() - apart from for ::queryTemporary and ::queryRange but they both call query() so they're covered already.

Also the $options documentation foreach of them is a bit off. For example ::insert() says

   * @param array $options
   *   (optional) An associative array of options to control how the query is
   *   run. The given options will be merged with
   *   \Drupal\Core\Database\Connection::defaultOptions().

It's not merged at all with that as far as I can see. But that should be a followup.

#52 seems overkill, since all these classes' execute methods in the end hand over the execution to Connection::query where we trigger the deprecation already.

Adding a test for \Drupal\Core\Database\Query\Select to demonstrate that.

+++ b/core/includes/database.inc
@@ -53,11 +53,9 @@
+  $target = empty($options['target']) ? 'default' : $options['target'];
+  unset($options['target']);

@@ -92,11 +90,9 @@ function db_query($query, array $args = [], array $options = []) {
+  $target = empty($options['target']) ? 'default' : $options['target'];
+  unset($options['target']);

My guts tell me to create a protected method a-la getTarget($options)

Addressed #54

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -511,4 +511,39 @@ protected static function getDatabaseDriverNamespace(array $connection_info) {
+  /**
+   * Get target helper.
+   *
+   * Helps get "target" database from the query options.
+   *
+   * @param array $options
+   *   An array of options to control how the query operates.
+   * @param string|null $default
+   *   The default database target name.
+   * @param string[] $default_aliases
+   *   Aliases for the target that should be recognized as a default target.
+   *   For cases like target "replica" that should be changed to the "default".
+   *
+   * @return mixed|string
+   *   The target database for the database connection.
+   */
+  public static function getTarget(array &$options, $default = 'default', array $default_aliases = []) {

Honestly it looks a bit weird to add a method on the Database class that is only used in deprecated code. If we really need that, maybe a _db_get_target procedural function in database.inc could do? Then to be deprecated itself and set for removal in D9?

Addressed #57

db_query() still not deprecated completely, so _db_get_target() shouldn't trigger deprecation message right now.

Removed trigger_error call. This will require a followup issue.

Followup issue #3049380: Complete deprecation of _db_get_target() function

This looks good to me. Just do not know if @alexpott's objection in #52 is properly addressed in #53. RTBCing to bring to the right eyes.

Change is hard. Nice work on a complex issue.

+++ b/core/includes/database.inc
@@ -472,10 +433,44 @@ function db_driver() {
+ * @deprecated as of Drupal 8.8.0, will be removed in Drupal 9.0.0.
+ */
+function _db_get_target(array &$options, $default = 'default', array $default_aliases = []) {

There needs to be an associated change record. Let's point at https://www.drupal.org/node/2993033.

This is a bit of a special case because we adding a deprecated function. I've debated whether this is a good idea but I think it is the best of a lot of bad options. Also I agree we shouldn't add an @trigger_error because all the methods calling it are already deprecated apart from db_query() and that's happening in another issue.

+++ b/core/includes/database.inc
@@ -472,10 +433,44 @@ function db_driver() {
+ * @param string|null $default
+ *   The default database target name.
+ * @param string[] $default_aliases
+ *   Aliases for the target that should be recognized as a default target.
+ *   For cases like target "replica" that should be changed to the "default".

I don't see why we have these two arguments and not a single boolean flag which is something like $allow_replica that defaults to TRUE and is FALSE for the couple of cases where it cannot be. The additional complexity seems unnecessary. Maybe it was when it was more generic and part of the database class but now it that does not appear to be the case.

Working on this.

Addressed #64

Missed one.

#67 addressed the #64
Looks good. +1 for rtbc

Committed 07a3820 and pushed to 8.8.x. Thanks!

It needs to publish change record

Follow-up issue is unblocked and ready for the review #3049380: Complete deprecation of _db_get_target() function