August 2005 - Development - lists.drupal.org

[drupal-devel] [task] Extend db_query()
by Cvbge 13 Aug '05

13 Aug '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) "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)

1 0

[drupal-devel] [task] Extend db_query()
by Cvbge 13 Aug '05

13 Aug '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) 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.

1 0

[drupal-devel] [task] Extend db_query()
by killes 13 Aug '05

13 Aug '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) 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.

1 0

[drupal-devel] [bug] Content type editing has an incorrect title
by drumm 13 Aug '05

13 Aug '05

Issue status update for http://drupal.org/node/28785 Post a follow up: http://drupal.org/project/comments/add/28785 Project: Drupal Version: 4.5.0 Component: node.module Category: bug reports Priority: normal Assigned to: drumm Reported by: drumm Updated by: drumm Status: patch (code needs review) Attachment: http://drupal.org/files/issues/node.module_7.diff (1019 bytes) When the content type configuration moved locations in the admin section the surrounding logic did not get updated properly. This shows itself in the page titles. drumm

1 0

[drupal-devel] [task] Make local tasks more themeable
by drumm 13 Aug '05

13 Aug '05

Project: Drupal Version: cvs Component: menu system Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: drumm Status: patch There is currently a lot of code that does some heavy lifting in the local task themeable functions. This is a bit of a simplification. No HTML is changed by this refactoring. To test this I tried making the local tasks hierarchical instead of two unordered lists. This was a easy change to do, with about 3 lines of changes. drumm -- View: http://drupal.org/node/17869 Edit: http://drupal.org/project/comments/add/17869

3 8

[drupal-devel] [task] Remove an unnecessary div in the srearch form
by drumm 13 Aug '05

13 Aug '05

Issue status update for http://drupal.org/node/28784 Post a follow up: http://drupal.org/project/comments/add/28784 Project: Drupal Version: cvs Component: search.module Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: drumm Status: patch (code needs review) Attachment: http://drupal.org/files/issues/search.module_1.diff (842 bytes) Don't need this since there class can be put directly on the form tag. drumm

1 0

[drupal-devel] [feature] Protect more files in .htaccess.
by Uwe Hermann 12 Aug '05

12 Aug '05

Issue status update for http://drupal.org/node/28776 Post a follow up: http://drupal.org/project/comments/add/28776 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Uwe Hermann Updated by: Uwe Hermann Status: patch (code needs review) Attachment: http://drupal.org/files/issues/htaccess_4.patch (895 bytes) The provided patch adds some more files and directories which will be protected by the default Drupal .htaccess file. First, lots of files and directories used by Subversion (svn), as I keep all my Drupal installations under version control (and Drupal might switch to svn later, too). The database/ directory and *.mysql and *.pgsql, because nobody needs to access those from the web. I removed the update/ directory from the list, there's no such directory in CVS or 4.6., so I guess it's obsolete. Uwe Hermann

1 0

[drupal-devel] [feature] introduce a "cron" user to fix various cron related issues
by robertDouglass 12 Aug '05

12 Aug '05

