September 2005 - Development - lists.drupal.org

[drupal-devel] [feature] Enabling access control on selfmade blocks for menu.module
by damnsure 05 Sep '05

05 Sep '05

Issue status update for http://drupal.org/node/30054 Post a follow up: http://drupal.org/project/comments/add/30054 Project: Drupal Version: 4.6.3 Component: menu system Category: feature requests Priority: normal Assigned to: damnsure Reported by: damnsure Updated by: damnsure Status: patch (code needs review) Attachment: http://drupal.org/files/issues/menu.module_1.patch (957 bytes) As I'm somehow new to drupal and not 100% sure if I'm doing the right thing please be indulgent. I wrote a patch to enable access control on selfmade blocks for menu.module. Till now it was possible to set blocks to certain pages but not depending on user roles. With this patch you will have an entry for each selfmade block under "access control -> menu modul", each starting with "show". damnsure

3 3

[drupal-devel] [task] Extend db_query()
by Cvbge 04 Sep '05

04 Sep '05

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) 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... 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) ------------------------------------------------------------------------ Sat, 13 Aug 2005 09:43:19 +0000 : Cvbge "DATA" should be "DATE" ------------------------------------------------------------------------ Sun, 14 Aug 2005 00:53:34 +0000 : killes(a)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(a)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(a)www.drop.org Cvbge: AFAIK we don't use NULL values inside the DB in Drupal core.

1 0

[drupal-devel] [task] Extend db_query()
by killes 04 Sep '05

04 Sep '05

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) Cvbge: AFAIK we don't use NULL values inside the DB in Drupal core. 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" ------------------------------------------------------------------------ Sun, 14 Aug 2005 00:53:34 +0000 : killes(a)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(a)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?)

1 0

[drupal-devel] [task] Extend db_query()
by Cvbge 04 Sep '05

04 Sep '05

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) 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?) 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) ------------------------------------------------------------------------ Sat, 13 Aug 2005 09:43:19 +0000 : Cvbge "DATA" should be "DATE" ------------------------------------------------------------------------ Sun, 14 Aug 2005 00:53:34 +0000 : killes(a)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(a)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

1 0

[drupal-devel] Preparing Drupal admin/help documentation for Drupal 4.7
by Charlie Lowe 04 Sep '05

04 Sep '05

After Drupal hits the code freeze for 4.7, we'll need to begin updating the admin/help documentation for 4.7 in a documentation sprint to prepare for the coming stable release. Kieran did a great job this summer at organizing a documentation sprint to create the recent updates for admin/help. We could certainly try to follow this process again of getting everyone organized to review all existing documentation and providing updates where needed. There are a few problems with this process: 1) Reviewing all existing module functionality is a massive undertaking. 2) Module functionality may change slightly during the feature freeze/release candidate cycle, meaning that some admin/help that has already been reviewed may need to re-reviewed. Tracking such changes is difficult. 2) Contributed modules may or may not be updated during the 4.7 feature freeze/release candidate cycle and may be end up with functionality change outside of the time frame of the documentation sprint. 3) Because this requires organizing people into specific tasks, it makes it difficult for anyone to jump in and assist when they have time. Rather than depending again on this hierarchical organizational model, I'd like to propose that admin/help documentation construction and update be built around a distributed, ongoing development model much like the rest of Drupal development. However, it depends in part upon you--the developers--so I'd like to get your feedback on the feasibility and method for this process: 1) Developers who have made changes to core or contributed modules which necessitate changes to existing admin/help will file documentation update requests via the Documentation project. When possible, those documentation requests should contain some description of the changes in functionality to the module. 2) Those who have created new contributed modules can similarly file requests for new admin/help documentation. 3) Anyone may jump in and assist with updating admin/help documentation by examining outstanding issues and assigning the tasks to themselves. 4) During the documentation sprint for the new release, we will push to make sure that sure that all outstanding issues are resolved. Thoughts? Charlie Lowe

1 0

[drupal-devel] [bug] Incorrect access checking for username auto completion
by Morbus Iff 04 Sep '05

04 Sep '05

