Posted by stormsweeper on July 17, 2009 at 7:28pm
| 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)| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| pgsql-unsigned.D7.patch | 1.09 KB | Idle | Failed: Failed to install HEAD. | View details |
Comments
#1
Marking as postgres specific
#2
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.
#5
Now let's see if the test bot chokes on this.
#6
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
forgot to call function as a method. fixed, this patch works, Thanks.
#8
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 columnAll 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
Lightly refactored the patch, which included catching a small bug with the inserts.
#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.
#13
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
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
I like it. Lets get this in.
#20
The last submitted patch failed testing.
#21
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
according to http://qa.drupal.org/pifr/test/19124 #12 passed the test. Lets get this in.
#24
Great! Thanks for the expanded test coverage.
Committed to HEAD.
#25
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
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
#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
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.
#30
fix several whitespace issues (no credit please).
#31
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
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.
#33
Thanks! Committed to HEAD.
#34
Nice one Berdir
#35
Automatically closed -- issue fixed for 2 weeks with no activity.