Issue status update for http://drupal.org/node/5380 Post a follow up: http://drupal.org/project/comments/add/5380 Project: Drupal Version: cvs Component: user system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: ax Updated by: robertDouglass Status: patch (code needs review) Why do we need a user besides uid=0 for cron? Just because the project module writes anonymous on certain closed issues? If cron runs with uid=0 most of the time anyway, why don't we just hardcode that in instead of creating a new type of user? robertDouglass Previous comments: ------------------------------------------------------------------------ Fri, 23 Jan 2004 23:33:00 +0000 : ax we currently have some problems caused by the undefined state of $user in the cause of cron runs. sometimes it's "Anonymous", sometimes it picks up the currently logged in user (i think). I stumbled upon this when reading issues closed by "Anonymous" caused by the automatic cron closure when issues are fixed. i always think "isn't it strange that "Anonymous" can close issues just as she feels like?" there are quite some related issues - search this site for "cron user". couldn't a cron user solve these problems? a la if (REQUEST_URI) == cron.php then $user = <cron_user>, with <cron_user> having some special properties such as showing "Automatically" for closed issues, being possible to excluded in statistics, putting the rss author from imported rss nodes into nodes author field, having special access checks etc. i didn't think about this much, but thought it is worth filing here. please discuss, elaborate - and don't flame me if i'm too off ;) ------------------------------------------------------------------------ Wed, 11 Feb 2004 05:04:29 +0000 : Chris Johnson Sounds like a good idea to me. It's one of many things I'd like to change if I get time to work on it. ------------------------------------------------------------------------ Mon, 27 Sep 2004 19:25:42 +0000 : Philippe Couln't we just write this in cron.php: <?php $cronuser = variable_get('cron_cronuser','nobody'); if ($cronuser = 'nobody'){ $user = user_load(array()); //I believe this results in Anonymous?? }else{ $user = user_load(array('name' => $cronuser)); } ?> We can set the variable cron_cronuser in a page like admin/cron. If the site administrator doesn't set a value, cron uses Anonymous for everything (not some user that happens to be logged on). If a valid user name is set, the site admin can give the cron user special privileges. ------------------------------------------------------------------------ Fri, 01 Oct 2004 18:41:16 +0000 : Philippe Setting the user like this in cron.php sends a cookie. (How? Where? Can we prevent this?) So everybody can get "cron" privileges by loading cron.php once. We could fix this by making cron.php accessible only from localhost (in .htaccess). Any ideas? ------------------------------------------------------------------------ Fri, 01 Oct 2004 21:11:37 +0000 : Philippe Attachment: http://drupal.org/files/issues/cron_uid.patch (3.62 KB) This patch lets the admin select a UID for cron in the admin/settings page. The variable name in which this is stored is 'cron_uid'. When cron.php is loaded, it sets a variable $cron_is_running. If this variable is set, no session will be opened (and no cookies will be sent). The cron page will use the Anonymous user (uid==0) if cron_uid is not set. Note that the site admin should create a "cron" user before setting the UID. ------------------------------------------------------------------------ Sun, 13 Mar 2005 19:18:05 +0000 : Chris Johnson Attachment: http://drupal.org/files/issues/cron_uid2.patch (3.19 KB) Previous patch still applies cleanly to HEAD and code algorith looks good on inspection. However, there are some style problems and one ugly bit using StdClass(), so I have re-rolled the patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 19:37:14 +0000 : killes(a)www.drop.org I like the idea, some comments, though: +if (!$cron_is_running) { + session_start(); +} Is this neccessary? 'User ID '.$cron_uid.' does not exist' ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:11:09 +0000 : killes(a)www.drop.org 'User ID '.$cron_uid.' does not exist' < == needs to be made translatable. Setting to active. ------------------------------------------------------------------------ Mon, 14 Mar 2005 05:56:17 +0000 : Chris Johnson Attachment: http://drupal.org/files/issues/cron_uid3.patch (3.29 KB) Ooops. Sorry I missed that text that needed translating. New patch attached with that bit corrected. Yes, I think the if (!$cron_is_running) code is needed to keep the cron user from creating many unneeded sessions. ------------------------------------------------------------------------ Mon, 14 Mar 2005 09:48:51 +0000 : stefan nagtegaal I am all for the idea of introducing a special id for running the cronjobs, but the approach you took here is far from perfect imo.. After looking at your patch, there are several questions and remarks that came up to my mind: I agree with Gerhard that the $cron_is_running is probably unneeded, I think it's better todo: <?php if (!variable_get('cron_busy', true)) { session_start(); } ?> I think that this is much to technical. Severa newbies will probable ask what a user id is, or what cron jobs are.. From both usability and userfriendlyness this patch is a big -1 from me.. I would suggest todo the following: - Make a new user in the db called 'Cron' with a user id -1, or something like that.. That way you don't have to add extra options to the administration, and the way of implementing should be much cleaner. Also does it improves usability... What do you think? ------------------------------------------------------------------------ Mon, 14 Mar 2005 13:11:44 +0000 : Anonymous Hi, The functionality is great, but your interface is not. IMO you should just choose good defaults and leave it with that. Why would users want to choose "UIDs" ofr "cron Jobs". Joe user should not have to think about UIDs and Crons. IT should Just Work[tm]. I know there are lots of places in Drupal where this is not the case either, but we are working on that. So when we introduce new functionality we should do it good at once. Why not go for Stefans suggestion of choosing a cron UID and jsut leave it like that. UIDs are not really used anyway, so you can choose just any UID (just the next one available, most often will be '2'). Bèr ------------------------------------------------------------------------ Mon, 14 Mar 2005 13:45:54 +0000 : javanaut "Make a new user in the db called 'Cron' with a user id -1" Just on a technical note, the database type for uids is "unsigned" and -1 won't work. ------------------------------------------------------------------------ Mon, 14 Mar 2005 19:08:48 +0000 : killes(a)www.drop.org @Chris: I am pretty sue that this patch will not be usefull if the cron user does not get a session. Drupal relies on the session to authenticate the user. If you are worried about too many sessions, you should make sure the session is clossed at the end of the run. his could be done in cron.php. @Stefan, Ber: I think we should introduce just a standard user and assign the "authneticated user" role to him. The set the variable to that uid so we can check which user it is. The problem is that we cannot create the user from database.mysql beacuse the next uid would be No. 1 which is reserved. So we would need to create the user on the first visit to the admin/settings page. We could also use drupal_set_message to inform the admin of this. We should also extent the relevant help text. While we are at it we could make the anon user use the db tabe for its name too and not a variable. ------------------------------------------------------------------------ Mon, 14 Mar 2005 20:33:00 +0000 : Chris Johnson Looks like we are sort of in a corner. Really the cron user should be in the database at install time, but we have to reserve uid 1 for the first user. Maybe there is another way to do that without having the first entry to admin create the cron user? That seems hackish to me. I agree that the anonymous user should also be completely coded into the database, and not a variable. The current methods causes anomolous behaviors. I, in fact, wrote several patches to do that a long time ago (http://drupal.org/node/5639) I looked at updating that patch a couple of months ago, but the new unregistered user comment capability where the user can enter his name made the code a lot more complicated. Maybe I will get motivated to try it again. If only my patch had been applied the first time! :-) ------------------------------------------------------------------------ Mon, 14 Mar 2005 21:21:12 +0000 : Olen How about creating a bunch of "reserved" users (like most linux distributions do)? uid 1 = root uid 2 = cron uid 3 - 8 = reserved for later use uid 9 = anonymous And then let the admin log in as root the first time. We still let him/her change the username of any of these users, so it should not cause to many problems for new installations. Older ones would either just have to live with it, or do a lot of 'UPDATE module_table SET uid=X WHERE uid=2'-queries. ------------------------------------------------------------------------ Mon, 14 Mar 2005 22:33:32 +0000 : Bèr Kessels What about following the linux thing, and start counting at 500 for normal users? That way people will recognise it, and numbers do not matter anyway. I mean, there is really no difference between 4 and 500, other than thats its a different number. 500 makes just as much sense as 10, or 2 or 4. Bèr ------------------------------------------------------------------------ Tue, 15 Mar 2005 12:59:18 +0000 : Philippe The reason why I created the "ugly" user interface, and let the site admin choose a cron-user ID in the settings page is because of backward compatibility with existing installations. (guess what: my site IS an existing installation) Perhaps another option is to give special users a user ID close to 2147483647 (MAXINT). That way it won't interfere with existing users. ------------------------------------------------------------------------ Thu, 16 Jun 2005 23:12:05 +0000 : degerrit At the risk of being publicly humiliated, and in the spirit of "there's no such thing as a stupid question" ... what exactly is the (security?) issue with adding: $user = user_load(array('uid' => 1)); in cron.php? I have two sites with node_privacy_by_role (NPBR) enabled, and cron does not index all the pages as a consequence (except when it is given a user id with full access like above). It took me some puzzling to figure out why the sites seemed stuck at "11% of the site has been indexed" or something similar. Of course, now a non-authenticated user can see excerpts of "private" pages through the search, which could be a big deal. Anyway, being able to choose a search user would solve the problem with NPBR as well it seems.

