Issue status update for
http://drupal.org/node/11071
Post a follow up:
http://drupal.org/project/comments/add/11071
Project: Drupal
-Version: 4.5.2
+Version: cvs
Component: node.module
Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: mathias
Updated by: killes(a)www.drop.org
-Status: active
+Status: patch (code needs review)
This patch still applies. I don't immediately see why this is related to
the revisions patch.
killes(a)www.drop.org
Previous comments:
------------------------------------------------------------------------
Sat, 25 Sep 2004 18:23:13 +0000 : mathias
With the new node-level access permissions, it is entirely possible for
users in the same role, or having the same taxonomy term to edit each
other's nodes. However when this happens, node_validate will transfer
ownership of the node to the user who last edited it. I think this
behavior should be changed so that original authorship is always
maintained unless specifically transferred.
The problem lies in node_validate, here:
$node->uid = $user->uid ? $user->uid : 0;
Since an alteration such as this could introduce an exploit, I'm
wondering what other's feel would be the best solution?
I was working on a role-based editing permissions module (based on
JonBob's nodeperm_role.module) where the author of a node controls
which groups can view/edit their post.
------------------------------------------------------------------------
Sat, 25 Sep 2004 21:34:00 +0000 : mathias
Attachment: http://drupal.org/files/issues/node_perm.patch (655 bytes)
Here is a proposed patch, which would then allow node authors to choose
which users could view/edit their post.
------------------------------------------------------------------------
Tue, 21 Dec 2004 16:42:24 +0000 : moshe weitzman
seems simple enough to me. we ought to protect against unintentially
changing the author, right? +1
------------------------------------------------------------------------
Mon, 27 Dec 2004 09:16:57 +0000 : Dries
Hopefully, this will become easier/clear as soon the revisions patch hit
CVS. Let's revisit this soon.
------------------------------------------------------------------------
Wed, 23 Feb 2005 05:52:26 +0000 : tangent
As requested in this issue [1], it may be desirable for users with the
permission to do so to change the owner of a node.
[1] http://drupal.org/node/17267
------------------------------------------------------------------------
Tue, 08 Mar 2005 20:57:14 +0000 : Dries
Waiting for the node revision patch to land.
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(a)www.drop.org
Reported by: killes(a)www.drop.org
Updated by: killes(a)www.drop.org
Status: patch (code needs review)
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.
killes(a)www.drop.org
Previous comments:
------------------------------------------------------------------------
Mon, 21 Feb 2005 12:48:30 +0000 : killes(a)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(a)www.drop.org
It's a patch.
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:19:13 +0000 : killes(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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"
Issue status update for
http://drupal.org/node/21214
Post a follow up:
http://drupal.org/project/comments/add/21214
Project: Drupal
Version: 4.6.0
Component: theme system
Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: mjr
Updated by: killes(a)www.drop.org
-Status: active
+Status: patch (code needs work)
It's a patch. Please provide in diff -u format and look at the Drupal
code format specs.
killes(a)www.drop.org
Previous comments:
------------------------------------------------------------------------
Sun, 24 Apr 2005 13:21:14 +0000 : mjr
Attachment: http://drupal.org/files/issues/theme.inc_0.diff (926 bytes)
when i tried to mirror a drupal site using wget -m i only the default
style sheet ./misc/drupal.css will be copied.
Reason:
the code to load that style sheet outputs an additional statement to
that style sheet.
The function theme_stylesheet_import does not.
A fix to this is included.
Best regards,
Michael
Hello,
The rewrite of the spam module is out of beta testing, and
ready for widespread use. An upgrade script is now
available. Thank you to everyone that provided feedback
during the development phase. The current release is version
2.0.6, which works with both Drupal 4.5 and Drupal 4.6.
http://www.kerneltrap.org/jeremy/drupal/spam/
If you are using the older spam module hosted at
drupal.org, you are advised to upgrade due to several
important bug fixes. (The old spam module would frequently
mismark nonspam content as spam due to a bug in the bayesian
filter.)
This new version of the module adds many often requested
features, such as the ability to expire spam content,
automatically deleting it after a configurable amount of
time. It has also been optimized for improved performance.
If you are using the trackback module, you will need to
apply the included trackback.patch file (4.6 only).
Features:
* Written in PHP specifically for Drupal.
* Highly configurable.
* Automatically detects and unpublishes spam comments and
other spam content.
* Automatically learns to detect spam in any language using
Bayesian logic.
* Automatically learns and blocks spammer URLs.
* Automatically blacklists IPs of learned spammers,
preventing them from posting additional spam and wasting
database resources.
* Detects repeated postings of the same identical content.
* Detects content containing too many links, or the same
link over and over.
* Supports the creation of custom filters using powerful
regular expressions.
* Can notify the user that his or her content was determined
to be spam, preventing confusion over why their content
doesn't show up.
* Can notify the site administrator in an email when spam is
detected.
* Provides simple administrative interfaces for reviewing
spam content.
* Provides comprehensive logging to offer an understanding
as to how and why content is determined to be or not to be
spam.
The new spam module can be found here:
http://www.kerneltrap.org/jeremy/drupal/spam/
Regards,
-Jeremy
Issue status update for
http://drupal.org/node/5371
Post a follow up:
http://drupal.org/project/comments/add/5371
Project: Drupal
Version: cvs
Component: base system
Category: bug reports
Priority: normal
Assigned to: mathias
Reported by: mathias
Updated by: rbrooks00
Status: patch (code needs review)
+1 for this patch
Having the built in ability to sort tabular data is incredibly useful.
rbrooks00
Previous comments:
------------------------------------------------------------------------
Fri, 23 Jan 2004 16:07:37 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays.patch (652 bytes)
Tablesort tries its best to rebuild the URI from which it was called.
The problem is when $_REQUEST holds arrays such as $edit. Tablesort
will process this as:
example.com?edit=Array
This can be problematic since many module conditions rely on whether or
not the $edit array is set. My patch disallows tablesort to append
"array $_REQUEST variables" to it's new query string, since it serves
no purpose.
------------------------------------------------------------------------
Sat, 31 Jan 2004 11:00:20 +0000 : Dries
I'd like to hear Moshe's take on this as I'm not sure this is the proper
fix. I'd think that the tablesort code knows exactly what fields it
should add to the URL.
------------------------------------------------------------------------
Sat, 31 Jan 2004 13:24:36 +0000 : moshe weitzman
I don't think this is the proper fix either. For example,
- browse to the Drupal.org issue search page [1].
- press Search button
- On the results page, look at the links in the table header and the
pager. The query state gets passed along by tablesort. This patch would
break this behavior.
Changing Status to 'Active', since this patch shouldn't be applied
without modification.
[1] http://drupal.org/project/drupal/issues/search
------------------------------------------------------------------------
Sat, 31 Jan 2004 17:01:56 +0000 : mathias
I'm pretty sure the project system is hacking the request object just
like I am before sending it to tablesort:
<?php
if ($_SERVER['REQUEST_METHOD'] == 'POST' && is_array($_POST['edit'])) {
foreach ($_POST['edit'] as $key => $value) {
if (!empty($value) && in_array($key, $fields)) {
$query->$key = $value;
$_POST[$key] = is_array($value) ? implode(',', $value) :
$value;
}
}
unset($_POST['edit'], $_POST['op']);
}
?>
Basically, we're both moving the edit array into separate name / value
pairs and then destroying it before handing it off to tablesort. I
don't understand what the benefit is of passing an array into a GET
query string since the key/value pairs aren't retained. We could set an
option in tablesort to ask it to decompose the array into it's key/value
pairs for us and pass them back in the query string. For example an
edit array such as:
<?php
$edit['a'] => 1
$edit['b'] => 2
would have a query string of:
&edit_a=1&edit_b=2
instead of the currently meaningless:
&edit=Array
?>
------------------------------------------------------------------------
Sat, 31 Jan 2004 17:12:57 +0000 : Goba
You mean:
<?php
$edit['a'] => 1
$edit['b'] => 2
would have a query string of:
&edit[a]=1&edit[b]=2
instead of the currently meaningless:
&edit=Array
?>
This reuses the wonderful array handling provided by PHP (as long as
nested arrays are handled properly).
------------------------------------------------------------------------
Sun, 01 Feb 2004 15:02:25 +0000 : Kjartan
Indeed project module hacks the request data. I don't really like this
as a general fix as it is a hack, but for now it is the best way. I
dont think the original patch will break this as it just ignors
arrays(). Project module loops through the array and converts them to
normal array string elements; $_POST['edit']['status'] = value becomes
$_POST['status'] = value, then I think I unset the edit array to clean
up the urls.
------------------------------------------------------------------------
Thu, 05 Feb 2004 13:42:16 +0000 : moshe weitzman
After feedback from Matias and Kjartan, I now consider this patch to be
desireable
------------------------------------------------------------------------
Thu, 12 Aug 2004 15:10:30 +0000 : killes(a)www.drop.org
Patch doesn't apply anymore.
------------------------------------------------------------------------
Fri, 08 Oct 2004 04:14:30 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_0.patch (2.55 KB)
This patch resolves the hacks project module and the ecommerce package
use to rebuild the $_REQUEST arrays when implementing paged resultsets
and/or tablesorting. I know some other developers have ran into this
bug as well usually after form submissions.
Tablesort will now fully handle multi-dimensional $_REQUEST arrays and
convert them to a valid querystring and preserve user-submitted data.
------------------------------------------------------------------------
Fri, 08 Oct 2004 04:34:12 +0000 : moshe weitzman
very nicely done ... bonus points for converting pager to use the new
array2uri() function ... tablesort's poor handling of arrays frustrated
me in a recent client project.
this will simplify almost all cases where a form post leads to a
sortable "results" table, such as in project.module.
------------------------------------------------------------------------
Fri, 15 Oct 2004 14:01:42 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_1.patch (2.99 KB)
Moved the new array2uri function to live with its array2* counterparts
in common.inc and cleaned up the function documentation a wee bit.
------------------------------------------------------------------------
Tue, 21 Dec 2004 16:43:33 +0000 : moshe weitzman
this is a nice code consolidation IMO
------------------------------------------------------------------------
Wed, 22 Dec 2004 10:50:33 +0000 : Dries
This patch adds more code than it removes.
That aside, it would be nice if the function's PHPdoc comments were
extended. Looking at the patch, it is easy to see how/when this
function is being used, but reading the PHPdoc comments it is not. I'd
add a simple example and some context information. Also, it is unclear
what $current_key is used for without inspecting the function's code.
For readability, maybe rename $array to $attributes? If that makes
sense, maybe rename 'array2uri' to 'attributes2uri'? Which makes me
wonder: how does this stack with the existing drupal_attributes()
function?
Do we really need the recursive array handling?
------------------------------------------------------------------------
Sun, 13 Mar 2005 20:25:06 +0000 : killes(a)www.drop.org
Doesn't apply anymore.
------------------------------------------------------------------------
Tue, 12 Jul 2005 22:49:07 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_2.patch (3.19 KB)
Updating this patch since I've been bitten by this issue again. The new
array2uri() function really shines for tablesorting and/or paging
search results after a $_POST request. The only coding change involved
for developers is to use $_REQUEST['edit'] and $_REQUEST['op'] instead
of $_POST['edit'] and $_POST['op'] within their function callback where
this functionality is desired. As an added benefit these pages could
also be bookmarked.
Dries, I've updated the documentation to hopefully makes things a
little easier to understand. I don't think renaming the $array variable
to $attributes or calling the function attributes2uri instead of
array2uri would make the function and its purpose more transparent.
It's not used for HTML attributes like the drupal_attributes function,
but rather for form submitted data (e.g., the $edit array) that needs
to be tablesorted/paged.
Project module and the ecommerce package still currently hack the
$_POST vars to circumvent this problem. Not sure if other modules are
also doing this.
------------------------------------------------------------------------
Fri, 15 Jul 2005 16:25:46 +0000 : Steven
If we're going to have an array2uri() function, we might as well add
urlencode() calls in there... this would centralize a lot of existing
urlencode() calls, I think.
------------------------------------------------------------------------
Mon, 18 Jul 2005 13:51:56 +0000 : Dries
Not going to commit this yet.
1. Let's wait for Mathias' feedback on Steven's comment.
2. I suggest using query or query_string, not querystring.
3. It is not clear why this can't be part of drupal_attributes($array)?
If there is an important difference, it would be nice to make that
clear in the PHPdoc comments.
------------------------------------------------------------------------
Fri, 22 Jul 2005 03:18:19 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_3.patch (3.23 KB)
Agreed with Steven that this is a logical place to urlencode values.
The patch has been updated accordingly.
There was some IRC discussion as to whether this function should be
called drupal_uri, drupal2uri or just plain ol' array2uri. I think
since this code doesn't make any Drupal specific function calls,
array2uri is the best name.
Which brings up a whole new issue for a whole new thread: Perhaps
drupal_attributes should be renamed array2attributes?
------------------------------------------------------------------------
Mon, 25 Jul 2005 08:50:00 +0000 : Steven
Dries: drupal_attributes is used for outputting attributes of an HTML
tag. This patch is about putting data into the query (GET) part of an
URL. They are completely different.
mathias: couldn't we use this function in drupal_get_destination() as
well ?
------------------------------------------------------------------------
Mon, 25 Jul 2005 08:56:05 +0000 : Steven
By the way, what exactly is the point of this check:
+ if (!strstr($query_string, $key_as_uri)) {
+ $query_string .= '&'. $key_as_uri. '='. urlencode($value);
+ }
It seems to be there to avoid duplicates, but there is no similar check
below. If it is required, it needs a comment to explain it.
------------------------------------------------------------------------
Tue, 26 Jul 2005 17:02:50 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_4.patch (3.39 KB)
drupal_get_destination() now uses array2uri(). I discovered that the use
of strstr(), was to compensate for the function calling itself at times
when it didn't need to. This bug is fixed and strstr() is removed. Good
catch!
------------------------------------------------------------------------
Tue, 26 Jul 2005 18:57:26 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_5.patch (3.58 KB)
Allow a string, NULL or array to be passed into array2uri(), making life
easier on developers per Steven's feedback on IRC.
------------------------------------------------------------------------
Thu, 28 Jul 2005 18:15:44 +0000 : moshe weitzman
thanks for updating this patch, matt. looks good to me. i did not test
it yet though.
------------------------------------------------------------------------
Fri, 29 Jul 2005 18:40:25 +0000 : Steven
Err, passing strings to /array/2uri() is pretty non-sensical and not at
all what I meant. I'll see if I can make a better patch later :P.
------------------------------------------------------------------------
Sat, 13 Aug 2005 18:37:23 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_6.patch (4 KB)
* Further code optimizations.
* Made drupal_get_destination() make better use of array2uri().
Issue status update for
http://drupal.org/node/5371
Post a follow up:
http://drupal.org/project/comments/add/5371
Project: Drupal
Version: cvs
Component: base system
Category: bug reports
Priority: normal
Assigned to: mathias
Reported by: mathias
Updated by: mathias
-Status: patch (code needs work)
+Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_6.patch (4 KB)
* Further code optimizations.
* Made drupal_get_destination() make better use of array2uri().
mathias
Previous comments:
------------------------------------------------------------------------
Fri, 23 Jan 2004 16:07:37 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays.patch (652 bytes)
Tablesort tries its best to rebuild the URI from which it was called.
The problem is when $_REQUEST holds arrays such as $edit. Tablesort
will process this as:
example.com?edit=Array
This can be problematic since many module conditions rely on whether or
not the $edit array is set. My patch disallows tablesort to append
"array $_REQUEST variables" to it's new query string, since it serves
no purpose.
------------------------------------------------------------------------
Sat, 31 Jan 2004 11:00:20 +0000 : Dries
I'd like to hear Moshe's take on this as I'm not sure this is the proper
fix. I'd think that the tablesort code knows exactly what fields it
should add to the URL.
------------------------------------------------------------------------
Sat, 31 Jan 2004 13:24:36 +0000 : moshe weitzman
I don't think this is the proper fix either. For example,
- browse to the Drupal.org issue search page [1].
- press Search button
- On the results page, look at the links in the table header and the
pager. The query state gets passed along by tablesort. This patch would
break this behavior.
Changing Status to 'Active', since this patch shouldn't be applied
without modification.
[1] http://drupal.org/project/drupal/issues/search
------------------------------------------------------------------------
Sat, 31 Jan 2004 17:01:56 +0000 : mathias
I'm pretty sure the project system is hacking the request object just
like I am before sending it to tablesort:
<?php
if ($_SERVER['REQUEST_METHOD'] == 'POST' && is_array($_POST['edit'])) {
foreach ($_POST['edit'] as $key => $value) {
if (!empty($value) && in_array($key, $fields)) {
$query->$key = $value;
$_POST[$key] = is_array($value) ? implode(',', $value) :
$value;
}
}
unset($_POST['edit'], $_POST['op']);
}
?>
Basically, we're both moving the edit array into separate name / value
pairs and then destroying it before handing it off to tablesort. I
don't understand what the benefit is of passing an array into a GET
query string since the key/value pairs aren't retained. We could set an
option in tablesort to ask it to decompose the array into it's key/value
pairs for us and pass them back in the query string. For example an
edit array such as:
<?php
$edit['a'] => 1
$edit['b'] => 2
would have a query string of:
&edit_a=1&edit_b=2
instead of the currently meaningless:
&edit=Array
?>
------------------------------------------------------------------------
Sat, 31 Jan 2004 17:12:57 +0000 : Goba
You mean:
<?php
$edit['a'] => 1
$edit['b'] => 2
would have a query string of:
&edit[a]=1&edit[b]=2
instead of the currently meaningless:
&edit=Array
?>
This reuses the wonderful array handling provided by PHP (as long as
nested arrays are handled properly).
------------------------------------------------------------------------
Sun, 01 Feb 2004 15:02:25 +0000 : Kjartan
Indeed project module hacks the request data. I don't really like this
as a general fix as it is a hack, but for now it is the best way. I
dont think the original patch will break this as it just ignors
arrays(). Project module loops through the array and converts them to
normal array string elements; $_POST['edit']['status'] = value becomes
$_POST['status'] = value, then I think I unset the edit array to clean
up the urls.
------------------------------------------------------------------------
Thu, 05 Feb 2004 13:42:16 +0000 : moshe weitzman
After feedback from Matias and Kjartan, I now consider this patch to be
desireable
------------------------------------------------------------------------
Thu, 12 Aug 2004 15:10:30 +0000 : killes(a)www.drop.org
Patch doesn't apply anymore.
------------------------------------------------------------------------
Fri, 08 Oct 2004 04:14:30 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_0.patch (2.55 KB)
This patch resolves the hacks project module and the ecommerce package
use to rebuild the $_REQUEST arrays when implementing paged resultsets
and/or tablesorting. I know some other developers have ran into this
bug as well usually after form submissions.
Tablesort will now fully handle multi-dimensional $_REQUEST arrays and
convert them to a valid querystring and preserve user-submitted data.
------------------------------------------------------------------------
Fri, 08 Oct 2004 04:34:12 +0000 : moshe weitzman
very nicely done ... bonus points for converting pager to use the new
array2uri() function ... tablesort's poor handling of arrays frustrated
me in a recent client project.
this will simplify almost all cases where a form post leads to a
sortable "results" table, such as in project.module.
------------------------------------------------------------------------
Fri, 15 Oct 2004 14:01:42 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_1.patch (2.99 KB)
Moved the new array2uri function to live with its array2* counterparts
in common.inc and cleaned up the function documentation a wee bit.
------------------------------------------------------------------------
Tue, 21 Dec 2004 16:43:33 +0000 : moshe weitzman
this is a nice code consolidation IMO
------------------------------------------------------------------------
Wed, 22 Dec 2004 10:50:33 +0000 : Dries
This patch adds more code than it removes.
That aside, it would be nice if the function's PHPdoc comments were
extended. Looking at the patch, it is easy to see how/when this
function is being used, but reading the PHPdoc comments it is not. I'd
add a simple example and some context information. Also, it is unclear
what $current_key is used for without inspecting the function's code.
For readability, maybe rename $array to $attributes? If that makes
sense, maybe rename 'array2uri' to 'attributes2uri'? Which makes me
wonder: how does this stack with the existing drupal_attributes()
function?
Do we really need the recursive array handling?
------------------------------------------------------------------------
Sun, 13 Mar 2005 20:25:06 +0000 : killes(a)www.drop.org
Doesn't apply anymore.
------------------------------------------------------------------------
Tue, 12 Jul 2005 22:49:07 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_2.patch (3.19 KB)
Updating this patch since I've been bitten by this issue again. The new
array2uri() function really shines for tablesorting and/or paging
search results after a $_POST request. The only coding change involved
for developers is to use $_REQUEST['edit'] and $_REQUEST['op'] instead
of $_POST['edit'] and $_POST['op'] within their function callback where
this functionality is desired. As an added benefit these pages could
also be bookmarked.
Dries, I've updated the documentation to hopefully makes things a
little easier to understand. I don't think renaming the $array variable
to $attributes or calling the function attributes2uri instead of
array2uri would make the function and its purpose more transparent.
It's not used for HTML attributes like the drupal_attributes function,
but rather for form submitted data (e.g., the $edit array) that needs
to be tablesorted/paged.
Project module and the ecommerce package still currently hack the
$_POST vars to circumvent this problem. Not sure if other modules are
also doing this.
------------------------------------------------------------------------
Fri, 15 Jul 2005 16:25:46 +0000 : Steven
If we're going to have an array2uri() function, we might as well add
urlencode() calls in there... this would centralize a lot of existing
urlencode() calls, I think.
------------------------------------------------------------------------
Mon, 18 Jul 2005 13:51:56 +0000 : Dries
Not going to commit this yet.
1. Let's wait for Mathias' feedback on Steven's comment.
2. I suggest using query or query_string, not querystring.
3. It is not clear why this can't be part of drupal_attributes($array)?
If there is an important difference, it would be nice to make that
clear in the PHPdoc comments.
------------------------------------------------------------------------
Fri, 22 Jul 2005 03:18:19 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_3.patch (3.23 KB)
Agreed with Steven that this is a logical place to urlencode values.
The patch has been updated accordingly.
There was some IRC discussion as to whether this function should be
called drupal_uri, drupal2uri or just plain ol' array2uri. I think
since this code doesn't make any Drupal specific function calls,
array2uri is the best name.
Which brings up a whole new issue for a whole new thread: Perhaps
drupal_attributes should be renamed array2attributes?
------------------------------------------------------------------------
Mon, 25 Jul 2005 08:50:00 +0000 : Steven
Dries: drupal_attributes is used for outputting attributes of an HTML
tag. This patch is about putting data into the query (GET) part of an
URL. They are completely different.
mathias: couldn't we use this function in drupal_get_destination() as
well ?
------------------------------------------------------------------------
Mon, 25 Jul 2005 08:56:05 +0000 : Steven
By the way, what exactly is the point of this check:
+ if (!strstr($query_string, $key_as_uri)) {
+ $query_string .= '&'. $key_as_uri. '='. urlencode($value);
+ }
It seems to be there to avoid duplicates, but there is no similar check
below. If it is required, it needs a comment to explain it.
------------------------------------------------------------------------
Tue, 26 Jul 2005 17:02:50 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_4.patch (3.39 KB)
drupal_get_destination() now uses array2uri(). I discovered that the use
of strstr(), was to compensate for the function calling itself at times
when it didn't need to. This bug is fixed and strstr() is removed. Good
catch!
------------------------------------------------------------------------
Tue, 26 Jul 2005 18:57:26 +0000 : mathias
Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_5.patch (3.58 KB)
Allow a string, NULL or array to be passed into array2uri(), making life
easier on developers per Steven's feedback on IRC.
------------------------------------------------------------------------
Thu, 28 Jul 2005 18:15:44 +0000 : moshe weitzman
thanks for updating this patch, matt. looks good to me. i did not test
it yet though.
------------------------------------------------------------------------
Fri, 29 Jul 2005 18:40:25 +0000 : Steven
Err, passing strings to /array/2uri() is pretty non-sensical and not at
all what I meant. I'll see if I can make a better patch later :P.
Issue status update for http://drupal.org/node/24749
Project: Drupal
Version: cvs
Component: other
Category: bug reports
Priority: minor
Assigned to: Anonymous
Reported by: Axel
Updated by: Axel
Status: patch
Attachment: http://drupal.org/files/issues/prefix-update.patch (297 bytes)
The patch adds prefixing for UPDATE SQL statements in prefix.sh.
Axel
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(a)www.drop.org
Reported by: killes(a)www.drop.org
Updated by: Cvbge
Status: patch (code needs review)
"DATA" should be "DATE"
Cvbge
Previous comments:
------------------------------------------------------------------------
Mon, 21 Feb 2005 12:48:30 +0000 : killes(a)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(a)www.drop.org
It's a patch.
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:19:13 +0000 : killes(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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)
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(a)www.drop.org
Reported by: killes(a)www.drop.org
Updated by: Cvbge
Status: patch (code needs review)
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)
Cvbge
Previous comments:
------------------------------------------------------------------------
Mon, 21 Feb 2005 12:48:30 +0000 : killes(a)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(a)www.drop.org
It's a patch.
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:19:13 +0000 : killes(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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.
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(a)www.drop.org
Reported by: killes(a)www.drop.org
Updated by: killes(a)www.drop.org
Status: patch (code needs review)
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.
killes(a)www.drop.org
Previous comments:
------------------------------------------------------------------------
Mon, 21 Feb 2005 12:48:30 +0000 : killes(a)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(a)www.drop.org
It's a patch.
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:19:13 +0000 : killes(a)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(a)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(a)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(a)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(a)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(a)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(a)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.
