August 2005 - Development - lists.drupal.org

[drupal-devel] [bug] Users with 'maintain books' or 'edit own book pages' permission cannot delete book pages
by Bèr Kessels 08 Aug '05

08 Aug '05

Issue status update for http://drupal.org/node/21559 Post a follow up: http://drupal.org/project/comments/add/21559 Project: Drupal Version: cvs Component: book.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: clydefrog Updated by: Bèr Kessels Status: patch (code needs review) I would say this is a "by design". Maybe we need to rename the two book permissions into: "create books" and "edit own book pages". None of the other nodes can be deleted by the roles with "edit own Foo" permissions, nor should the books. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Fri, 29 Apr 2005 04:32:19 +0000 : clydefrog Attachment: http://drupal.org/files/issues/delete_own_book_pages.patch (626 bytes) AFAICT, only 'administer nodes' permission allows deletion of book pages. This patch allows users with 'maintain books' or 'edit own book pages' permission to delete book pages. The logic doesn't check if the node has updates pending. Should it? ------------------------------------------------------------------------ Fri, 29 Apr 2005 14:10:22 +0000 : clydefrog It's a patch, duh. ------------------------------------------------------------------------ Fri, 29 Apr 2005 14:21:27 +0000 : rivena If maintain books is a permission that overlaps other permissions, it should say so. For example, /maintain books (create, edit, and delete all book pages)/. While I agree that someone who has been given maintainer status should be able to delete pages, I wonder if 'delete own pages' should be made into a seperate permission. I could see a situation where I would not want someone to be able to delete their own pages. Do these permissions affect things that are not book pages but have been put into books? Anisa. It's /Golden Week/! ------------------------------------------------------------------------ Sat, 30 Apr 2005 03:43:17 +0000 : clydefrog I think it's clear that "maintain books" implies the ability to edit and delete book pages. No other node module provides a "delete own " permission, so it would be inconsistent for book.module to provide it. Can you provide some examples to justify the inconsistency? These permissions do not affect nodes which aren't book pages but are in a book. Those nodes are controlled by their respective type-specific permissions, but only users with "maintain books" permission can add or remove those nodes from books. I believe this is the correct behavior. ------------------------------------------------------------------------ Sat, 30 Apr 2005 11:03:47 +0000 : rivena Hmm... But, you just said that until this patch, it *didn't* include the delete pages permission. Right? So, now you are changing it's behavior, but leaving the wording the same. How will people know? Or does it not matter, because you presume they didn't really know it didn't include delete in the first place? I don't know that I can defend my idea of having a seperate delete own pages permission against a standard of consistency. If all the other edit own permissions include the delete pages permission, then I have no quibbles. :) Except it would be more userfriendly if I knew what the permissions meant before I granted them to someone, without having to look at the code. But I do realize that is completely out of the scope of the patch. :) Where do all these little changes *go*, anyway? The changelog just says things like... refractored the search module. Anisa. ------------------------------------------------------------------------ Sat, 30 Apr 2005 16:04:14 +0000 : clydefrog I believe that "maintain books" permission implies the ability to delete book pages, so this patch fixes that bug. Other node modules include deletion in the "edit own" permission. For example, page.module: <?php if ($op == 'update' || $op == 'delete') { if (user_access('edit own pages') && ($user->uid == $node->uid)) { return TRUE; } } ?> I agree that it would be better if permissions were clearer about what they implied. There was a discussion started by killes [1] a while ago: "One problem with Drupal is the way it handles user permissions. The permissions are attached to the user object but nobody really knows what they do. To find out what a particular permission allows you to do, you often need to have a look at the code. " I don't know if it ever lead to any code. I think the changelog is meant to list the changes since the previous version without too much technical detail. If you want to see every last change, take a look at the cvs messages [2]. [1] http://lists.drupal.org/archives/drupal-devel/2005-04/msg00704.html [2] http://drupal.org/project/cvs/3060 ------------------------------------------------------------------------ Mon, 23 May 2005 17:33:18 +0000 : rivena I've been looking at the book module permissions recently, and did a bit of testing. I wanted to know what was the minimum permissions needed to create a new book. I assumed based on this discussion, and a similar discussion on the drupal support list that this was 'maintain books' and 'administer nodes'. I ran into a problem, then added 'access nodes' to this. So now my user has no other permissions other than 'maintain books', 'administer nodes' and 'access nodes'. You'd think this is more than sufficient to do something simple like create a new book, but in fact, with just these permissions, you do not have access to create a book page. You can edit other pages, and move them into a new book. Once you have create pages permission, you appear to finally have all the permissions needed, edit own pages is not needed. So, since, the point of this patch was that not having delete permission was odd, might I suggest adding create permission to maintain books? ;p Anisa.