Issue status update for http://drupal.org/node/24617 Post a follow up: http://drupal.org/project/comments/add/24617 Project: Drupal Version: cvs Component: node.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: drumm Updated by: Morbus Iff Status: patch (code needs work) Dries: I don't see that as a problem - a spider could do the same thing by just hitting /user/0, 1, 2, 3, 4, and so on. Morbus Iff Previous comments: ------------------------------------------------------------------------ Thu, 09 Jun 2005 00:36:55 +0000 : drumm The auto completion for user name on node edit pages checks user_access('administer users') when it should be something more like node_access($node, 'edit'). ------------------------------------------------------------------------ Fri, 10 Jun 2005 15:32:58 +0000 : Thox -1 The current "Authored by" field is only for users "administer nodes" permission. ------------------------------------------------------------------------ Fri, 10 Jun 2005 15:35:53 +0000 : Thox Whoops, administer nodes != administer users. This makes things different. The true permission should be administer nodes... which almost suggests that the autocomplete function should be part of node.module, not user.module. It depends where else the autocomplete is used in the future. ------------------------------------------------------------------------ Fri, 10 Jun 2005 15:57:50 +0000 : killes(a)www.drop.org I think the function should stay in user.module, but node.module should get a menu callback that utilizes it. This is not a problem as user.module is a required module. ------------------------------------------------------------------------ Fri, 10 Jun 2005 19:40:57 +0000 : Thox Attachment: http://drupal.org/files/issues/access.patch (1.55 KB) Attached patch moves the menu entry from user.module into node.module and fixes the permission check. ------------------------------------------------------------------------ Tue, 05 Jul 2005 18:24:15 +0000 : Dries Say we wanted to make the Author-field on "edit comment" pages editable. I think the permissions would clash, and you'd be able to by-pass permissions if you have access to at least one (because they'd all share the same callback). So, I don't think this solution is sufficiently generic and possibly insecure. Not? ------------------------------------------------------------------------ Tue, 05 Jul 2005 18:55:17 +0000 : moshe weitzman I think some are compounding two separate idea. In my mind, the username autocomplete callback needs a very minimal permission like 'view user profile'. And that callback belongs in user.module. The decision about showing the author field and the responsibility for validating its contents belong to node.module and comment.module (in Dries' example). ------------------------------------------------------------------------ Thu, 07 Jul 2005 08:31:08 +0000 : Thox If the menu item stays in user.module and the permission changed to 'access user profiles', then any users that are 'administer nodes' but not 'access user profiles' would get errors trying to use the autocomplete. The following would work, but I'm not sure if it's the right way to go: <?php 'access' => user_access('access user profiles') || user_access('administer nodes') ?> I think the 'access user profiles' permission is best to use, possibly even going far enough to change the autocomplete field to a plain text field if the user doesn't have the permission. ------------------------------------------------------------------------ Wed, 24 Aug 2005 19:21:39 +0000 : Morbus Iff I'm not entirely sure what Dries is talking about in #5. However, I tend to think that "access user profiles" is the right way to go - I've been testing changing $admin_access to $view_access on NHPR.org, and it seems to work fine. By itself, being able to access those URLs isn't any sort of security concern - the places that depend on it for editing (such as 'administer nodes') is controlled by a much stronger permission. Thox's comment in #7 would be a problem, true, but I'm trying to figure out when someone would have 'administer nodes' permissions without having the ability to view a user profile (if they're able to edit the node of someone else, shouldn't they know as much public information about that user as possible?). ------------------------------------------------------------------------ Thu, 01 Sep 2005 18:13:47 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_24617_autocompleteperms.patch (793 bytes) This is the currently running code I have over on NHPR.org. ------------------------------------------------------------------------ Sun, 04 Sep 2005 13:43:22 +0000 : Dries The problem with Thox' patch is that it doesn't work when editing comments; some people might have 'adminster comments' permission, but not 'administer nodes' permission. The problem with Morbus' patch is that everyone can access all usernames. (Example: http://nhpr.org/user/autocomplete/a)

1 0

[drupal-devel] [bug] Incorrect access checking for username auto completion
by Dries 04 Sep '05

04 Sep '05

