Download & Extend

Postgres numeric check needs to happen after precision check in schema generation

Project:Drupal core
Version:7.x-dev
Component:postgresql database
Category:bug report
Priority:critical
Assigned:Unassigned
Status:closed (fixed)
Issue tags:PostgreSQL Surge

Issue Summary

The current function:

<?php
 
protected function createFieldSql($name, $spec) {
...
    if (!empty(
$spec['unsigned'])) {
     
$sql .= " CHECK ($name >= 0)";
    }

    if (!empty(
$spec['length'])) {
     
$sql .= '(' . $spec['length'] . ')';
    }
    elseif (isset(
$spec['precision']) && isset($spec['scale'])) {
     
$sql .= '(' . $spec['precision'] . ', ' . $spec['scale'] . ')';
    }

...
  }
?>

So what happens is if you define a schema element of type numeric with a precision and scale specified, and unsigned, you get this :

colname numeric CHECK (colname >= 0)(10, 2)

The patch switches these two so you get the expected result of

colname numeric(10, 2) CHECK (colname >= 0)
AttachmentSizeStatusTest resultOperations
pgsql-unsigned.D7.patch1.09 KBIdleFailed: Failed to install HEAD.View details

Comments

#1

Title:Postgres numeric check needs to happen after precision check» Postgres numeric check needs to happen after precision check in schema generation
Component:database system» postgresql database

Marking as postgres specific

#2

Status:needs review» needs work

This type of things scream for a test. Would it be possible to extend the Schema API tests to include this case?

#3

Let me see what tests are there now, although I don't think testbot uses Postgres.

#4

Added a new test that creates a table with unsigned columns of each number type, and then tries to insert a negative value in all of them. The test is db agnostic.

AttachmentSizeStatusTest resultOperations
pgsql-unsigned-with-test.D7.patch3.36 KBIdleFailed: 11755 passes, 103 fails, 92 exceptionsView details

#5

Status:needs work» needs review

Now let's see if the test bot chokes on this.

#6

Status:needs review» needs work

Not sure what I did, but it looks like the new test broke the Schema API test class so bad that it didn't load (see the detailed results). Will chew through this.

#7

Priority:normal» critical
Status:needs work» reviewed & tested by the community

forgot to call function as a method. fixed, this patch works, Thanks.

AttachmentSizeStatusTest resultOperations
522786-pgsql-unsigned-with-test.D7.patch3.35 KBIdlePassed: 14688 passes, 0 fails, 0 exceptionsView details

#8

Status:reviewed & tested by the community» needs work

Couple of minor things with the test:

+   * Tests creating unsigned columns and that those columns reject
+   * negative values on insert.

Please rephrase so this fits within 80 chars.

+    // set up unsigned schema with just serial column

All comments should be written "With sentence capitalization, ending in a period." There are a few lines like this.

+      'fields' => array('serial_col' => array('type' => 'serial', 'unsigned' => TRUE, 'not null' => TRUE)),
...
+      $col_spec = array('type' => $type, 'unsigned'=> TRUE);
...
+   * @param $col
etc.

We don't abbreviate variable, etc. names. Please spell it out as "column".

#9

Working on it now. Is there a compiled addendum anywhere for test coding standards over and above the standard Drupal ones?

#10

Status:needs work» needs review

Lightly refactored the patch, which included catching a small bug with the inserts.

AttachmentSizeStatusTest resultOperations
522786-pgsql-unsigned-with-test.D7.patch3.53 KBIdleFailed: 12411 passes, 1 fail, 0 exceptionsView details

#11

Also, there's already a D6 version of this patch here: #256001: pgsql driver does not handle unsigned numeric fields

#12

I missed the inline comments before.

AttachmentSizeStatusTest resultOperations
522786-pgsql-unsigned-with-test.D7.patch3.56 KBTest request sentFailed: Invalid PHP syntax in modules/simpletest/tests/schema.test.View details

#13

Status:needs review» needs work