1 0

[drupal-devel] [task] Extend db_query()
by Bèr Kessels 08 Aug '05

08 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: Bèr Kessels Status: patch (code needs review) 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! Bèr Kessels 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.

1 0

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

08 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.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. 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.

1 0

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

08 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_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. 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.

1 0

[drupal-devel] Patch & Bug Bingo
by Gerhard Killesreiter 08 Aug '05

08 Aug '05

Hi there! Some of you will already have noticed the new patch bingo function at drupal.org. It will select one of the pending patches for you at random. The idea is that you then review that patch. The function is available at http://drupal.org/patchbingo . It is also linked from the contributors block which you can enable it from your profile. Yesterday I also created a bug bingo function that will draw a random bug for you to verify and maybe fix. This is available at http://drupal.org/bugbingo and also through said block. Please use at your leisure and help getting stuff done. Cheers, Gerhard

1 0

[drupal-devel] [bug] comment preview "Required" is easily bypassed
by Jeremy Andrews 08 Aug '05

08 Aug '05

http://drupal.org/node/28420 Setting "Preview comment" to "Required" does not strictly require that the comment be previewed first. This is being abused by spammers to quickly and efficiently post spam comments. I discovered this after I added a new feature to my new spam module [1] to auto-blacklist spammer IP addresses, allowing me to block comment spammers when they preview a comment and thus preventing them from ever inserting their spam into my database. I configured my comment module to "require" comment previews, and yet found that the comments were slipping past my filter. I finally realized what the spammer is doing is setting $_POST['op'] to 'Post comment', effectively bypassing the preview phase. I'm currently looking for a clean solution to this. At the moment the only idea I have is to generate a token at the preview phase, and validate the token at the post phase. Unfortunately the token would have to be stored in the databse between the preview and the post, which adds overhead. Alternatively, I've considered using a time-based hash which would constantly update depending on the time of day. This could easily be validated without storing anything in the database. If too long has gone between the preview and the post, an additional preview step would be required... The down side here is that the time-based hash would be publically available, and thus the spammer could easily duplicate it in their script. A private key could solve for that, but increases the complexity as it adds a configuration step. I have the feeling I'm missing a simpler, cleaner solution. Suggestions? I have created a bug report here: http://drupal.org/node/28420 -Jeremy [1] http://www.kerneltrap.org/jeremy/drupal/spam/

2 1

[drupal-devel] [bug] .htaccess - issue with rewriting to cgi's
by davek 08 Aug '05

08 Aug '05

Issue status update for http://drupal.org/node/28423 Post a follow up: http://drupal.org/project/comments/add/28423 Project: Drupal Version: 4.6.0 Component: other Category: bug reports Priority: normal Assigned to: Anonymous Reported by: davek(a)davekaufman.net Updated by: davek(a)davekaufman.net -Status: active +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/htaccess_3.patch (375 bytes) The patch in question. davek(a)davekaufman.net Previous comments: ------------------------------------------------------------------------ Mon, 08 Aug 2005 03:43:25 +0000 : davek(a)davekaufman.net After attempting to create a custom rewrite rule that had nothing to do with drupal, I noticed that the following rules in the default .htaccess file were being too greedy and seemingly matching every URL provided: # Rewrite current-style URLs of the form 'index.php?q=x'. RewriteCond %{REQUEST_FILENAME} !-f RewriteCond %{REQUEST_FILENAME} !-d RewriteRule ^(.*)$ index.php?q=$1 [L,QSA] as near as I can determine, the problem arose from the fact that my custom rewrite was dealing with a cgi. Specifically, I was attempting to rewrite a URL of the form: http://domain.tld/finger/username to the form http://domain.tld/cgi-bin/finger.pl?user=username but it wasn't working. After beating my head against it for a couple hours, I discovered that mod_rewrite can be set to log what it's doing, which is when I found the overly greedy rules. The fix (or at least the fix I discovered) is to add the following line to that section of the .htaccess file: RewriteCond %{REQUEST_FILENAME} !.*cgi-bin.* which says, in essence, "any URI that contains the string 'cgi-bin' is exempt from the following RewriteRule" I've included a "patch" containing this change. Rewriting URLs for cgi's is a pretty common thing to do, can someone please verify this issue?