Issue status update for http://drupal.org/node/24617 Post a follow up: http://drupal.org/project/comments/add/24617 Project: Drupal Version: cvs Component: node.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: drumm Updated by: Dries -Status: patch (ready to be committed) +Status: patch (code needs work) The problem with Thox' patch is that it doesn't work when editing comments; some people might have 'adminster comments' permission, but not 'administer nodes' permission. The problem with Morbus' patch is that everyone can access all usernames. (Example: http://nhpr.org/user/autocomplete/a) Dries Previous comments: ------------------------------------------------------------------------ Thu, 09 Jun 2005 00:36:55 +0000 : drumm The auto completion for user name on node edit pages checks user_access('administer users') when it should be something more like node_access($node, 'edit'). ------------------------------------------------------------------------ Fri, 10 Jun 2005 15:32:58 +0000 : Thox -1 The current "Authored by" field is only for users "administer nodes" permission. ------------------------------------------------------------------------ Fri, 10 Jun 2005 15:35:53 +0000 : Thox Whoops, administer nodes != administer users. This makes things different. The true permission should be administer nodes... which almost suggests that the autocomplete function should be part of node.module, not user.module. It depends where else the autocomplete is used in the future. ------------------------------------------------------------------------ Fri, 10 Jun 2005 15:57:50 +0000 : killes(a)www.drop.org I think the function should stay in user.module, but node.module should get a menu callback that utilizes it. This is not a problem as user.module is a required module. ------------------------------------------------------------------------ Fri, 10 Jun 2005 19:40:57 +0000 : Thox Attachment: http://drupal.org/files/issues/access.patch (1.55 KB) Attached patch moves the menu entry from user.module into node.module and fixes the permission check. ------------------------------------------------------------------------ Tue, 05 Jul 2005 18:24:15 +0000 : Dries Say we wanted to make the Author-field on "edit comment" pages editable. I think the permissions would clash, and you'd be able to by-pass permissions if you have access to at least one (because they'd all share the same callback). So, I don't think this solution is sufficiently generic and possibly insecure. Not? ------------------------------------------------------------------------ Tue, 05 Jul 2005 18:55:17 +0000 : moshe weitzman I think some are compounding two separate idea. In my mind, the username autocomplete callback needs a very minimal permission like 'view user profile'. And that callback belongs in user.module. The decision about showing the author field and the responsibility for validating its contents belong to node.module and comment.module (in Dries' example). ------------------------------------------------------------------------ Thu, 07 Jul 2005 08:31:08 +0000 : Thox If the menu item stays in user.module and the permission changed to 'access user profiles', then any users that are 'administer nodes' but not 'access user profiles' would get errors trying to use the autocomplete. The following would work, but I'm not sure if it's the right way to go: <?php 'access' => user_access('access user profiles') || user_access('administer nodes') ?> I think the 'access user profiles' permission is best to use, possibly even going far enough to change the autocomplete field to a plain text field if the user doesn't have the permission. ------------------------------------------------------------------------ Wed, 24 Aug 2005 19:21:39 +0000 : Morbus Iff I'm not entirely sure what Dries is talking about in #5. However, I tend to think that "access user profiles" is the right way to go - I've been testing changing $admin_access to $view_access on NHPR.org, and it seems to work fine. By itself, being able to access those URLs isn't any sort of security concern - the places that depend on it for editing (such as 'administer nodes') is controlled by a much stronger permission. Thox's comment in #7 would be a problem, true, but I'm trying to figure out when someone would have 'administer nodes' permissions without having the ability to view a user profile (if they're able to edit the node of someone else, shouldn't they know as much public information about that user as possible?). ------------------------------------------------------------------------ Thu, 01 Sep 2005 18:13:47 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_24617_autocompleteperms.patch (793 bytes) This is the currently running code I have over on NHPR.org.

1 0

[drupal-devel] [bug] function format_date uses gmdate() incorrectly
by thehunmonkgroup 04 Sep '05

04 Sep '05

