PostgreSQL: Impossible to change a field to serial, bigserial, or numeric

Can you confirm that this is a bug in D8 as well?

@tim.plunkett, it appears so. Haven't actually tried it, but the same block of code appears in the D8 pgsql database driver, lines 519-521.

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D...

Wow, amazing , someone working on postgresql! I very much like it. Thanks.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.phpundefined
@@ -513,21 +513,21 @@ class Schema extends DatabaseSchema {
-    // We need to typecast the new column to best be able to transfer the data
-    // Schema_pgsql::getFieldTypeMap() will return possibilities that are not
-    // 'cast-able' such as 'serial' - so they need to be casted int instead.
-    if (in_array($spec['pgsql_type'], array('serial', 'bigserial', 'numeric'))) {
-      $typecast = 'int';
+    // Type "serial" is known to PostgreSQL, but *only* during table creation,
+    // not when altering. Because of that, we create it here as an int. After
+    // we create it we manually re-apply the sequence.
+    if (in_array($spec['pgsql_type'], array('serial', 'bigserial'))) {
+      $field_def = 'int';

So this is the only change here, the rest is a variable rename? Fair enough.

I think the asterisks around only are unneeded. I'd swap the double quotes around serial for single quotes, and add them around int, or remove it from serail.

But with them it does pass? Isn't that enough proof?

A.

If your unit tests are broken already, it won't get worse. However, regressing in D8 (ie B) would be.

Re-rolled the patch from #11 and hand-tested. It seems to work for me. Marking as reviewed and tested.

Do the current schema tests we have fail in PostgreSQL without this patch applied? If not let's add the coverage here.

I'm not sure it's a right place to post, but here's a backport of #18 to d7.

Using proper terminology

The D7 patch from #20 fixed the following problem with the Get Locations module, just an FYI:

Update #7102

Failed: PDOException: SQLSTATE[42601]: Syntax error: 7 ERROR: syntax error at or near "(" LINE 1: ...ABLE getlocations_fields ALTER "latitude" TYPE int(10, 6) US... ^: ALTER TABLE {getlocations_fields} ALTER "latitude" TYPE int(10, 6) USING "latitude"::int(10, 6); Array ( ) in db_change_field() (line 3017 of /xxx/includes/database/database.inc).

Patch from #20 works for me. Can this change to RTBC?

Where are the schema tests in D8, mentioned in #19? I was going to try to write a test, but I only found ConnectionTest, which uses a mock database connection. Are there tests for the various database drivers?

Re #26, please see core/modules/system/src/Tests/Database/

Thanks!

Re #26, please see core/modules/system/src/Tests/Database/

Thanks!

OK, two files, one being just tests, which will fail when using PostgreSQL, and the other being the tests plus an updated version of #20 so that it applies to 8.0.x-dev.

Apply to head. Yay, asserting field characteristics doesn't remove the field any more. I should probably make a new issue for the wrong comment about precision and scale I found.

Can't ever remember to change the status.

Nice, thanks @Island Usurper. Even if currently not used in core it's a nice addition for contrib and should help with the D8 upgrade path. I did testing with PostgreSQL and all tests pass, but it seems a bit odd to create 8000+ tables resulting in 25000+ tests for Schema?

Drupal\system\Tests\Database\SchemaTest 25774 passes

Yeah, I thought about that, @bzrudi71, but I eventually decided to go for completeness because I couldn't decide where "enough" was. If someone who is more familiar with testing wants to chime in to say, "We only need to make sure that one combination of precision and scale work. It's safe to assume the rest will too", that'd be great. I can certainly rewrite the patch that way, or someone else can jump in, too.

+1 for

We only need to make sure that one combination of precision and scale work. It's safe to assume the rest will too

:-)

Alright, smaller test it is.

The patch looks good to me.
The only problem for me is the number of new tests. Almost 5700 new tests and 1900 table creations.

Yes, still way to much tests I think ;-)

Talked to catch on IRC and his suggestion would be to limit the number of new tests to 1 or 2.

Took out the different sizes for the int columns, and half of the "not null", "initial", and "default" variations. Now SchemaTest runs with 748 total passes.

We have currently 658 tests in Schema without patch:

Drupal\system\Tests\Database\SchemaTest 658 passes

with patch:

Drupal\system\Tests\Database\SchemaTest 748 passes

The test itself takes just a second or two to complete, so I no longer see a problem with the number of tests. Reviewed the patch but didn't find anything wrong.
+1 for RTBC.

+++ b/core/modules/system/src/Tests/Database/SchemaTest.php
@@ -477,4 +477,92 @@ protected function assertFieldCharacteristics($table_name, $field_name, $field_s
+    $field_specs = array();
+
+    // Test int and float types.
+    foreach (array('int', 'float') as $type) {
+      $base_field_spec = array(
+        'type' => $type,
+        'size' => 'normal',
+      );
+      $variations = array(
+        array('not null' => FALSE),
+        array('not null' => TRUE, 'initial' => 1, 'default' => 7),
+      );
+
+      foreach ($variations as $variation) {
+        $field_specs[] = $variation + $base_field_spec;
+      }
+    }
+
+    // Test numeric types.
+    $precision = 10;
+    $scale = 2;
+
+    $base_field_spec = array(
+      'type' => 'numeric',
+      'scale' => $scale,
+      'precision' => $precision,
+    );
+    $variations = array(
+      array('not null' => FALSE),
+      array('not null' => TRUE, 'initial' => 1, 'default' => 7),
+    );
+
+    foreach ($variations as $variation) {
+      $field_specs[] = $variation + $base_field_spec;
+    }
+
+    foreach ($field_specs as $i => $old_spec) {
+      foreach ($field_specs as $j => $new_spec) {
+        if ($i === $j) {
+          // Do not change a field into itself.
+          continue;
+        }
+
+        $this->assertFieldChange($old_spec, $new_spec);
+      }
+    }
+  }

Can we rewrite this function so that it is a lot more compact and improve its readability.

The first two thirds are just setting up data structures for Schema API. I don't know that there's much that can be done there. It follows the same style and setup as other test functions in that class, so if you think this function is too verbose, maybe there should be a new issue to refactor more of the tests.

Updated and compacted SchemaTest::testSchemaChangeField().

I think all concerns are addressed now and I found nothing obvious during a second review. RTBC and thanks all!

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 6e51cc1 and pushed to 8.0.x. Thanks!

Tested using SQLite - no new fail introduced.

diff --git a/core/modules/system/src/Tests/Database/SchemaTest.php b/core/modules/system/src/Tests/Database/SchemaTest.php
old mode 100755
new mode 100644

Fixed on commit

Here is the patch from #47 ported to D7. I had to include a change assertFieldCharacteristics() that was in #29 because there is a change that hasn't been backported yet, but is necessary for this test to work. Basically, assertFieldCharacteristics() shouldn't drop the field it tests.

Should we find the issue that took care of that and make sure it gets properly backported before dealing with this?

wake up testbot

Rerolled the patch (does not applied anymore).

Sorry, my mistake, wrong file.