1 0

[drupal-devel] [bug] Some URLs get ?PHPSESSID added to them
by kbahey 08 Aug '05

08 Aug '05

Issue status update for http://drupal.org/node/4109 Post a follow up: http://drupal.org/project/comments/add/4109 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: Eric Scouten Reported by: Eric Scouten Updated by: kbahey Status: patch (code needs review) You are right about the fact that the use of PHPSESSID in the URL on a redirect is totally unnecessary. That line should be removed from the base code altogether, there is no excuse for it. As for url_rewriter.tags, I am not 100% sure, but I tried it with someone who was facing difficulties, and it did not work as far as I can recall. In any case, your solution should go in base. It cannot hurt, unless some host has disabled it. kbahey Previous comments: ------------------------------------------------------------------------ Thu, 13 Nov 2003 00:22:39 +0000 : Eric Scouten Some URLs get ?PHPSESSID=(whatever) added to them. I find this distracting and unnecessary for anonymous users. Submitting patch #147 for consideration to remove this issue. ------------------------------------------------------------------------ Fri, 05 Dec 2003 14:39:25 +0000 : Kjartan This is a local PHP configuration issue. Don't see it as something Drupal should mess with, the less PHP settings Drupal messes with the better. ------------------------------------------------------------------------ Fri, 05 Dec 2003 15:11:33 +0000 : killes(a)www.drop.org Could you elaborate which config setting one has to tweak? ------------------------------------------------------------------------ Fri, 05 Dec 2003 15:18:11 +0000 : Anonymous php_flag session.use_trans_sid off php_flag session.use_only_cookies on The first will disable the automatic addition of the session ID to internal URLs, the second will remove support for passing session IDs in URLs. See http://php.net/session [1] for more info. BTW the above settings are for Apache, in PHP.ini you should not add the php_flag part, and you should use = inbetween the name and value... [1] http://php.net/session ------------------------------------------------------------------------ Mon, 07 Jun 2004 22:42:06 +0000 : shane It seems that Drupal (via the .htaccess file) already "messes with" a bunch of PHP settings. For example, my Drupal install "came with" these messed with settings: php_value register_globals 0 php_value track_vars 1 php_value short_open_tag 1 php_value magic_quotes_gpc 0 php_value magic_quotes_runtime 0 php_value magic_quotes_sybase 0 php_value arg_separator.output "&" php_value session.cache_expire 200000 php_value session.gc_maxlifetime 200000 php_value session.cookie_lifetime 2000000 php_value session.auto_start 0 php_value session.save_handler user php_value session.cache_limiter none php_value allow_call_time_pass_reference On I'm not sure that adding two more "php_value" flags to the stock .htaccess file is going to arguably mess with more than already is. Personally I find it extremely un-professional and very un-clean to have funky PHPSESSID url strings tacked onto a majority of my URLs. I've worked extremely hard to implement clean URLs (above and beyond the already great "clean url" feature of Drupal - I don't like them being turned into muck. I'd vote that these two flags be added to the .htaccess file that is distributed with Drupal. However, I'm uncertain of the overall true impact of making these changes, so barring an technical reason - I would absolutely request this as a feature. I've implemented these in many of my production sites already (just today) - we'll see what impact they have. ------------------------------------------------------------------------ Tue, 08 Jun 2004 02:56:00 +0000 : marky It's a performance thing. You need to remember that while not everyone has edit access to php.ini, those of us that do control our own servers see no need for .htaccess directives that set php options. >From the Apache docs [2]: "Apache will look in every directory for .htaccess files. Thus, permitting .htaccess files causes a performance hit, whether or not you actually even use them! Also, the .htaccess file is loaded every time a document is requested." Ok, I'm lucky in that I control my own server, so I can effectively remove the .htaccess file and move its config options to php.ini and the vhost section of httpd.conf, thus speeding up the responsiveness of my server. But if you don't have that option there's nothing wrong with adding those directives to your directory .htaccess file. That's what it's for - it allows directory specific config options for those that don't have access to their server config. As Kjartan said, "the less PHP settings Drupal messes with the better". I've got to agree - as far as I'm concerned it's already big enough. The "php_value short_open_tag 1", "php_value session.auto_start 0" and "php_value session.cache_limiter none" for example serve little purpose, as they mirror the installation default settings (apart from the last, where the available options are: nocache, private or public). If you don't have the choice, use .htaccess, and add whatever directives you see fit. But please remember that not everyone uses it. Anyway, just my 2 beads. [2] http://httpd.apache.org/docs-2.0/howto/htaccess.html#when ------------------------------------------------------------------------ Tue, 08 Jun 2004 04:38:32 +0000 : cel4145 Why not add them, but commented out with an explanation? When I encountered this problem, I had to spend a little time on drupal.org finding the solution. However, I am familiar with the .htaccess file from doing installs. If it had been included there commented out, I would have probably remembered it. After all, this is much the same way that httpd.conf functions with it's many, optional, commented out configuration options. Just makes it easier when a non-standard configuration option has to be used. Besides, I've also seen many Drupal sites with this problem. I suspect that some of those sites would have implemented these flags if they had been an option in the .htaccess file. And it looks bad. People that visit the sites that don't know any better will assume it's Drupal, not the lack of proper PHP settings. ------------------------------------------------------------------------ Tue, 08 Jun 2004 10:39:27 +0000 : killes(a)www.drop.org It wouldn't hurt if Drupal had a better documented and possibly extended .htaccess file. Those who whish not to use it should really not care... session.save_handler = user should probably included as well. ------------------------------------------------------------------------ Tue, 08 Jun 2004 12:17:06 +0000 : robertDouglass If you use these settings: php_flag session.use_trans_sid off php_flag session.use_only_cookies on won't it make it so that people with cookies disabled cannot have session state? And if the PHPSESSID is in the URL, isn't that an indication that the browser being used refused to take a cookie? I'd really like clarification on this, if anyone knows the answers. ------------------------------------------------------------------------ Tue, 27 Jul 2004 13:25:46 +0000 : JonBob The original issue is definitely by design, as these URLs need the session ID to preserve the session when running under the given PHP settings. A better-documented .htaccess is a different issue entirely. ------------------------------------------------------------------------ Thu, 19 Aug 2004 18:04:13 +0000 : gtoddv It hasn't been mentioned in this thread or anywhere else that I can find, but this session ID issue breaks validation. I don't know is this will cause accessibility issues too. I have tried the .htaccess fix and that doesn't change anything. ------------------------------------------------------------------------ Sat, 06 Nov 2004 07:17:21 +0000 : wazdog The non-validating problem is another problem with your server setup. PHP can be setup to used encoded &'s or not. Your server isn't if the url isn't validating. You'd have to talk to your server admin to fix that. ------------------------------------------------------------------------ Thu, 30 Dec 2004 20:32:35 +0000 : m_freeman2004 This is what you have to include in the .htaccess file: # Fix for ?PHPSESSID in clean URLs php_value session.use_trans_sid 0 php_value session.use_only_cookies 1 # End of fix ------------------------------------------------------------------------ Wed, 01 Jun 2005 19:07:53 +0000 : dr05 The code in .htaccess file: # Fix for ?PHPSESSID in clean URLs php_value session.use_trans_sid 0 php_value session.use_only_cookies 1 # End of fix don't work! Error: "Internal Server Error!" ------------------------------------------------------------------------ Thu, 14 Jul 2005 15:25:22 +0000 : jasonwhat Anyone else looking into this? Can one get rid of the id added onto the url but still keep the sessions going? ------------------------------------------------------------------------ Mon, 25 Jul 2005 06:16:22 +0000 : Eric Scouten dr05, what version of Apache and what OS are you using? I've been using this same patch with no problems on Mac OS X, Linux, and FreeBSD-based servers using both Apache 1.3.x and 2.0.x. Attached patch will apply these changes to .htaccess. (IMHO, this is useful for the majority of users; those who are smart enough to understand the performance issues can comment out these lines.) ------------------------------------------------------------------------ Mon, 25 Jul 2005 06:57:18 +0000 : clydefrog You forgot the patch, Eric. ------------------------------------------------------------------------ Mon, 25 Jul 2005 08:02:26 +0000 : Eric Scouten Attachment: http://drupal.org/files/issues/htaccess-no-session-ids.patch (383 bytes) D'oh! ------------------------------------------------------------------------ Mon, 25 Jul 2005 08:45:48 +0000 : kbahey There are cases where no parameter will fix this issue. On shared hosting systems where you do not have access to php.ini, and the hosting company has setup things in a way that they cannot be overriden. I found this out the hard way, after lobbying for the settings to be included in the init_set() statements in the settings.php prior to 4.6 being released. They are simply ignored on some hosts. On other hosts, you need to have a php.ini of your own, if suphp or SuExec is being used. In some cases, the only way to make things work is to compile your own PHP as a CGI (detailed writeup) [3]. This can have disadvantages too, such some performance impact, as well as excessive CPU utilization (hosting company may not like it). [3] http://baheyeldin.com/drupal/how-to-get-rid-of-phpsessid-in-drupal-and-othe… ------------------------------------------------------------------------ Mon, 25 Jul 2005 08:49:31 +0000 : kbahey Here is some references: issue 21170 [4], and a forum discussion [5]. [4] http://drupal.org/node/21170 [5] http://drupal.org/node/17947 ------------------------------------------------------------------------ Sat, 30 Jul 2005 16:24:47 +0000 : ufku in my case (shared hosting, ini_set() disabled) a php.ini file with only this line in it solved the problem. url_rewriter.tags = '' you should place the php.ini in your site's base directory ------------------------------------------------------------------------ Sun, 07 Aug 2005 12:18:31 +0000 : djnz Attachment: http://drupal.org/files/issues/rewrite_tags.patch (468 bytes) Drupal already attempts to change a number of values using ini_set() in settings.php, including session.use_trans_sid which should cure this problem. Unfortunately as already noted elsewhere [6], session.use_trans_sid only worked in ini_set() up to PHP 4.2.3. Rather than change the whole philosophy and change this setting in .htaccess or php.ini (neither of which work for many hosted set-ups), surely the answer is to correct settings.php as described The attached patch does just that - it is only one line. Note that session ids are still appended to the url when redirecting by Drupal eg after a POST. This is also easily patched [7], but as it is done deliberately (why?) I have left it alone. [6] http://drupal.org/node/17947#comment-36339 [7] http://drupal.org/node/25852#comment-45111 ------------------------------------------------------------------------ Sun, 07 Aug 2005 13:26:17 +0000 : kbahey PHPSESSIDs in the URL are a) ugly, b) negatively affects search engine indexing, and most importantly, c) is a security risk that can enable a malicious user to hijack your session. We should work on a solution to eliminate this once and for all, and for all configurations. The current approach (putting some PHP parameters in settings.php, or instructing users to change them in .htaccess or php.ini) has severe shorcomings: Whatever we do, there are bound to be some configurations that no amount of PHP parameters will cure. Some hosts deny their users the ability to change some or all of the above parameters. I discovered this the painful way with many hosts I have used, and friends and clients I have helped. I think we need to rethink this whole session thing, in a way that we are not dependant on idiosyncracies of various hosts and PHP configurations. Perhaps attempt to set a DRUPAL cookie and not maintain a session unless we are able to set it. This way we know that we will not have a PHPSESSID in the URL. If we are not able to set the cookie, then we allow only anonymous access that is sessionless.. I am not sure if that will work or not: what other options do we have here? ------------------------------------------------------------------------ Sun, 07 Aug 2005 22:02:19 +0000 : djnz I am convinced that ini_set('url_rewriter.tags','') is the answer. I find it hard to believe that any host would go to the trouble to disable this (not just a matter of setting compile time paramaters, you would have to hack the PHP source; and why would they?) - certainly no cPanel host I have used does. The reason the code currently in settings.php doesn't work is not because hosts disable it, it is because PHP does not support it. RTFM. ------------------------------------------------------------------------ Sun, 07 Aug 2005 22:23:59 +0000 : djnz Attachment: http://drupal.org/files/issues/trans_sid.patch (1.4 KB) After further thought, there really is no point in appending the SID to a redirect url, so the attached patch solves this problem too. If anyone has a problem with session IDs in URLs after applying this patch, I'll offer them free hosting myself. Maybe. ;)