Issue status update for http://drupal.org/node/29681 Post a follow up: http://drupal.org/project/comments/add/29681 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: thehunmonkgroup Reported by: thehunmonkgroup Updated by: thehunmonkgroup Status: patch (code needs review) i'm fairly certain that my logic is correct here. the php doc on gmdate() is not totally clear, but i think that if you read it over closely, you'll see that it's the other way around--gmdate() is actually the one that adjusts the timestamp passed. i would recommend writing some short scripts to test out how gmdate() and date() actually operate(that's what i had to do!)--i think you'll see that what happens is gmdate() takes the timestamp passed, applies the server timezone offset, and returns the (formatted) result as GMT. that's what my tests have shown, and for the format_date function, this operation is incorrect--the goal of the function is to simply make the timestamp passed into a human-readable date. no adjustment is necessary. thehunmonkgroup Previous comments: ------------------------------------------------------------------------ Thu, 25 Aug 2005 22:04:51 +0000 : thehunmonkgroup Attachment: http://drupal.org/files/issues/format_date.patch (817 bytes) function format_date in common.inc is using gmdate() incorrectly. the function is trying to format a local timestamp, and GMT conversion is incorrect in this case. date() should be used instead. bug also exists in 4.6, and this patch should apply there, too. haven't checked the earlier versions of drupal... ------------------------------------------------------------------------ Sat, 27 Aug 2005 04:47:39 +0000 : thehunmonkgroup switching to patch status ------------------------------------------------------------------------ Sat, 03 Sep 2005 23:05:37 +0000 : clydefrog Are you sure about the logic here? I've tried to fix timezone issues before, and I always get confused. My reading of the documentation makes me think your patch is wrong: If the timestamp has already been converted to a localized time, then gmdate() should be used because gmdate() does no timezone adjustments (it does not care what PHP's timezone settings are). Using date() is incorrect because it does care what PHP's timezone settings are.

1 0

[drupal-devel] Disaster Relief: Projects we need help with
by Kieran Lal 04 Sep '05

04 Sep '05

Hello, there are several efforts ongoing to provide technical assistance to the Relief Effort. For a general overview, read David Geilhufe blog here: http://blog.social-source.com We have created a Katrina development list which you can subscribe to if you are reading this from a forwarded email: katrinadev- subscribe(a)civicspacelabs.org I see several efforts going on. Let me list them in order of priority as I understand them and then you all can correct me. 1)schema/dictionary for missing person data. http://zesty.ca/ katrina/- Lead Ping Yee and Jon Plax 2)Get an RSS standard. http://www.afallstardjs.com/test.xml -Jon Plax 3)Identify data sources: http://del.icio.us/tag/peoplefinder. Volunteers needed to move this list: http://neworleansnetwork.org/ node/22 to the peoplefinder tag. John Lebowsky and Ethan Zuckerman. http://www.omidyar.net/group/katrinarefugee/news/0/ 4)We need data: David NewBerger:thegeekguy@gmail.com and Tony Chang: tony(a)ponderer.org are working on crawlers. Crawlers should post their efforts here: http://www.omidyar.net/group/katrinarefugee/news/2/ 5)Accomodation matcher. Michael Haggerty is coding away on modifying event finder to help. I have posted a accommodation spec here: http://neworleansnetwork.org/node/29 6)Remotely acquired data can be posted over HTTP POST to the New Orleans Network CiviCRM contact database. Dave Greenberg. http:// neworleansnetwork.org/node/30 7)http://neworleansnetwork.org a Drupal/CivicSpace site with CiviCRM customized as a people finder application. 8)Chris Holland and Josh Kleinpeter are looking for a dedicated crawler that they can point at a list of people sites. If you have a search appliance you can donate some cycles to please contact them. If you can please post to the katrinadev list and let us know what you can do. Cheers, Kieran

1 0

[drupal-devel] [bug] function format_date uses gmdate() incorrectly
by clydefrog 04 Sep '05

04 Sep '05

Issue status update for http://drupal.org/node/29681 Post a follow up: http://drupal.org/project/comments/add/29681 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: thehunmonkgroup Reported by: thehunmonkgroup Updated by: clydefrog Status: patch (code needs review) Are you sure about the logic here? I've tried to fix timezone issues before, and I always get confused. My reading of the documentation makes me think your patch is wrong: If the timestamp has already been converted to a localized time, then gmdate() should be used because gmdate() does no timezone adjustments (it does not care what PHP's timezone settings are). Using date() is incorrect because it does care what PHP's timezone settings are. clydefrog Previous comments: ------------------------------------------------------------------------ Thu, 25 Aug 2005 22:04:51 +0000 : thehunmonkgroup Attachment: http://drupal.org/files/issues/format_date.patch (817 bytes) function format_date in common.inc is using gmdate() incorrectly. the function is trying to format a local timestamp, and GMT conversion is incorrect in this case. date() should be used instead. bug also exists in 4.6, and this patch should apply there, too. haven't checked the earlier versions of drupal... ------------------------------------------------------------------------ Sat, 27 Aug 2005 04:47:39 +0000 : thehunmonkgroup switching to patch status

1 0