Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vidhatanand created an issue. See original summary.

pritamprasun’s picture

pritamprasun’s picture

Status: Active » Needs review
JayKandari’s picture

Assigned: Unassigned » JayKandari
cilefen’s picture

Issue summary: View changes
JayKandari’s picture

Assigned: JayKandari » Unassigned
Status: Needs review » Needs work

#2 looks good.
another instance of "db_set_active()" is present in following File: Kindly add this to #2 as well.
core/lib/Drupal/Core/Database/Install/Tasks.php:159

Thanks!

gaurav.kapoor’s picture

call to db_set_active in core/lib/Drupal/Core/Database/Install/Tasks.php:159 also changed with new syntax.

gaurav.kapoor’s picture

Status: Needs work » Needs review
JayKandari’s picture

Status: Needs review » Needs work

Minor Code standards issue. Else it looks good.

index 40a589c..7628a40 100644
--- a/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php

--- a/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php
@@ -59,7 +59,7 @@ public function minimumVersion() {

@@ -59,7 +59,7 @@ public function minimumVersion() {
   protected function connect() {
     try {
       // This doesn't actually test the connection.
-      db_set_active();
+       \Drupal\Core\Database\Database::setActiveConnection();
       // Now actually do a check.

--- a/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -58,7 +58,7 @@ public function minimumVersion() {

@@ -58,7 +58,7 @@ public function minimumVersion() {
   protected function connect() {
     try {
       // This doesn't actually test the connection.
-      db_set_active();
+       \Drupal\Core\Database\Database::setActiveConnection();
       // Now actually do a check.

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php
@@ -54,7 +54,7 @@ public function getFormOptions(array $database) {

@@ -54,7 +54,7 @@ public function getFormOptions(array $database) {
   protected function connect() {
     try {
       // This doesn't actually test the connection.
-      db_set_active();
+       \Drupal\Core\Database\Database::setActiveConnection();
       // Now actually do a check.

Minor correction :
\Drupal\Core... lines are having an extra space before the code.

Kindly correct these as well. Thanks !!

gaurav.kapoor’s picture

gaurav.kapoor’s picture

Status: Needs work » Needs review
JayKandari’s picture

Status: Needs review » Reviewed & tested by the community

Patch #10 Looks good.
Thanks !

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

The meta issue reads: "...replace all usages of the function (except tests for the function itself)...". LoggingTest is the only test calling db_set_active(), it's not exactly specifically testing the global function but as it's the only test, I'm not sure what we want.

xjm’s picture

Issue tags: +DrupalCampNJ2017
Sharique’s picture

In my opinion we should replace LoggingTest instance also.
Replace full namespace call, as namespace is already in use statement.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

voleger’s picture

Status: Needs review » Reviewed & tested by the community

Patch #15 still applying. Tested on 8.6.x.
Looks good

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This patch should add an @trigger_error() to db_set_active() so people can discover they need to replace the calls. See https://www.drupal.org/core/deprecation
This will also prove we've replaced all the usages in core.

harsha012’s picture

Status: Needs work » Needs review
FileSize
3.94 KB

fixed as per #19

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/includes/database.inc
@@ -335,6 +335,10 @@ function db_transaction($name = NULL, array $options = []) {
+  @trigger_error('db_set_active() is deprecated in Drupal 8.0.x and will be
+  removed before Drupal 9.0.0. Use
+  \Drupal\Core\Database\Database::setActiveConnection() instead.
+  https://www.drupal.org/project/drupal/issues/2848812.', E_USER_DEPRECATED);

The message should all be on one line otherwise it won't flow right when displayed in logs etc...

Also it would nice to add a test of the db_set_active() function to \Drupal\KernelTests\Core\Database\RegressionTest(). You'll need to add @group legacy and @expectedDeprecation deprecation message to the new test.

harsha012’s picture

Status: Needs work » Needs review
FileSize
4.9 KB

change recored - https://www.drupal.org/node/2944084
fixed as per #21

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/includes/database.inc
    @@ -335,6 +335,7 @@ function db_transaction($name = NULL, array $options = []) {
    +  @trigger_error('db_set_active() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Database\Database::setActiveConnection() instead.https://www.drupal.org/node/2944084.', E_USER_DEPRECATED);
    

    This should end with ... instead. See https://www.drupal.org/node/2944084.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php
    @@ -57,4 +57,16 @@ public function testDBIndexExists() {
    +    $this->assertSame(TRUE, db_set_active(), 'Returns true for active connection.');
    +    $this->assertSame(FALSE, db_set_active(), 'Returns false for non-active connection.');
    

    The docs are

       * @return string|null
       *   The previous database connection key.
       */
      final public static function setActiveConnection($key = 'default') {
    

    So I can't see how this works.

harsha012’s picture

Status: Needs work » Needs review
FileSize
5.04 KB

improved the test

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php
@@ -113,11 +113,11 @@ public function testEnableMultiConnectionLogging() {
     $queries1 = Database::getLog('testing1');

+++ b/core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php
@@ -57,4 +59,16 @@ public function testDBIndexExists() {
+    $this->assertTrue(db_set_active($get_active_db), 'Database connection is active');

This method returns the key. Let's assert against that rather than truthiness.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
5.08 KB
657 bytes

Addressed the #25 comment on updated patch & also added an interdiff.

voleger’s picture

Status: Needs review » Reviewed & tested by the community

Looks good for me

  • catch committed 2708ff7 on 8.6.x
    Issue #2848812 by harsha012, gaurav.kapoor, Yogesh Pawar, Sharique,...
catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x, thanks!

gaurav.kapoor’s picture

Great!!. I hope other issues like these will be pushed as well.

voleger’s picture

#31 +1
There are alot issues like this (db_* replacements) that should be synchronized after each commit.
So If some of the other issues are RTBC it would be great to review and commit to saving the time to track changes at the related issues.
Anyway, thanks)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.