1 0

[drupal-devel] [bug] your personal contact form link is broken
by tostinni 12 Aug '05

12 Aug '05

Issue status update for http://drupal.org/node/28728 Post a follow up: http://drupal.org/project/comments/add/28728 Project: Drupal -Version: 4.6.0 +Version: cvs -Component: user.module +Component: contact.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: watermark Updated by: tostinni -Status: active +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/contact_help.patch (1.49 KB) This patch correct the malformed url in the Contact module help. tostinni Previous comments: ------------------------------------------------------------------------ Fri, 12 Aug 2005 00:50:29 +0000 : watermark On my drupal site, if I got to my account->edit and scroll down to *Contact settings* I see a link with the name *your personal contact form*. This link goes to http://www.example.com/user/1/contact instead of http://www.example.com?q=user/1/contact. Hope this helps and it is not a duplicate post. Cheers, Mark

1 0

[drupal-devel] [feature] Profile field visiblity should vary by role
by drumm 12 Aug '05

12 Aug '05

Issue status update for http://drupal.org/node/14149 Post a follow up: http://drupal.org/project/comments/add/14149 Project: Drupal Version: cvs Component: profile.module Category: feature requests Priority: normal Assigned to: ejort Reported by: sushi76 Updated by: drumm -Status: patch (code needs review) +Status: patch (code needs work) There needs to be an upgrade path so existing private and public settings are set up ad permissions when people upgrade. Code which becomes unused, such as the old constant definition, should simply be deleted. In scanning through the comments it looks like profiles as nodes came up. This is a bad idea at this point because a lot of work is actively being done on the node system by John with Content Creation Kit. It would be somewhat like building without a solid foundation, or simply making CCK work harder than necessary. I think this would potentially introduce other issues. Any discussion of profiles as nodes should be moved off of this issue so this patch, which appears to not need it can go through. drumm Previous comments: ------------------------------------------------------------------------ Thu, 09 Dec 2004 18:38:09 +0000 : sushi76 Currently there are only these three Options Private field, content only available to privileged users. (Admin users) Public field, content shown on profile page but not used on member list pages. (Everyone, but not in the member list) Public field, content shown on profile page and on member list pages. (Everyone, everywhere) It should be possible to set the visibilty of profile fields by user roles. ------------------------------------------------------------------------ Sun, 06 Mar 2005 20:40:34 +0000 : tulula I agree! If visibility is set by roles OR if the option is left up to the user to display their info at their discretion vs Admin at level of setting up profile fields. ------------------------------------------------------------------------ Mon, 06 Jun 2005 05:03:27 +0000 : ejort I agree that it should be possible to set visibility per role. There is profile data on my site that I want my organisation's committee members to see, but nobody else, there's information that I want only authenticated users to see, but not anonymous users etc. More than that, I think we should be able to set field editability by role also. There is data on my site that only certain people should be able to edit. If we get to this point then this would be a superset of the functionality discussed in the PROFILE_HIDDEN updates [1] thread, but would be much more flexible. [1] http://drupal.org/node/21219 ------------------------------------------------------------------------ Fri, 10 Jun 2005 08:07:49 +0000 : ejort Attachment: http://drupal.org/files/issues/profile.module.role_based_field_permissions.… (12.97 KB) Hi, Here's a patch which implements role based edit / access permissions for profile fields. It allows you to give permissions to each role (and for each field) to: * access a field (for all accounts) * access a field for your own account * edit a field for your account Users with permission 'administer users' can access / edit all fields, and users must have enough permission to edit/access a profile for these extra permissions to come into effect. I believe this allows for more flexibility than the existing profile.module method of PROFILE_PRIVATE and in HEAD PROFILE_HIDDEN. You can eg: * hide some fields from anonymous users, while allowing other roles to still view that info * have fields viewable by only the admin and user who owns the account (like PROFILE_PRIVATE * have fields viewable by some roles, but only editable by the admin * have fields hidden from all except the admin, and editable only by the admin * it will make you more attractive to the opposite sex etc. I'm developing this because I need it for one of my sites, hence, unfortunately this patch is against the 4.6 profile.module. If people think this is a valuable feature for HEAD I can roll a patch against that. Cheers, Eric ------------------------------------------------------------------------ Fri, 10 Jun 2005 09:13:37 +0000 : Bèr Kessels I am not too sure about this. A typical profile contains between five and 10 fields, but twenty fields is not unheard of. This patch will flood the permissions screen unnneccesary. I would opt for the hard way: An advanced profile module, or CRM module, or so. hat module could implemen,t all sorts of advanced levelled permissions. In order to maintain a high level of usability in core, we should try to minimize options and checkboxes. I know from flexinode, which adds similar dynamic permissions, that the interface for permissions is really not up to this big amount of permissions. I really think such things should not be in core. Buty that all said, it /is/ a handy feature :) ------------------------------------------------------------------------ Fri, 10 Jun 2005 09:53:27 +0000 : ejort Yes, unfortunately it can add a large number of permissions to the permissions page. I did consider trying to place the permission selections on each fields edit page instead (similar to the visibility settings), but as a certain eminent author recently wrote* [2]: "please use *only* the permissions page to set permissions, and do not create new permissions-alike settings in any configuration pages " :-) I think that the PROFILE_PRIVATE and PROFILE_HIDDEN (and the PROFILE_PUBLIC_*) settings are currently doing just that, implementing an alternate permissions system which addresses a few use cases, but is ultimately not particularly flexible. Is there any chance that the permissions sections can benefit from some collapsible goodness to alleviate the flooding? I think it's a shame to reduce the systems flexibility because it may create 'too many' permissions settings, especially as it is generally a set once and forget proposition. Perhaps we should look at the underlying UI problem? *That would be Bér :-) [2] http://drupal.org/node/22565#comment-32248 ------------------------------------------------------------------------ Fri, 10 Jun 2005 13:27:15 +0000 : Bèr Kessels Ejort, I know i wrote that. However, this is a little bit different. am not saying you should make a new system for changing the apperance of profile fields. I am only saying that having per-profile-field permissions is: * way OTT for the big mayority of users * extremely cluttering for the permission page (which, essetially is a problem in that interface, not in your "feature") ------------------------------------------------------------------------ Tue, 14 Jun 2005 15:22:41 +0000 : ejort I don't agree that this is extremely off topic for most users. A large number of people using drupal in any business setting will likely find the extra flexibility invaluable (and the use cases popping up in discussions for regarding PROFILE_HIDDEN show that the current approach doesn't cover enough of the possibilities). I've worked on multiple jobs where I had to create contact lists and employee profiles where basic information was accessible to the public (name, work email, profile text), certain details (direct phone numbers) were only visible to internal company employees, other data (mobile phone numbers) were visible only to team leaders, other data (next of kin contact details) were only visible to an admin, and the user wasn't allowed to edit certain items (such as the public profile text). I've had similar needs come up across several jobs, but then again I wrote the patch so of course I needed it! :-) I also believe that the permissions page problem isn't too large an issue. The permissions are created in response to the admin creating profile fields (3 per field, 'access', 'access own', 'edit'), so the page has no extra permissions by default. This also isn't an interface that is accessed on a daily basis, and there is a good visual separation of the sections so it is easy to skim past the longer profile section if that's not what you're interested in. Of course, this is all just my opinion, and I'm a biased observer :-) ------------------------------------------------------------------------ Tue, 14 Jun 2005 15:37:19 +0000 : Bèr Kessels I know its a handy feature, that will be used in some cases. But then it should nbe in an advanced-profile.module. Not in core. ------------------------------------------------------------------------ Tue, 14 Jun 2005 16:12:35 +0000 : judah I would have to disagree with you ber on this one. I have posted some comments on the Hidden Profile update mentioned above. The permission system *should* be apart of core. These shouldn't be broken out into another advanced module. There *shouldn't* be a node_privacy_by_role. This is unheard of in other systems. I have worked on many other cms solutions and permissions is always a part of the design. Should we plaster them on every feature? No. But we should not so easily dismiss this issue. Node privacy by role and field privacy by role should be considered more closely as a core function rather than where it is now. IMHO. Having said that I do understand the need to break things or branch things too. So it is up to you designers to consider this. Assuming this patch gets approved I would suggest you make the role assignment options available on the profile edit page if it's not already. ------------------------------------------------------------------------ Tue, 14 Jun 2005 21:09:23 +0000 : killes(a)www.drop.org judah: Drupal has the benefit that we can use any node access scheme we like by using another module. @all: I think the best thing would eb to make profiles nodes and wait for cck for field based permissions. ------------------------------------------------------------------------ Wed, 15 Jun 2005 11:19:20 +0000 : Bèr Kessels I think an interim solution would be to: * make profiles nodes * use Flexinode if one wished for profiles . otherwise use a dediceated node for this * add per field permissions to flexinode. ------------------------------------------------------------------------ Wed, 15 Jun 2005 14:14:07 +0000 : Boris Mann I think that profile does indeed need to be re-vamped at some point, and making it a node is likely the right way to go -- then we can benefit from all the field-level work that is going on as we head towards CCK. This is a good interim patch, and should likely be applied to HEAD until someone comes up with the one true profile node -- following our motto to commit things a little earlier so that they can get some testing and use, rather than waiting forever. I know we have a need for it right now as well, and so will help keep this up to date. The one issue is the UI for permissions. It should probably be a separate screen, rather than access control. I would put it directly in the per-field edit screen (e.g. admin/setting/profile/edit/1) or be a permission tab (e.g. admin/setting/profile/permissions). ------------------------------------------------------------------------ Wed, 15 Jun 2005 17:22:49 +0000 : fago the profile node sounds good. i think it's also good to have the possibility to define profile fields for different roles only, so that every role has its own profile. i've done recently a site, where i needed very extended profiles - so i wrote a profile.module based on the drupal profile.module which does this and a lot of other customized stuff.. i also introduced profile-fields, which can be edited only by user with "administer users", as mentioned here. i think, you might run into same problem i did: i already mentioned this issue [3] i'm using this patch, i posted there, on this site. otherwise a user could just inject data, with the same name as admin-only-field, in the user-data field, which will be loaded in the user object by user_load(). further profile_load_profile won't overwrite the value.. [3] http://drupal.org/node/21515 ------------------------------------------------------------------------ Wed, 15 Jun 2005 21:44:15 +0000 : dmclark sorry if this is a nOOb question, but I try to apply the patch and get: patch: **** malformed patch at line 9: /**

1 0