Replace all calls to db_merge(), which is deprecated

#4 solves the problem.Thanks

The meta issue reads: "...replace all usages of the function (except tests for the function itself)...". GarbageCollectionTest calls it.

This patch is originally written by @Pavan_B_S in #2856714-4: Replace all calls to db_merge, which is deprecated. So please give him credit for it.

I have reviewed the patch from @Pavan_B_S in #2856714-5: Replace all calls to db_merge, which is deprecated and I came to the following conclusion:

All calls to db_merge() have been replaced.
The function has already been marked as deprecated.
There are no code references in D8.
The patch is RTBC for me.

Almost forgot: @chiranjeeb2410 created an interdiff.txt file for a patch. So he should also get a credit.

Thanks everyone for working on this!

+++ b/core/tests/Drupal/KernelTests/Core/Database/MergeTest.php
@@ -18,7 +18,7 @@ class MergeTest extends DatabaseTestBase {
-    $result = db_merge('test_people')
+    $result = \Drupal::database()->merge('test_people')

@@ -201,7 +201,7 @@ function testInvalidMerge() {
-      db_merge('test_people', $options)
+      \Drupal::database()->merge('test_people', $options)

So, this and other calls in MergeTest are the tests for db_merge(), which does not actually exactly wrap the same service. That would be beyond the scope of not removing the self-tests. If it were a thin wrapper that would be one thing, but not much is testing the $options parameter, which is set to a different default in the procedural function:

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

I'm not sure how to handle that.

I think if this patch were completing the scope defined on the issue, it would not change the merge tests.

Here is the code related to the target in getConnection():

  // 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';
  }

So it could be that the hunk in db_merge() is just redundant anyway.

The question is whether we should remove all test coverage for db_merge(). I'm hesitant about that, so we could remove the MergeTest changes from this patch and discuss how to handle the testing in a followup. I don't think the correct way for Connection::merge() to be tested would be getting the service from the container so the followup should check whether it already has separate tests. We can and still should remove the others in this patch, though.

Thanks!

I have fixed the 'db_merge-2848952-8.patch' patch because it was not applying any more because 'array()' notation has already been applied in the files affected by the patch.

Thank you for the continued work everyone.

+++ b/core/modules/shortcut/src/ShortcutSetStorage.php
@@ -71,7 +71,7 @@ public function deleteAssignedShortcutSets(ShortcutSetInterface $entity) {
-    db_merge('shortcut_set_users')
+    \Drupal::database()->merge('shortcut_set_users')

I think we need to inject the dependency.

As per comment #17 updated patch file.

Looks very nice, thanks!

I found one nitpick:

+++ b/core/modules/shortcut/src/ShortcutSetStorage.php
@@ -22,6 +23,13 @@ class ShortcutSetStorage extends ConfigEntityStorage implements ShortcutSetStora
+  ¶

Trailing whitespace.

Please find updated patch file.

@jeetendrakumar Would you please post the interdiff on #20?

interdiff on #20

I applied the patch in #22. It applied cleanly and changes all the occurences.

Thank you for working on this everyone! I have some notes and suggestions for getting this one finished:

• I asked for a constructor injection in #17, which was done quite properly by @jeetendrakumar in #18. But I was in error when I asked for it. The parent issue specifies "Open follow-up issues to inject dependencies as appropriate" and the Issue Scope Guidelines cite a similar change as an example of "bad scope".
• Would a contributor please remove the injection in ShortcutSetStorage from this patch and return it to the static call?
• Would a contributor please open a follow-up issue that is stalled on this issue to inject the dependency in ShortcutSetStorage?

Sorry for the confusion.

Fixed #24.

This doesn't belong in the search.module component, I think database system is a better match.

The patch does apply, and it replaces all the calls to db_merge - the only remaining place where I can find it is in database.inc after applying the patch.

My review in #14 is still not addressed. We also still need the followup in #24.

Thanks!

Filed postponed follow-up for #14-#24 #2947769: Inject services after replacing all calls for db_merge()

Also db_merge() should trigger error like #2820179-18: Replace usages of deprecated method db_change_field(), working on it

Patch
- moves database connection service retrieval out of loops
- adds triggering deprecated error
- adds test for deprecated error
- adds todos to filed issues

Filed second follow-up for #14.2 #2947775: Move setting default target out of db_merge() and other deprecated db_* functions

According #2820179-18: Replace usages of deprecated method db_change_field() test class should be annotated as @group legacy

annotation per method is a way

Addressing #14.1 - we should not make any changes in the self-tests as it defined in the parent issue.

Added expectedDeprecation for the legacy test.

Oops. My bad

It seems like @voleger mixed up patch with changes for issue 2848817
I added expectedDeprecation annotation for legacy test using patch in #37 as baseline since this was the last not-mixed patch.

Added the possibility to reuse added database connection in further db_* function replacements.

missed one replacement

+++ b/core/tests/Drupal/KernelTests/Core/Database/MergeTest.php
@@ -9,11 +9,14 @@
+ * @group legacy
...
+   * @expectedDeprecation Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call merge() on it. For example, $injected_database->merge($table, $options);

@@ -39,6 +42,8 @@ public function testMergeInsert() {
+   * @expectedDeprecation Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call merge() on it. For example, $injected_database->merge($table, $options);

@@ -67,6 +72,8 @@ public function testMergeUpdate() {
+   * @expectedDeprecation Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call merge() on it. For example, $injected_database->merge($table, $options);

@@ -88,6 +95,8 @@ public function testMergeUpdateExcept() {
+   * @expectedDeprecation Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call merge() on it. For example, $injected_database->merge($table, $options);

@@ -114,6 +123,8 @@ public function testMergeUpdateExplicit() {
+   * @expectedDeprecation Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call merge() on it. For example, $injected_database->merge($table, $options);

@@ -143,6 +154,8 @@ public function testMergeUpdateExpression() {
+   * @expectedDeprecation Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call merge() on it. For example, $injected_database->merge($table, $options);

@@ -162,6 +175,8 @@ public function testMergeInsertWithoutUpdate() {
+   * @expectedDeprecation Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call merge() on it. For example, $injected_database->merge($table, $options);

@@ -194,6 +209,8 @@ public function testMergeUpdateWithoutUpdate() {
+   * @expectedDeprecation Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call merge() on it. For example, $injected_database->merge($table, $options);

+++ b/core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php
@@ -59,6 +60,17 @@ public function testDBIndexExists() {
+   * @expectedDeprecation Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call merge() on it. For example, $injected_database->merge($table, $options);
+   */
+  public function testDBMergeIsDeprecated() {
+    $this->assertTrue(db_merge('test') instanceof Merge);

Why not move this to DatabaseLegacyTest.php and replace usage in MergeTest.php

patch does not apply

Rerolled the patch.

NW to address #48. Working on it.

Addressed #48, injected the db connection in ShortcutSetStorage.

This should be more or less ready - GarbageCollectionTest was using Database::getConnection() already; if that needs to be changed it will be done by a follow up of #2991337: Document the recommended ways to obtain the database connection object.

Sorry wrong interdiff in #53, this should be ok.

$this->assertSame is strict on type.

Looks good

Committed 44b3b41 and pushed to 8.7.x. Thanks!

+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
@@ -172,4 +171,22 @@ public function testDbCreateTable() {
+    $result = db_merge('test_people')
+      ->key('job', 'Presenter')
+      ->fields([
+        'age' => 31,
+        'name' => 'Tiffany',
+      ])
+      ->execute();

I'm sure it needs follow-up to change test to "instanceOf" assertion because we must test that this function returns declared object - but instead it tests that object works!
For that purpose we have DB api tests)

#61: umm... how about to have both assertInstanceOf and the result of the operation? I think here it is not bad for contrib drivers to go through the actual operation.

Contrib db drivers has own testing for driver classes anyway, so IMO no reason to test what driver class does cos it's it totally different test

Opened follow up: #2994955: Followup to #2848952 - fix DatabaseLegacyTest::testDbMerge