Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test

I like the idea for this issue. We should do the same with unique and non-unique indexes.
The patch looks good to me.
I am missing testing for the table does not exists and testing for a table without a primary key.

Patch looks great! @mondrake can you add the tests requested by #4, then I could RTBC ;-)

I think if we want to do the same for unique keys it should be a new issue.

Awesome, looks great to me! Thanks.

The patch looks good to me as well, I just found two tiny nitpcks :)

1. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -408,6 +408,18 @@ public function fieldExists($table, $column) {
   +   * @param $table
   

   Missing the 'string' type hint.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -791,6 +793,45 @@ protected function assertFieldChange($old_spec, $new_spec, $test_data = NULL) {
   +   * Tests the Schema::findPrimaryKeyColumns method.
   

   We could change this to @covers \Drupal\Core\Database\Schema::findPrimaryKeyColumns.

Could we add testing with more complicated scenario's. Like:

1. A table with a primary key that consist of multiple columns.
2. A table where the first column of the table is not be part of primary key.
3. In combination with adding/removing/changing column(s) from an existing table. And then testing its primary key.

The patch looks good to me. Great work!

Re #13:

In combination with adding/removing/changing column(s) from an existing table. And then testing its primary key.

Let's leave this out for now as this is currently broken on MariaDB and also inconsistent between MySQL/SQLite on the on hand and PostgreSQL on the other hand. See #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers and the latest comments in #2938951: The primary key is not correctly re-created when updating the storage definition of an identifier field for more information. I think we should expand the former issue to be "Fix dropping of columns in a composite primary key on MariaDB and make it consistent across database drivers" and then that one can include that coverage (and be blocked on this one).

Re #12:

1. Correct, but out of scope of this patch. Actually most of the methods are missing the typehint in this test class.

That's not quite true, it's very much in the scope of this patch :) Since it is adding new code, it should conform to the coding standards regardless of the surrounding code.

Hehe, I kind of noticed that already in #2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test but didn't realize I did. I just added a bunch of sort()s in the respective assertions and was done with it. You are right, though, that we should fix this properly ;-) Thanks! And thanks @daffie for insisting on the added tests! Will try to look at Postgres one later (unless someone beats me to it)

@tstoeckler: The honour should go to @wimleers. He is the one with his api-first initiative that we should not only test the "easy path". So we should do the same here. :)

Alright, this should pass in all three database engines now.

For SQLite, #20 was pretty much on the right track, except you fixed the issue for normal indexes not for the primary key. So I adapted your approach for the primary key and it worked. I had to use an explicit ksort() in PHP there, as we cannot sort in SQL directly because we do not want to alter the order of the fields and the primary key information is attached to them directly. I also reverted the interdiff from #20. I think it's correct to retrieve the indexes in the specified order, but if we make that change here we should also add test coverage so I'd rather leave that for a separate issue.

For PostgreSQL, the only place I could find where the order of the primary key is stored is in the i.indkey array column in the order of the array itself. Which the array_position() function we can then extract the correct key for sorting. Since we needed that anyway, I altered the query a bit so that the return value is keyed by the position directly so that we don't need the explicit array_keys() in PHP. I removed the data_type field, but we weren't using that anyway, that was just copied from https://wiki.postgresql.org/wiki/Retrieve_primary_key_columns I guess (but due to the ordering we diverge from that query anyway...)

I just noticed that this is marked against 8.6.x, but this now blocks a number of bugs, so is there a chance we can backport this to 8.5.x?

Apparently array_position() is only PostgreSQL >=9.5 :-(

So I went ahead and re-implemented the ordering in PHP. Since the order is only available (as far as I can tell) in array form, which gets into PHP as a concatenated array, we have to do some annoying string futzing for that. Please feel free to suggest a succincter or more readable version if you can come up with one ;-)

In the drivers for PostgreSQL and SQLite are sorting methods added. I have added a test with a more complicated primary key to see if the one test with a multiple primary key is not a lucky pass. I also added some comments to the tests.