The last submitted patch failed testing.

#14

I'm going to wait a bit and set this back to CNR. The current HEAD of Drupal is broken, it seems.

#15

OK, sorted out the update, but the same three asserts fail on a clean checkout of HEAD. They're all string comparisons, but as far as I can tell the text has not changed:

In modules/system/system.test, lines 118-122:

<?php
    $this
->assertText(t('The configuration options have been saved.'), t('Modules status has been updated.'));

   
// Check that hook_modules_installed and hook_modules_enabled hooks were invoked and check tables.
   
$this->assertText(t('hook_modules_installed fired for aggregator'), t('hook_modules_installed fired.'));
   
$this->assertText(t('hook_modules_enabled fired for aggregator'), t('hook_modules_enabled fired.'));
?>

The module is enabled just fine, and every other assert passes. I'll check back tomorrow to set this to CNR, as I am certain the patch is not at fault.

#16

Status:needs work» needs review

is testbot fixed now?

#17

Oh, and I think this was the cause of the last fails: #554106: Module enable/disable tests failing on testing servers

#18

Just marked #256001: pgsql driver does not handle unsigned numeric fields postponed on this one, since that is the same patch for Drupal 6.

#19

Status:needs review» reviewed & tested by the community

I like it. Lets get this in.

#20

Status:reviewed & tested by the community» needs work

The last submitted patch failed testing.

#21

Status:needs work» needs review

It looks like another test failed before it got to the ones in here.

#22

Josh Waihi requested that failed test be re-tested.

#23

Status:needs review» reviewed & tested by the community

according to http://qa.drupal.org/pifr/test/19124 #12 passed the test. Lets get this in.

#24

Status:reviewed & tested by the community» fixed

Great! Thanks for the expanded test coverage.

Committed to HEAD.

#25

Status:fixed» needs work

Hm. I just got an e-mail about PHP syntax errors in 'schema.inc' and this patch is the only culprit I can think of. Rolled back.

Any ideas?

#26

Status:needs work» reviewed & tested by the community

strange, PHP parses these files over CLI fine (they die when PHP tries to find the parent Classes naturally) - I can't see a php syntax error anywhere. I can run Drupal and tests no problem with this Patch applied. I don't think this is your HEAD killer.

#27

Status:reviewed & tested by the community» needs review

#12 is showing up as syntax errors from the bot, we need to resolve that before re-committing.

#28

@catch, then is it a bot issue, not the patch. I can run Drupal tests with the patch applied. If there was a syntax error, that wouldn't be possible would it?

#29

Status:needs review» reviewed & tested by the community

Yep, agreed. Bot seems to be down again, and can't see a re-test button on the patch, so re-uploading (with minor re-roll for offset/fuzz) and setting back to RTBC.

AttachmentSizeStatusTest resultOperations
522786-pgsql-unsigned-with-test.patch3.58 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 522786-pgsql-unsigned-with-test.patch.View details

#30

fix several whitespace issues (no credit please).

AttachmentSizeStatusTest resultOperations
522786-pgsql-unsigned-with-test_30.patch3.25 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 522786-pgsql-unsigned-with-test_30.patch.View details

#31

Version:7.x-dev» 6.x-dev
Status:reviewed & tested by the community» patch (to be ported)

Ok, since testbot is down for the count for lord knows how long, I went ahead and committed this again. I verified manually with php -l that there were no syntax errors, so hopefully testbot agrees when it lives again. :P

Marking down to 6.x.

#32

Version:6.x-dev» 7.x-dev
Status:patch (to be ported)» needs review

Nope, testbot does not :)

db_create_table() and db_add_field() don't have a $ret argument anymore.

The attached patch fixes this, schema tests pass again now.

AttachmentSizeStatusTest resultOperations
fix_test.patch1.07 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_test.patch.View details

#33

Status:needs review» fixed

Thanks! Committed to HEAD.

#34

Nice one Berdir

#35

Status:fixed» closed (fixed)

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

nobody click here