[drupal-devel] [task] Extend db_query()
Cvbge
drupal-devel at drupal.org
Tue Sep 13 16:31:58 UTC 2005
Issue status update for
http://drupal.org/node/17656
Post a follow up:
http://drupal.org/project/comments/add/17656
Project: Drupal
Version: cvs
Component: database system
Category: tasks
Priority: normal
Assigned to: killes at www.drop.org
Reported by: killes at www.drop.org
Updated by: Cvbge
Status: patch (code needs review)
ok, last thing (really ;)): maybe add some information to docs saying
that null is converted to '0' (not to '') ? :)
Cvbge
Previous comments:
------------------------------------------------------------------------
Mon, 21 Feb 2005 12:48:30 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/db-query.patch (2.16 KB)
We should make our database abstraction layer more robust and ensure
that module authors can use it without string manipulations inside the
query. Several queries use implode() to get their arguments into the
query. This is undesirable as we rely on the module author to check the
keys and values of such arrays for exploitation attempts.
I have created the attached patch which shouldbe able to allow us to
not use implode anymore.
A minor problem is that all inserted values will be treated as strings.
This might be a problem with PostgreSQL at least. However, the same
strategy is already used in Drupal core without any complaints I know
of.
Summary: This patch will alow us to simplify some code in node.module,
user.module, taxonomy.module and probably others.
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:03:58 +0000 : killes at www.drop.org
It's a patch.
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:19:13 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/db-query_0.patch (2.07 KB)
Squeezed out two lines of code after consultation with Karoly. Adds only
10 loc (plus some docs).
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:23:08 +0000 : chx
Do I need to say +1?
------------------------------------------------------------------------
Thu, 03 Mar 2005 00:15:10 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/db-query_1.patch (2.69 KB)
After some discussion with Adrian at Drupal Con we found out that we do
not know why node_save currently works with pgsql. It currently assumes
that all db columns are strings. It seems to work but we should not rely
on it.
Here is a patch that checks for the type of field that is inserted.
It needs testing.
------------------------------------------------------------------------
Tue, 26 Jul 2005 01:17:04 +0000 : drumm
+1 for making this into an API. I've seen too many hacked together query
builders in Drupal and Contrib. I have not tested.
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:30:29 +0000 : Bèr Kessels
untested. a big +1 for the feature
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:41:52 +0000 : killes at www.drop.org
the patch still applies. the new patch here updates node_save to use it.
Untested.
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:42:24 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/node-%a.patch (926 bytes)
the patch still applies. the new patch here updates node_save to use it.
Untested.
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:45:17 +0000 : Cvbge
"After some discussion with Adrian at Drupal Con we found out that we do
not know why node_save currently works with pgsql.
"
Well, it does not work. Real life exampless of not-working include
flexinode (my experience) and forum module (as someone reported).
Probably also others.
The bug occurs when a normal user (but with necessarily rights) adds a
node and he has no controls (the 'moderated', 'sticky', 'published'
etc). The, in node_load() $node->sticky, $node->moderated (and maybe
others) are set to FALSE (or TRUE, but in this case it works).
When doing printf("%s", FALSE) the FALSE is change to empty string. The
sticky and moderated db fields are numeric and postgresql do not accept
'' (empty string) as a value of integer type.
The result is for example such error:
warning: pg_query(): Query failed: ERROR: invalid input syntax for
integer: "" in ..../includes/database.pgsql.inc on line 45.
user error:
query: INSERT INTO node (title, uid, type, teaser, status, moderate,
promote, sticky, body, comment, created, changed, nid) VALUES('xx',
'2', 'flexinode-1', '<div class="flexinode-body flexinode-1"><div
class="flexinode-image-3"><div class="form-item">
<label>Zdjęcie:</label><br />
<img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [1]
</div>
</div></div>', '1', '', '1', '', '<div class="flexinode-body
flexinode-1"><div class="flexinode-image-3"><div class="form-item">
<label>Zdjęcie:</label><br />
<img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [2]
</div>
< in ..../includes/database.pgsql.inc on line 62.
[1] http://drupal.org/..../pliki//tmp/male.jpg
[2] http://drupal.org/..../pliki//tmp/male.jpg
------------------------------------------------------------------------
Wed, 27 Jul 2005 13:03:14 +0000 : Cvbge
Attachment: http://drupal.org/files/issues/node.module_5.diff (500 bytes)
Here's a quick fix for 4.6
------------------------------------------------------------------------
Wed, 27 Jul 2005 13:10:54 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/node-a.patch (1.8 KB)
the last patch wasn't good.
------------------------------------------------------------------------
Mon, 08 Aug 2005 08:19:14 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/db_api_taxonomy.patch (3.5 KB)
Here is an additional patch for taxonomy.module
it removes the two custom functions that are used for inserts and
updates.
Needs testing.
@Cvbge: shoduld we have a better test than just is_numeric in
_db_argument_type to fix the pgsql problems? We could also try to pass
$value by reference and cast it to int. this function is only used for
updates/inserts so we can afford an additional check.
------------------------------------------------------------------------
Mon, 08 Aug 2005 08:37:25 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/db_query.patch (2.74 KB)
Here's a revised version fo the original patch. _db_argument_type now
gets the $value argument by reference and we check for bools too.
numerics and bools are cast to int.
------------------------------------------------------------------------
Mon, 08 Aug 2005 10:34:36 +0000 : Bèr Kessels
I cannot say anything about performance for I do not know how to
benchmark this. But I tried it, And rewrote some queries in a custom
module, to use it, and like it a lot.
The code becomes cleaner, and better readable. But above all, i see
this idea going into a very interesting and good direction: that of
'more' query builders in core'.
a +1!
------------------------------------------------------------------------
Mon, 08 Aug 2005 10:51:14 +0000 : Cvbge
I'll be talking about postgres db only.
The patch won't work as expected in some cases.
Let's say we have a text/char/other character column and we want to
insert a text that looks like a number. The _db_argument_type() will
recognize the value as a numeric/number etc and will use $key = %d
format. Even not considering printf() numeric conversion this can lead
to incorrect value inserted into db.
Examples of such problematic texts are: 01 (leading 0), 2e2 (scientific
notation), probably also text with trailing/leading white spaces. When
inserting such data into db without quotes the postgres db will think
this is a number and will change 010 to 10, 2e2 to 200 . Here is an
example:
piotr=> CREATE TABLE t(c CHAR(10));
piotr=> INSERT INTO t VALUES (01);
piotr=> INSERT INTO t VALUES (2e2);
piotr=> INSERT INTO t VALUES (-01);
piotr=> INSERT INTO t VALUES (000);
piotr=> SELECT * from t;
c
------------
1
200
-1
0
(4 rows)
I think it's safe to use '%s' everywhere. I could not find it writtent
directly in postgres documentation (although I remember reading it
somewhere some time ago). Postgres will convert the string to correct
integer/numeric/bool value.
------------------------------------------------------------------------
Sat, 13 Aug 2005 01:14:41 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/db_query_0.patch (2.69 KB)
Ok, the patch now does only distinguish between integerss and the rest.
This is needed to decide whether we need quotes.
------------------------------------------------------------------------
Sat, 13 Aug 2005 09:40:59 +0000 : Cvbge
I'm sorry, but I see no reason for this integer check. Can you explain
why there is a need to differentiate between integers and other types?
The only problem I see is with converting bool(FALSE). This value is
converted to empty string ('') and when trying to insert it into
numeric, float, bool, date (probably any column that is not text type)
etc you get error.
One solution would be to check for bool(FALSE) and convert it to 0.
This would work for numeric-like and bool fields. But it would not work
for DATA columns (would produce error). Also, entering '0' into text
column might not be what the author wanted.
But this is more a problem with the coder - if you have integer/date
etc field, insert integer/date/etc data type, not bool!
Side note: the integer-version, the one without '', will be used only
for integers (i.e. ..., -2, -1, 0, 1, 2, ... see
http://php.net/manual/en/language.types.integer.php).
------------------------------------------------------------------------
Sat, 13 Aug 2005 09:43:19 +0000 : Cvbge
"DATA" should be "DATE"
------------------------------------------------------------------------
Sun, 14 Aug 2005 00:53:34 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/db_query_1.patch (2.71 KB)
Ok, another attempt. There is indeed no real reason to use %d for
integers, they work fine as strings and are converted by sprintf to
strings independend of %d or %s. But it is the Right Thing(tm), so I
kept it.
Booleans, however, need the %d formatter in order to be converted to 0
and 1. TRUE to 1 always works, but FALSE to 0 only works with %d.
All tests on php 4.3.x.
------------------------------------------------------------------------
Thu, 18 Aug 2005 21:46:04 +0000 : Cvbge
The part I was objecting previously looks ok now. I haven't tested the
code though.
I still think it'd be nice to change php NULL to real SQL NULLs, but I
don't have how to do it (at least not with current approach). The best
would be if there was (s)printf flag that would just ignore it's
argument...
------------------------------------------------------------------------
Sat, 27 Aug 2005 19:36:04 +0000 : Thomas Ilsche
Attachment: http://drupal.org/files/issues/node_save.patch (907 bytes)
+1, tested
Runs nicely but maybe NULL and floats might need special treatment.
I have attached an INCOMPLETE patch for node_save using the new feature
that also fixes the following issue.
------------------------------------------------------------------------
Tue, 30 Aug 2005 18:13:48 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/node_save_0.patch (3.65 KB)
Here is an updated patch that takes a lot of the code I added to
node_save out again (approx 20 loc). it also fixes a docs glitch in
that function. Needs testing and is to be used with the patch from
http://drupal.org/node/17656#comment-39222
------------------------------------------------------------------------
Sun, 04 Sep 2005 18:13:36 +0000 : Cvbge
A remark about NULL values:
in this patch they are treated as integers and thus allways changed to
'0'.
In existing code the '' was INSERTed into text-like columns (%s) if
didn't check if the variable was set (or did not care).
If they now use %a and still do not check for NULLs the '0' will be
inserted which migh or might not create problems (it won't matter for
if()s, but maybe for other uses?)
------------------------------------------------------------------------
Sun, 04 Sep 2005 19:50:58 +0000 : killes at www.drop.org
Cvbge: AFAIK we don't use NULL values inside the DB in Drupal core.
------------------------------------------------------------------------
Sun, 04 Sep 2005 20:56:41 +0000 : Cvbge
You're probably right, then
1. not checking if a variable is set is a bug (and I've already filled
a bug for one of such bugs, can't find it - sticky, moderated etc.
fields were not set at all when not selected when submitting a post)
2. There's already a lot of DEFAULT NULL in the database schema...
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:03:09 +0000 : Cvbge
Hello, NULLs again.
Previously I wrote that NULLs are treated as integers and passed with
%d and converted to 0.
This is of course not true [were I sick when writting it?]. They are
treated as strings and converted to '' (empty string)
This will create similar problems as FALSE. Maybe NULL should be
treated as %d (and converted to 0) after all?
------------------------------------------------------------------------
Tue, 13 Sep 2005 16:06:30 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/db_query_2.patch (2.72 KB)
Ok, I've included is_null() in the condition.
More information about the drupal-devel
mailing list