Replace all calls to db_select, which is deprecated

Reroll for 8.6.x
Reverted changes for SelectTest.

There are a few comment references and core/tests/Drupal/KernelTests/Core/Database/SelectTest.php Are those meant to be replaced in this patch as well?

This is from meta issue #2848161: [meta] Replace calls to deprecated db_*() wrappers:

3. Open an issue (Category: Task, priority: Normal, make this issue the parent issue) to replace all usages of the function (except tests for the function itself) and link the issue below.

So, as I understand, calls inside tests that cover that function shouldn't be replaced.

Rerolled
Added legacy test
Applied recommendations from #2991337: Document the recommended ways to obtain the database connection object

+++ b/core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php
@@ -19,10 +20,11 @@ class DatabaseTestController {
+    $connection = Database::getConnection();

this is controller - needs DI

Rerolled after #2996441: Replace all calls to db_query_temporary, which is deprecated.
Also #2996441: Replace all calls to db_query_temporary, which is deprecated. addressed #14 so patch is ready for review

Just reroll, no changes

queued for retest mysql cos failure looks unrelated

Probably should postpone on #2853118: Replace all calls to db_insert, which is deprecated as it is just going to need another reroll, all these db_* deprecation patches keep clashing in DatabaseLegacyTest.

yep, better to reroll it after db_insert patch

Postponed on #2853118: Replace all calls to db_insert, which is deprecated

db_insert is in, so rerolled this one hopefully for the final time.

Hope it is green!

Adjust credits - @jeetendrakumar somehow was excluded

reroll after #2996030: Convert web tests to browser tests for node module - round 2

+++ b/core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest.php
@@ -343,12 +343,12 @@ public function testJoinSubquery() {
+    $query = $this->connection->select('test_task', 'tt', ['target' => 'replica']);

I am not so sure about this. The replica target is dealt with in db_select by selecting a different connection through Database::getConnection:

Database::getConnection($options['target'])->select($table, $alias, $options);

but

$this->connection->select('test_task', 'tt', ['target' => 'replica'])

is not changing the connection AFAICS.

We should address #2947775: Move setting default target out of db_merge() and other deprecated db_* functions first?

More eyes please...

Addressed #28

Is it worth catching the 'target' option being accidentally passed to Query::_construct() and triggering an error on this? This would mean modifying all the deprecated db_*() functions slightly to remove that from the array, but it would catch bugs elsewhere now this parameter has moved.

+++ b/core/modules/search/src/Plugin/views/argument/Search.php
@@ -48,7 +48,7 @@
+      $this->searchQuery = \Drupal::database('replica')->select('search_index', 'i')->extend('Drupal\search\ViewsSearchQuery');

+++ b/core/modules/search/src/Plugin/views/filter/Search.php
@@ -120,7 +120,7 @@
+      $this->searchQuery = \Drupal::database('replica')->select('search_index', 'i')->extend('Drupal\search\ViewsSearchQuery');

as you affect this lines, please use ViewsSearchQuery::class directly instead of string

#30 but that would practically mean that we are deprecating passing the 'target' key in the $options array, no? And creating a BC break.

IMHO we should try to address this in lower level in Connection::query / prepareQuery / prepare so that when the 'target' key is set, the statement is prepared against the target connection and not the default one. In other words, swap the used connection on lower level and keep BC.

@mondrake Good point about keeping BC

On one hand it is "strange" to change connection withing connection object
otoh functional wrappers were working this way... but "database::select/query" do not

So allowing changing connection through options is API addition and not very useful
But makes sense to update CR about that - db_select & database::select already works this way since 8.0

So, should this issue be postponed to #2947775: Move setting default target out of db_merge() and other deprecated db_* functions?

Tagging "Needs framework manager review" as I think direction is needed here now.

Yes let's postpone.

#30 but that would practically mean that we are deprecating passing the 'target' key in the $options array, no? And creating a BC break.

Why is this a BC break? 'target' isn't currently supported anyway when called via the OO route, you have to explicitly specify the target when calling getConnection(). If not an error we could log a warning or a silenced error?

Unpostponing based on the discussion in #2947775: Move setting default target out of db_merge() and other deprecated db_* functions - using Database::getconnection() when you need to specify the target is fine for conversions.

Rerolled and addressed #31

+++ b/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php
@@ -258,11 +258,11 @@
    * that.
    */
   public function testUnion() {
-    $query_1 = $this->connection->select('test', 't')
+    $query_1 = db_select('test', 't')
       ->fields('t', ['name'])
       ->condition('age', [27, 28], 'IN');
 
-    $query_2 = $this->connection->select('test', 't')
+    $query_2 = db_select('test', 't')
       ->fields('t', ['name'])
       ->condition('age', 28);
 
@@ -281,11 +281,11 @@

@@ -281,11 +281,11 @@
    * Tests that we can UNION ALL multiple SELECT queries together.
    */
   public function testUnionAll() {
-    $query_1 = $this->connection->select('test', 't')
+    $query_1 = db_select('test', 't')
       ->fields('t', ['name'])
       ->condition('age', [27, 28], 'IN');
 
-    $query_2 = $this->connection->select('test', 't')
+    $query_2 = db_select('test', 't')
       ->fields('t', ['name'])
       ->condition('age', 28);
 

Strange it looks like in the interdiff these were reverted but it seems the patch is OK. Back to RTBC.

Just confirming that applying the patch and checking with git grep db_select there are no more occurences in code. There's a bunch of leftover references in comments and in test messages but I suppose these can be addressed in a follow-up documentation issue.

+++ b/core/modules/search/src/Plugin/views/argument/Search.php
@@ -48,7 +49,7 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+      $this->searchQuery = \Drupal::database('replica')->select('search_index', 'i')->extend(ViewsSearchQuery::class);

+++ b/core/modules/search/src/Plugin/views/filter/Search.php
@@ -120,7 +121,7 @@ public function validateExposed(&$form, FormStateInterface $form_state) {
+      $this->searchQuery = \Drupal::database('replica')->select('search_index', 'i')->extend(ViewsSearchQuery::class);

+++ b/core/modules/tracker/tracker.pages.inc
@@ -22,13 +22,13 @@
+    $query = \Drupal::database('replica')->select('tracker_node', 't')

This is no-go cos \Drupal::database() has no $target argument, so it needs to use other "factory"

This is postponed now.

I don't think it needs to be postponed, we can use Database::getConnection() in the places that need it, then the database factory service issue can either convert or have a follow-up to convert those cases.

Changed replica connections to Database::getConnection() as per #48

Looks good

Fixed this docs issue on commit, and pushed to 8.7.x, thanks! This issue flushed quite a lot of other issues out, see you in the follow-ups.