The current function:

  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)
CommentFileSizeAuthor
#32 fix_test.patch1.07 KBberdir
#30 522786-pgsql-unsigned-with-test_30.patch3.25 KBscor
#29 522786-pgsql-unsigned-with-test.patch3.58 KBcatch
#12 522786-pgsql-unsigned-with-test.D7.patch3.56 KBstormsweeper
#10 522786-pgsql-unsigned-with-test.D7.patch3.53 KBstormsweeper
#7 522786-pgsql-unsigned-with-test.D7.patch3.35 KBjosh waihi
#4 pgsql-unsigned-with-test.D7.patch3.36 KBstormsweeper
pgsql-unsigned.D7.patch1.09 KBstormsweeper

Comments

stormsweeper’s picture

stormsweeper commented
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

damien tournoud’s picture

damien tournoud commented
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?

stormsweeper’s picture

stormsweeper commented

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

stormsweeper’s picture

stormsweeper commented
StatusFileSize
new pgsql-unsigned-with-test.D7.patch3.36 KB

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.

stormsweeper’s picture

stormsweeper commented
Status: Needs work » Needs review

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

stormsweeper’s picture

stormsweeper commented
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.

josh waihi’s picture

josh waihi commented
Priority: Normal » Critical
Status: Needs work » Reviewed & tested by the community
Issue tags: +PostgreSQL Surge
StatusFileSize
new 522786-pgsql-unsigned-with-test.D7.patch3.35 KB

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

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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".

stormsweeper’s picture

stormsweeper commented

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

stormsweeper’s picture

stormsweeper commented
Status: Needs work » Needs review
StatusFileSize
new 522786-pgsql-unsigned-with-test.D7.patch3.53 KB

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

stormsweeper’s picture

stormsweeper commented

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

stormsweeper’s picture

stormsweeper commented
StatusFileSize
new 522786-pgsql-unsigned-with-test.D7.patch3.56 KB

I missed the inline comments before.

Status: Needs review » Needs work

The last submitted patch failed testing.

stormsweeper’s picture

stormsweeper commented

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

stormsweeper’s picture

stormsweeper commented

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:

    $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.

stormsweeper’s picture

stormsweeper commented
Status: Needs work » Needs review

is testbot fixed now?

stormsweeper’s picture

stormsweeper commented

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

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

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

josh waihi’s picture

josh waihi commented
Status: Needs review » Reviewed & tested by the community

I like it. Lets get this in.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

stormsweeper’s picture

stormsweeper commented
Status: Needs work » Needs review

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

Josh Waihi requested that failed test be re-tested.

josh waihi’s picture

josh waihi commented
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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Great! Thanks for the expanded test coverage.

Committed to HEAD.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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?

josh waihi’s picture

josh waihi commented
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.

catch’s picture

catch
he/him
Primary language English
commented
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.

josh waihi’s picture

josh waihi commented

@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?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 522786-pgsql-unsigned-with-test.patch3.58 KB

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.

scor’s picture

scor commented
StatusFileSize
new 522786-pgsql-unsigned-with-test_30.patch3.25 KB

fix several whitespace issues (no credit please).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new fix_test.patch1.07 KB

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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Thanks! Committed to HEAD.

josh waihi’s picture

josh waihi commented

Nice one Berdir

Status: Fixed » Closed (fixed)
Issue tags: -PostgreSQL Surge

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

Issue tags: +PostgreSQL Surge

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.