Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
PostgreSQL says: ERROR: invalid input syntax for integer: ""
when you try to create a new vocabulary and one of the following checkboxes are unchecked: multiple, required, relations & tags. This is because Postgres doesn't accept '' as a zero integer value (while MySQL does).
Attached is a patch which adds the following code to taxonomy.module:
$edit['multiple'] = empty($edit['multiple']) ? (0) : ($edit['multiple']);
$edit['required'] = empty($edit['required']) ? (0) : ($edit['required']);
$edit['relations'] = empty($edit['relations']) ? (0) : ($edit['relations']);
$edit['tags'] = empty($edit['tags']) ? (0) : ($edit['tags']);
If the values are empty, they are set to 0. This solution works well, but I don't know if there is another preferred way to do this. How are int values from checkboxes usually handeled?
Comment | File | Size | Author |
---|---|---|---|
#27 | drupal-head-taxonomy-35125_2_0.patch | 4.75 KB | zoo33 |
#26 | drupal-head-taxonomy-35125_2.patch | 0 bytes | zoo33 |
#25 | drupal-head-taxonomy-35125_1.patch | 4.68 KB | zoo33 |
#19 | drupal-head-taxonomy-35125_0.diff | 1.78 KB | Cvbge |
#4 | taxonomy_cvs_postgres_4.patch | 1.28 KB | zoo33 |
Comments
Comment #1
zoo33 CreditAttribution: zoo33 commentedOK, I found some similar code in the function taxonomy_save_vocabulary() in taxonomy.module where the 'tags' checkbox is handled:
So i removed my previous additions (which were in taxonomy_admin() by the way) and added these three lines to taxonomy_save_vocabulary() instead:
Attached is a new patch.
And here's some info about my setup:
PostgreSQL 7.5.11
PHP 4.3.10
Apache 2.0.54
Drupal CVS, Oct 24 2005
Comment #2
zoo33 CreditAttribution: zoo33 commentedAnother case of the same problem turned up when I started adding terms to the vocabulary. It's the "weight" value that is not specified as '0'.
In the function taxonomy_save_term() I added:
This problem seems to be the same as the one described here: http://drupal.org/node/23690
The attached patch deals with both these issues. There may be more updates as I keep working with my taxonomies...
It would be nice if someone could confirm that I'm not just being an idiot and that this issue actually exists. :) Maybe we should even set the priority to critical?
Comment #3
Cvbge CreditAttribution: Cvbge commentedzoo33, can you please make a patch using -up switches for diff ("unified patch"), as described here: http://drupal.org/patch
Thank you.
Comment #4
zoo33 CreditAttribution: zoo33 commentedRight, thanks for the pointer! :)
Here's another patch.
Any thoughts on the actual code?
Comment #5
Cvbge CreditAttribution: Cvbge commentedThe code looks ok, so if it fixes this bug you can set Status to "ready to be committed"
Comment #6
chx CreditAttribution: chx commentedPjotr, you could have set it as well.
Comment #7
zoo33 CreditAttribution: zoo33 commentedThat's nice.
I'm only wondering if this kind of stuff should be handled by the new Form API's validation mechanism. I'm not so familiar with it, so I don't know. Is taxonomy.module converted to it yet?
Anyway, the patch solves the problem and it doesn't add any new concepts to the code - it just repeats stuff that was already there. Might as well use it for now.
Comment #8
Dries CreditAttribution: Dries commentedIs this a form API regression/bug?
Can't we fix this in the database layer? The PostgreSQL database backend should cast empty %d's to 0, not to ''?
Comment #9
Dries CreditAttribution: Dries commentedThe MySQL backend should do the same, as MySQL does this implicitely. Casting an empty element to a string, where a numeric value is expected, is a bug IMO.
Comment #10
zoo33 CreditAttribution: zoo33 commentedFirst, I'm new to the Drupal APIs and code structure so there's obviously a lot of things I don't know about it, but here are my thoughts:
I agree that it's not ideal to solve a general problem like this in countless places in module code. If there's a general solution to it, that would be better. But since modules construct the actual SQL queries that get sent to the database (don't they?), I guess they also have to make sure the queries are valid (in terms of what PostgreSQL and MySQL accepts).
The way it is now, taxonomy.module generates queries that try to set integer columns to ''. I don't see how Drupal's database layer can change that in any reasonable way.
Maybe the form layer is capable of checking that a certain value is always returned as an integer? The functions that define the forms are: taxonomy_form_vocabulary() and taxonomy_form_term()
If there is no such general solution, this has to be dealt with locally. So, here is the code that constructs the values in INSERT and UPDATE queries:
These two functions know nothing about the columns and their types, so the only way they can distinguish between different data types is by checking the actual PHP value. That means that the values (in $data) have to be the correct types before they get here. An empty string is not an integer, so is_int won't catch it.
(And BTW, why doesn't _taxonomy_prepare_update() check for ints when _taxonomy_prepare_insert() does? Should I add that to the patch?)
On the other hand, taxonomy_save_vocabulary() and taxonomy_save_term() deal with actual column names explicitly, so that's a better place to check for empty strings that correspond to integer columns. This already happens with the 'tags' column - all this patch does is adding similar checks for 'multiple', 'required', 'relations' and 'weight'.
So, should the patch be committed?
Comment #11
Cvbge CreditAttribution: Cvbge commentedPostgresql can't do anything in this case. It gets a value ('') which is not a integer nor can be converted to one, and it's over.
Yes, it is form api or something bug. It should send NULL or FALSE when a number is expected.
See also "Extend db_query()" (http://drupal.org/project/comments/add/35125) for some discussion.
Comment #12
Cvbge CreditAttribution: Cvbge commented"It should NOT send". of course
Comment #13
zoo33 CreditAttribution: zoo33 commentedCvbge, the issue you refer to is here: http://drupal.org/node/17656 (You pasted the wrong url.)
I read the discussion and I like the approach of building queries in one common place. This would replace _taxonomy_prepare_insert() and _taxonomy_prepare_update().
But, each module will still have to supply the values in their correct types. So the patch is still needed as far as I can see, with or without the new db_query().
(A nice feature for module developers would be to integrate form generation, validation and sql building so that each part is aware the each other. One would specify form fields, validation rules and database types once and the conversion would be handled automatically. But that's a kind of big project...)
Comment #14
zoo33 CreditAttribution: zoo33 commentedOK, I've looked into this a bit more, and I can see that the proposed changes to db_query() described in http://drupal.org/node/17656 could be a solution to this problem (as all null values would be converted to 0). It would require that taxonomy.module was reworked to take advantage of the new functionality however. Until that patch gets committed, my patch works as a temporary solution.
Comment #15
Cvbge CreditAttribution: Cvbge commentedThat patch (http://drupal.org/node/17656) is not going to be commited, it seems.
So yes, this bug needs other patch (probably yours).
Comment #16
zoo33 CreditAttribution: zoo33 commentedI'm changing the status to critical as this bug definitely has to be fixed before 4.7 can be released.
We need this patch or some other solution. This module doesn't use the string formatting capabilities of db_query() – that would be another (preferred?) way to solve it, although it would require more changes to the code. I guess that's what Dries was talking about before, I just didn't get it. :)
Comment #17
Cvbge CreditAttribution: Cvbge commentedOTOH, I hope http://drupal.org/node/10407#comment-54381 is going to be commited, which should also fix this error ;)
Comment #18
zoo33 CreditAttribution: zoo33 commentedWouldn't that solution require that taxonomy passed the queries with %-modifiers to db_query()? The problem here I think is that taxonomy.module builds it's own queries by simply implode()ing arrays without first checking for empty values.
Comment #19
Cvbge CreditAttribution: Cvbge commentedYou're right.
I've update the patch so it should be PHP5 safe
Comment #20
zoo33 CreditAttribution: zoo33 commentedYes, that's better. Nice filename standard for the patch too! :)
Comment #21
Dries CreditAttribution: Dries commentedIf I commit that other patch, this code can be simplified, not? There is some explicit 0-check that might be able to remove.
Comment #22
Cvbge CreditAttribution: Cvbge commentedNot in this case. As zoo33 mentioned, this functions do not use full potential of db_query(). Code snippet:
If we change the
db_query('UPDATE {vocabulary} SET '. _taxonomy_prepare_update($data) .' WHERE vid = %d', $edit['vid']);
to something like
db_query("UPDATE {vocabulary} SET name = '%s', ..., weight = %d, ... WHERE vid = %d", $edit['name'], ..., $edit['weight'], ..., $edit['vid']);
then we could drop some (maybe all?) checks.
Comment #23
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedDries, if you had committed http://drupal.org/node/17656 you'd be able to simply remove those special extra functions...
Comment #24
Dries CreditAttribution: Dries commentedCvbge: in that case, I favor to rewrite that code and to make it more Drupal-like.
Comment #25
zoo33 CreditAttribution: zoo33 commentedAlright, here's another patch based on Dries' comments. I've only tested it very briefly.
I have identified 4 calls to
db_query()
that don't use string formatting and thus are vulnerable to type errors. I changed those so that they specify each field explicitly, and use type modifiers, rather than simplyimplode()
ing an array. Two functions became obsolete –_taxonomy_prepare_insert()
and_taxonomy_prepare_update()
– so I removed those.(I can think of other ways to achieve the same thing, for example extending those two functions to handle type modifiers, but I doubt if it's worth it.)
The code is now somewhat less readable than before and maybe a little bit harder to maintain, but the compatibility issues should be gone and I think it conforms better to the way
db_query()
is meant to be used.I made the calls to
db_query()
span over two lines to increase readability – maybe that's a deadly sin, I don't know. Just let me know and I'll change that.Comment #26
zoo33 CreditAttribution: zoo33 commentedSorry, forgot to include Cvbge's PHP5 compatibility changes... We should do:
...rather than...
Comment #27
zoo33 CreditAttribution: zoo33 commentedWhat the... Empty file? Trying again.
Comment #28
Cvbge CreditAttribution: Cvbge commentedI've finally reviewed the patch... uff...
Patch looks good, works as expected (no errors, can add/edit vocabularies/terms).
There is a bug with possibility to add vocabularies/terms without required fields (e.g. empty name, no type selected) or not unique name (but maybe this one is correct). I don't think this bug is related to this patch, so I'll file an issue.
Comment #29
Cvbge CreditAttribution: Cvbge commentedRight, there was already an issue: http://drupal.org/node/35467
Comment #30
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #31
(not verified) CreditAttribution: commented