Used the same name twice for creating a table in the test.

Thanks @daffie! The test also looks more readable now. Assuming the tests come back green, maybe we can get an RTBC here? ;-)

Thanks @daffie! The test also looks more readable now.

Taking this as given my tests addition as good enough for RTBC.

The patch looks good.
There is enough testing for the problem.
It is RTBC for me.

There is a follow-up: #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers.

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
@@ -502,13 +502,15 @@ protected function introspectSchema($table) {
-          $schema['primary key'][] = $row->name;
+          $schema['primary key'][$row->pk - 1] = $row->name;

$row->pk - 1 needs a comment. Since this looks pretty odd. I think we need to document that $row->pk contains a number that reflects the primary key order. The number starts from 1 (i guess) so we subtract 1 to make it a zero indexed array. Given that we're doing

ksort($schema['primary key']);

later, I think we should do what we do for postgres and do

ksort($schema['primary key']);
$schema['primary key'] = array_values($schema['primary key']);

And then we don't have to do -1 subtracting.

It feels awkward to be adding API for tests-only but having #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers following is good because that will introduce a usage in non-test code.

I think we need to document that $row->pk contains a number that reflects the primary key order. The number starts from 1 (i guess) so we subtract 1 to make it a zero indexed array.

Yup, that's exactly right. Will fix according to your suggestion.

Here we go.

@alexpott's review has been addressed, back to RBTC :)

He, I wanted to do that! ;-)
+1 for RTBC.

