Calling DatabaseSchema::getPrefixInfo() on a non-default connection returns the wrong database - write tests

And this is why calling globals inside objects is bad. :-) Thanks, Dave!

No tests.

Same D8 patch with a test added.

That'll teach me for just copying existing tests and editing.

Updating the clean patch with reroll. Please review.

Looks quite good, however:

+++ b/core/tests/Drupal/Tests/Core/Database/MultiplePrefixTest.php
@@ -0,0 +1,43 @@
+    $this->assertNotIdentical($info1['prefix'], $info2['prefix'], 'Each target connection has a different prefix.');

I would assert that those prefixes are the expected ones. That they are different probably is enough info for assuring this bug is gone, but this way we could avoid different bugs in the future.

Also, it would be awesome to upload a patch with only the test assuring that it test the actual bug. See "Upload a test case that fails" in https://drupal.org/core-gates#testing

Here is the same patch as #12 with a little more testing and the patch with _only_ the tests to show the fault in the original code base.

Already fixed in #2554241: mysql's Schema implementation can only provide information about the default connection..

@Dave Reid: It is going to be fixed for Drupal 7 in #2311305: Schema::getPrefixInfo() return value refers to the wrong database.

Looks like both those issues should have originally been closed as duplicates of this one, but a little late for that unfortunately.

The Drupal 8 patch was committed without tests, so the Drupal 7 backport that's RTBC in the other issue doesn't have tests either. The patch here has tests, so I guess the simplest thing to do is repurpose it to be about adding the tests to both Drupal 8 and 7...

Re-roll

RTBC for the tests, a test-only patch (e.g. with the fix reverted) would be great, too.

RTBC - looks good to me

Still RTBC

Edit: deleted.

Committed and pushed 2c2caccd45da80ec1aecdfa8905bd7d8765b1c2f to 8.2.x and 8bf7d7c to 8.1.x. Thanks!

Setting to patch to ported until the d7 porting issue exists.

diff --git a/core/tests/Drupal/KernelTests/Core/Database/PrefixInfoTest.php b/core/tests/Drupal/KernelTests/Core/Database/PrefixInfoTest.php
index 8032dd2..94c3dd1 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/PrefixInfoTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/PrefixInfoTest.php
@@ -14,7 +14,7 @@ class PrefixInfoTest extends DatabaseTestBase {
   /**
    * Tests that DatabaseSchema::getPrefixInfo() returns the right database.
    */
-  function testGetPrefixInfo() {
+  public function testGetPrefixInfo() {
     $connection_info = Database::getConnectionInfo('default');
     // Copy the default connection info to the 'extra' key.
     Database::addConnectionInfo('extra', 'default', $connection_info['default']);
@@ -54,4 +54,5 @@ function testGetPrefixInfo() {
 
     Database::removeConnection('extra');
   }
+
 }

A couple of CS fixes on commit.

Seems we broke PostgreSQL as well as SQLite bot ;-)
https://dispatcher.drupalci.org/job/php7_sqlite3.8/1380/
https://dispatcher.drupalci.org/job/php7_postgres9.1/1389/

@bzrudi71 yep - weird this had been testing on postgres and sqlite... reverted.

The problem that the tests for PostgreSQL and SQLite are failing is that we are testing the return array from the method \Drupal\Core\Database\Driver\mysql\Schema::getPrefixInfo(). This return array is a keyed array with info about amongst other things the database. The problem is that only for MySQL the key "database" is set and not for the other two by Drupal core supported databases. So testing for the other two databases will fail.

Ready for a good review.

Looks good to me and passes on Postgres / SQlite npw. Thanks daffie!

Hmmm.... okay but then what code can rely on this having the database information? Why does mysql have this info and the other dbs do not?

The database information is added because it is used in Drupal\Core\Database\Driver\mysql\Schema::buildTableNameCondition. That method can easily be rewritten so that it does not need it any more. Or we can fix it the other way around and have all getPrefixInfo method set the database information. @alexpott: What do you think is the best solution.

@daffie yeah I looked into it and agree with your analysis of why - I guess what we have in #46 is the right way to go because otherwise it'd be a change for the mysql implementation. It is a shame these things have got out-of-sync because it makes working with the APIs tricker than it should be.

Committed and pushed 32fa9448c41f0c88e92d35cbb6265d7b7013b1b4 to 8.2.x and d34b1a2 to 8.1.x. Thanks!

FILE: ...l/core/tests/Drupal/KernelTests/Core/Database/PrefixInfoTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 65 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

diff --git a/core/tests/Drupal/KernelTests/Core/Database/PrefixInfoTest.php b/core/tests/Drupal/KernelTests/Core/Database/PrefixInfoTest.php
index 15c89d9..131fb7a 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/PrefixInfoTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/PrefixInfoTest.php
@@ -62,4 +62,5 @@ function testGetPrefixInfo() {
       Database::removeConnection('extra');
     }
   }
+
 }

Fixed on commit.

Setting to patch to ported until the d7 porting issue exists.

Created #2772127: [D7] Calling DatabaseSchema::getPrefixInfo() on a non-default connection returns the wrong database - write tests for D7.