1 0

[drupal-devel] [feature] Comment module does not support extension of its forms
by moshe weitzman 08 Aug '05

08 Aug '05

Issue status update for http://drupal.org/node/14708 Post a follow up: http://drupal.org/project/comments/add/14708 Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: tangent Updated by: moshe weitzman -Status: patch (code needs review) +Status: patch (code needs work) it seems that the patch at http://drupal.org/node/28255 is more consistent with nodeapi and more thorough. if someone else agrees, please mark this issue as a dupe of that one. moshe weitzman Previous comments: ------------------------------------------------------------------------ Wed, 22 Dec 2004 23:20:44 +0000 : tangent The comment module currently does not support 3rd party modules being able to add form elements or form validation. This is important for the captcha module to be able to add an extra form item. ------------------------------------------------------------------------ Wed, 22 Dec 2004 23:22:07 +0000 : tangent Attachment: http://drupal.org/files/issues/comment.form.patch (824 bytes) Here is a patch. ------------------------------------------------------------------------ Thu, 23 Dec 2004 11:54:00 +0000 : Anonymous Haven't tested it, but it seems identical to the changes I'd made for my experiments. +1 for this. This is important to have if people want captchas on comments. Other modules that I think can benefit by this: Ratings (rate the post along with commenting on it). ------------------------------------------------------------------------ Thu, 23 Dec 2004 12:15:44 +0000 : moshe weitzman this is a good direction for hook_comment(). for consistency with nodeapi, i propose splitting into 'form pre' and 'form post' operations and also adding a 'view' operation. ------------------------------------------------------------------------ Thu, 23 Dec 2004 14:37:56 +0000 : tangent I agree with the commentapi thing (however it is named or done) for several reasons. In the short term though, in order to use a captcha module immediately, it wouldn't hurt to update the existing code to support the purpose since I suspect the aforementioned redesign will not happen overnight. ------------------------------------------------------------------------ Fri, 24 Dec 2004 06:48:13 +0000 : TDobes There's a similar discussion going on in another issue [1]... one of these should be marked as a duplicate. [1] http://drupal.org/node/13539 ------------------------------------------------------------------------ Fri, 24 Dec 2004 11:51:35 +0000 : stefan nagtegaal Just not to forget: If such a patch like this makes it into core it would be nice to strip down the comment.module untill it is a real api. Removing the comment moderation part into an cmoderation.module would be a good thingtm. This way we make drupal more flexible and light weight... ------------------------------------------------------------------------ Sat, 22 Jan 2005 09:41:25 +0000 : stefan nagtegaal Duplicate of http://drupal.org/node/13539.. ------------------------------------------------------------------------ Sat, 02 Apr 2005 13:59:32 +0000 : escoles I'm not clear on how to apply this patch. The name of the file is "comment.form", not "comment.module"; is that simply an error? Or should I be able to find a resource somewhere with the filename "comment.form"? Related question: If I operate on the assumption that the filename should be "comment.module" and try to run patch, I get the error message "Only garbage was found in the patch input." Does anybody know what that would mean? ------------------------------------------------------------------------ Mon, 04 Apr 2005 12:13:11 +0000 : jotarp(a)www.jotarp.org Attachment: http://drupal.org/files/issues/comment.module_7.patch (674 bytes) Try this. ------------------------------------------------------------------------ Wed, 20 Apr 2005 14:19:54 +0000 : jgoerzen(a)changelog.complete.org I'm running Drupal 4.6.0 with the attached patch 7. It has been causing me all sorts of weird session troubles [2]. Reversing the patch makes the troubles go away. Something is really awry here... [2] http://lists.drupal.org/archives/drupal-support/2005-04/msg00061.html ------------------------------------------------------------------------ Sun, 12 Jun 2005 11:49:52 +0000 : varunvnair Is comment.module_7.patch still causing problems, esp with Drupal 4.6.1? Just wanted to know because I wanted to enable captchas for anon users and was thinking of applying this patch. ------------------------------------------------------------------------ Mon, 18 Jul 2005 11:27:01 +0000 : Hanu Attachment: http://drupal.org/files/issues/comment.module_9.patch (698 bytes) This patch works for me and my 4.6.2 installation. ------------------------------------------------------------------------ Wed, 03 Aug 2005 23:54:43 +0000 : Eaton That above patch (v 9) Works for me on 4.6.2 as well. I'm using it in a new custom module to impose length limits on comments, very very useful. ------------------------------------------------------------------------ Sun, 07 Aug 2005 15:08:13 +0000 : koorneef Attachment: http://drupal.org/files/issues/comment.module_10_0.patch (1.02 KB) Attached patch is for CVS (post 4.6.2, dd: 2005-08-07). Please check the patch for errors. It works on my test site but one never knows ....

1 0

[drupal-devel] [task] Use node_load instead of extra db_query
by robertDouglass 08 Aug '05

08 Aug '05

Issue status update for http://drupal.org/node/27947 Post a follow up: http://drupal.org/project/comments/add/27947 Project: Drupal Version: cvs Component: profile.module Category: tasks Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: robertDouglass Status: patch (code needs review) Attachment: http://drupal.org/files/issues/profile_node_load.txt (803 bytes) Since node_load caches its results and we know that a node is being loaded anyway, it makes no sense to have an extra db query just to determine the user id for the node. robertDouglass

2 1