Sorry, I've had another look at the patch and I have a BC concern.

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -408,6 +408,18 @@ public function fieldExists($table, $column) {
+  /**
+   * Finds the primary key columns of a table, from the database.
+   *
+   * @param string $table
+   *   The name of the table.
+   *
+   * @return string[]|false
+   *   A simple array with the names of the columns composing the table's
+   *   primary key, or FALSE if the table does not exist.
+   */
+  abstract public function findPrimaryKeyColumns($table);

This is going to break contributed database drivers. I think we need to think about this a bit more carefully for BC purposes.

I think this is the best solution to minimise the BC break. Back to RTBC.

You could write a Unittest that mocks Connection::driver() to a not by Drupal core supported driver. Calling Schema::findPrimaryKeyColumns() should then result in your RuntimeException being thrown.

Discussed with @catch in #contribute. We agreed that throwing the exception is the right way to go but...

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -408,6 +408,23 @@ public function fieldExists($table, $column) {
+  /**
+   * Finds the primary key columns of a table, from the database.
+   *
+   * @param string $table
+   *   The name of the table.
+   *
+   * @return string[]|false
+   *   A simple array with the names of the columns composing the table's
+   *   primary key, or FALSE if the table does not exist.
+   *
+   * @throws \RuntimeException
+   *   If the driver does not override this method.
+   */
+  public function findPrimaryKeyColumns($table) {
+    throw new \RuntimeException("The '" . $this->connection->driver() . "' database driver does not implement " . __METHOD__);
+  }

We need to document that all non-test usages of this method MUST catch this exception and do the most sensible thing if this not available including continuing with old buggy behaviour because in Drupal 8 we can not guarantee this functionality is available.

Also in reviewing the code we can do the FALSE return when the table does not exist in this implementation.

all non-test usages of this method MUST catch this exception and do the most sensible thing if this not available including continuing with old buggy behaviour

Can you elaborate on this? Even though practically speaking the current lack of this functionality is really an oversight, technically speaking we are adding new functionality here. The chain of issues makes very clear that there really is no way around adding this, but still there will be no current code that is using this function. So I'm not sure what you mean by "continuing with old buggy behavior"? While wrapping each usage of this method in a try-catch is anything from elegant, I'm not actually talking about elegance here. I just really don't understand what to do in such a case. At the end of the day this would mean that even with this patch we are not providing a proper API to fetch the primary key because you still need to somehow deal with the case where you don't know what the primary key is.

That is fine with me, I think in actual real life use-cases - similar to the code in Entity API in the related issues - what you actually want to do is make sure the primary key has a certain state, so instead of asking for the current primary key, it's easy enough to just recreate it to the desired state. I just thought up to this point that the scope of the issue was a different one and that we were actually adding an API function here.

But I guess that means we should mark the function as @internal?

Also in reviewing the code we can do the FALSE return when the table does not exist in this implementation.

Nice catch, good idea!

@tstoeckler if the calling code does not catch the exception and do something sensible - which might be continuing with the buggy present behaviour - then this is a breaking change. Code that didn't halt execution now will and given this exception will occur during low level entity schema operations that might be even worse than the current behaviour.

Sorry, I'm still not getting it.

which might be continuing with the buggy present behaviour

What behavior specifically? As I stated above the method cannot be called right now, so I'm not sure what you mean by that.

So when #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers and #2976493: Creating a table with an explicitly empty primary key is broken on PostgreSQL are fixed and use this new method they have to catch the exception rather than just allowing the exception to surface to the user.

So #2976493: Creating a table with an explicitly empty primary key is broken on PostgreSQL doesn't actually use the new method outside of the tests.

#2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers does use it, but it's a special case: it's using it inside of the database drivers themselves. So there's no need to add a try-catch as the drivers themselves can be sure that it will never be thrown.

What I thought what we were talking about is code outside of tests and outside of the drivers.

Well #52 really makes me question the wisdom of this public API addition then. Since there is no requirement for the addition to the public API. If we decide we want to add this then we do need to add a disclaimer on Schema::findPrimaryKeyColumns() that if this used outside of tests or the drivers themselves then the exception MUST be caught because we cannot guarantee that it is implemented by contrib/custom database drivers in Drupal 8.

So do you think it would be sufficient to declare it @internal? I think that matches pretty well with only using it in tests and in the drivers themselves (and we can also document it, in this case).

The question is then just whether remove the abstract base method entirely and consider it an "implementation detail" of each of the three database drivers in core or whether to keep throwing the exception. I would prefer the latter, but more than anything I would love for this to land, so I'll pretty much comply with anything you and @catch can agree on ;-)

If we are going to make the method Schema::findPrimaryKeyColumns() @internal, are we then going to make it a protected method instead of public?

I'd be +1 on making it protected and using reflection in tests to get access to it and having a default implementation that throws an exception. That way:

• when contributed db drivers run a test and it fails for this they get a great message.
• we don't increase the public API unnecessarily
• contrib/custom are still encouraged to implement but know that not implementing won't break a thing.

So, in other words, this?! ;-)

It all looks good to me. This is what @alexpott would like to happen.

I have only 1 minor documentation point, that can be solved on submit:

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -25,6 +27,8 @@ class SchemaTest extends KernelTestBase {
   public function testSchema() {
+    $schema = Database::getConnection()->schema();

Add between those 2 lines of code the following documentation:

/** @var \Drupal\Core\Database\Schema $schema */

Back to RTBC.

@daffie that's not necessary - \Drupal\Core\Database\Connection::schema() already provides the typehint.

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -791,6 +799,126 @@ protected function assertFieldChange($old_spec, $new_spec, $test_data = NULL) {
+    /** @var \Drupal\Core\Database\Schema $schema */
+    $schema = Database::getConnection()->schema();

In fact the typehint here is not necessary.

Committed 3edde1f and pushed to 8.6.x. Thanks!

diff --git a/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
index 4107cb7aa2..278d75635d 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -803,7 +803,6 @@ protected function assertFieldChange($old_spec, $new_spec, $test_data = NULL) {
    * @covers ::findPrimaryKeyColumns
    */
   public function testFindPrimaryKeyColumns() {
-    /** @var \Drupal\Core\Database\Schema $schema */
     $schema = Database::getConnection()->schema();
     $method = new \ReflectionMethod(get_class($schema), 'findPrimaryKeyColumns');
     $method->setAccessible(TRUE);

Fixed on commit.

@alexpott: Thank you for the explanation.