August 2005 - Development - lists.drupal.org

[drupal-devel] [bug] Category selection does not work with Movable Type blogapi
by Dries 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/24030 Post a follow up: http://drupal.org/project/comments/add/24030 Project: Drupal Version: 4.6.2 Component: blogapi.module Category: bug reports Priority: normal Assigned to: walkah Reported by: mrmachine Updated by: Dries Status: patch (code needs review) I committed this patch to DRUPAL-4-6. I won't commit it to HEAD. We need a better fix for this. Dries Previous comments: ------------------------------------------------------------------------ Tue, 31 May 2005 22:38:14 +0000 : mrmachine Using a blog client (linux - either Drivel or BlogTK) and the Moveable Type api, you can retrieve categories and select one for your post, but once the post is submitted, it fails to allocate the category - making the use of a blog tool pretty useless. ------------------------------------------------------------------------ Tue, 31 May 2005 22:45:51 +0000 : laura s I use ecto, set to MoveableType API,but so far I'm not able to repeat this problem. Could it be the client? I was also able to set Drupal's BlogAPI to the MetaWeblog setting and it seemed to work fine -- even with ecto set to MoveableType API. You might be able to try that as a workaround. ------------------------------------------------------------------------ Thu, 02 Jun 2005 12:04:12 +0000 : mrmachine I tried ecto on my son's Windows box, and yes - it does set categories correctly ... too bad there's no linux version but anyway, i tried changing Drupal's blogapi to Metaweblog, and kept Drivel and BlogTK set to Moveable Type API. It didn't work for Drivel, but it did for BlogTK ... thanx for the hint! ------------------------------------------------------------------------ Sun, 05 Jun 2005 14:36:44 +0000 : fflewddur I've been trouble-shooting this issue using w.bloggar on Windows and Drivel on Linux. w.bloggar is able to post using the Movable Type API, but the category selection only works if you click "Post", not "Post and Publish". Drivel, by default, publishes all posts, which is why the category never shows up when using Drivel with Drupal. To post entries with categories to an official Movable Type implementation, the client must first post the entry, then call mt.setPostCategories, and then call mt.publishPost. The mt.publishPost call is what seems to be confusing Drupal, as it doesn't appear to differentiate between a post that has been published with one which has. Movable Type does not publicly display posts which have not been published; it considers them as drafts which the author has not yet finished. Thus, Movable Type servers require the mt.publishPost call. I have Ethereal traces of several example posts to Drupal using both w.bloggar in "post" and "post and publish" mode, as well as Drivel. If you think they'd be useful in debugging, let me know and I'll forward them on to you. Cheers, Todd ------------------------------------------------------------------------ Thu, 09 Jun 2005 03:57:11 +0000 : mrmachine has anyone had a chance to look at this since Todd debugged it? ------------------------------------------------------------------------ Mon, 13 Jun 2005 20:56:58 +0000 : mrmachine can someone have a look at this? ------------------------------------------------------------------------ Tue, 14 Jun 2005 00:05:25 +0000 : walkah Attachment: http://drupal.org/files/issues/blogapi-publish-post.patch (634 bytes) having a look - it seems that mt.publishPost may well have been wiping the term info set by setCategory. As I only have ecto available to me currently, I can't test this patch... but if I understand the debugging work correctly - this should fix it. (the difference being ecto and MarsEdit - the two that I have access to currently - don't call publishPost). please confirm this fixes things... thanks. (I'll test more when I'm home again at the end of the week). ------------------------------------------------------------------------ Tue, 14 Jun 2005 23:06:29 +0000 : mrmachine beauty! this has fixed it for me! thanx shayne ------------------------------------------------------------------------ Wed, 15 Jun 2005 04:29:48 +0000 : baudolino This is not fixed until a decision about whether to commit this patch is made. Reverting this to patch status. ------------------------------------------------------------------------ Sun, 19 Jun 2005 20:25:55 +0000 : Dries Actually, this is an annoying property of node_save() ... I guess you can't use node_invoke_nodeapi($node, 'insert') or node_invoke_nodeapi($node, 'update')? ------------------------------------------------------------------------ Thu, 23 Jun 2005 17:53:32 +0000 : walkah i'm not sure that'd make a difference, would it? isn't the issue that $node->taxonomy is unset? ------------------------------------------------------------------------ Wed, 20 Jul 2005 11:56:50 +0000 : mrmachine i upgraded to 4.6.2 (a tad late though - i'd been hacked), and am now back to the original problem (i.e. - using Drivel or BloGTK, i am unable to set the category - i can choose one, but it doesn't stick) ... i'm guessing it's because this is "not fixed" and so wasn't committed with the 4.6.2 release. am i able to use the same patch to "not fix" 4.6.2? in doing so, i won't mess up any 4.6.2 security patches, will i? shayne ------------------------------------------------------------------------ Sat, 23 Jul 2005 21:26:06 +0000 : mrmachine "am i able to use the same patch to "not fix" 4.6.2? " no. no i can't. i'd really like to test any possible fixes. are there any? ------------------------------------------------------------------------ Mon, 25 Jul 2005 00:18:51 +0000 : mrmachine has anyone managed to test this patch on 4.6.2? i haven't been able to get it working, so i'm back to a patched 4.6.1 blogapi.module ... i just hope this doesn't mean i'm still open to security vulnerabilities of 4.6.1 as my site was defaced only last week ... though i'm sure the vulnerability was only in the xmlrpc.php file.

1 0

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

14 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) Eric, that makes sense. Thanks for the explanation. 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. ------------------------------------------------------------------------ Fri, 12 Aug 2005 15:59:10 +0000 : robertDouglass 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? ------------------------------------------------------------------------ Sun, 14 Aug 2005 07:57:09 +0000 : ejort @robertDouglass: Why do we need a user besides uid=0 for cron? There are genuine use cases for cron running with greater permissions than those provided by uid=0, and having a specific cron user allows the administrator to determine exactly which permissions the cron user gets. For instance I just spent half a day figuring out why my simplenews installation was mailing multiple empty messages to subscribers plus 1 working message. My simplenews mail messages are only accessible by users with the 'authenticated user' role via taxonomy_access, my simplenews install sends for 20 seconds on message submit and then continues sending later via cron. When sending via cron the message body couldn't be accessed by the anonymous user and so an empty message got sent. It was only when a logged in user initiated a send run that a proper message was sent. Without the ability to assign permissions to a cron user, I have to allow access to my newsletters to anonymous users. So, that's my +1 for this functionality. @killes: . So we would need to create the user on the first visit to the admin/settings page. I agree that this variable ('cron_uid') should be set automatically without requiring any admin intervention. Can't we create this user in update.php? The variable defaults to 0 which is the current behaviour in any case. Finally, I think that the logic to not create a session for the cron user should be removed from this patch. Instead we should destroy the session at the end of the cron run. Cheers, Eric

1 0

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

14 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: ejort Status: patch (code needs review) @robertDouglass: Why do we need a user besides uid=0 for cron? There are genuine use cases for cron running with greater permissions than those provided by uid=0, and having a specific cron user allows the administrator to determine exactly which permissions the cron user gets. For instance I just spent half a day figuring out why my simplenews installation was mailing multiple empty messages to subscribers plus 1 working message. My simplenews mail messages are only accessible by users with the 'authenticated user' role via taxonomy_access, my simplenews install sends for 20 seconds on message submit and then continues sending later via cron. When sending via cron the message body couldn't be accessed by the anonymous user and so an empty message got sent. It was only when a logged in user initiated a send run that a proper message was sent. Without the ability to assign permissions to a cron user, I have to allow access to my newsletters to anonymous users. So, that's my +1 for this functionality. @killes: . So we would need to create the user on the first visit to the admin/settings page. I agree that this variable ('cron_uid') should be set automatically without requiring any admin intervention. Can't we create this user in update.php? The variable defaults to 0 which is the current behaviour in any case. Finally, I think that the logic to not create a session for the cron user should be removed from this patch. Instead we should destroy the session at the end of the cron run. Cheers, Eric ejort 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. ------------------------------------------------------------------------ Fri, 12 Aug 2005 15:59:10 +0000 : robertDouglass 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?

1 0

[drupal-devel] [bug] node_validate does not respect group editing
by killes 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/11071 Post a follow up: http://drupal.org/project/comments/add/11071 Project: Drupal -Version: 4.5.2 +Version: cvs Component: node.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: mathias Updated by: killes(a)www.drop.org -Status: active +Status: patch (code needs review) This patch still applies. I don't immediately see why this is related to the revisions patch. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ Sat, 25 Sep 2004 18:23:13 +0000 : mathias With the new node-level access permissions, it is entirely possible for users in the same role, or having the same taxonomy term to edit each other's nodes. However when this happens, node_validate will transfer ownership of the node to the user who last edited it. I think this behavior should be changed so that original authorship is always maintained unless specifically transferred. The problem lies in node_validate, here: $node->uid = $user->uid ? $user->uid : 0; Since an alteration such as this could introduce an exploit, I'm wondering what other's feel would be the best solution? I was working on a role-based editing permissions module (based on JonBob's nodeperm_role.module) where the author of a node controls which groups can view/edit their post. ------------------------------------------------------------------------ Sat, 25 Sep 2004 21:34:00 +0000 : mathias Attachment: http://drupal.org/files/issues/node_perm.patch (655 bytes) Here is a proposed patch, which would then allow node authors to choose which users could view/edit their post. ------------------------------------------------------------------------ Tue, 21 Dec 2004 16:42:24 +0000 : moshe weitzman seems simple enough to me. we ought to protect against unintentially changing the author, right? +1 ------------------------------------------------------------------------ Mon, 27 Dec 2004 09:16:57 +0000 : Dries Hopefully, this will become easier/clear as soon the revisions patch hit CVS. Let's revisit this soon. ------------------------------------------------------------------------ Wed, 23 Feb 2005 05:52:26 +0000 : tangent As requested in this issue [1], it may be desirable for users with the permission to do so to change the owner of a node. [1] http://drupal.org/node/17267 ------------------------------------------------------------------------ Tue, 08 Mar 2005 20:57:14 +0000 : Dries Waiting for the node revision patch to land.

1 0

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

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

1 0

[drupal-devel] [bug] static export of theme style sheets impossible
by killes 13 Aug '05

13 Aug '05

Issue status update for http://drupal.org/node/21214 Post a follow up: http://drupal.org/project/comments/add/21214 Project: Drupal Version: 4.6.0 Component: theme system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: mjr Updated by: killes(a)www.drop.org -Status: active +Status: patch (code needs work) It's a patch. Please provide in diff -u format and look at the Drupal code format specs. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ Sun, 24 Apr 2005 13:21:14 +0000 : mjr Attachment: http://drupal.org/files/issues/theme.inc_0.diff (926 bytes) when i tried to mirror a drupal site using wget -m i only the default style sheet ./misc/drupal.css will be copied. Reason: the code to load that style sheet outputs an additional statement to that style sheet. The function theme_stylesheet_import does not. A fix to this is included. Best regards, Michael

1 0

[drupal-devel] spam module v2.0
by Jeremy Andrews 13 Aug '05

13 Aug '05

Hello, The rewrite of the spam module is out of beta testing, and ready for widespread use. An upgrade script is now available. Thank you to everyone that provided feedback during the development phase. The current release is version 2.0.6, which works with both Drupal 4.5 and Drupal 4.6. http://www.kerneltrap.org/jeremy/drupal/spam/ If you are using the older spam module hosted at drupal.org, you are advised to upgrade due to several important bug fixes. (The old spam module would frequently mismark nonspam content as spam due to a bug in the bayesian filter.) This new version of the module adds many often requested features, such as the ability to expire spam content, automatically deleting it after a configurable amount of time. It has also been optimized for improved performance. If you are using the trackback module, you will need to apply the included trackback.patch file (4.6 only). Features: * Written in PHP specifically for Drupal. * Highly configurable. * Automatically detects and unpublishes spam comments and other spam content. * Automatically learns to detect spam in any language using Bayesian logic. * Automatically learns and blocks spammer URLs. * Automatically blacklists IPs of learned spammers, preventing them from posting additional spam and wasting database resources. * Detects repeated postings of the same identical content. * Detects content containing too many links, or the same link over and over. * Supports the creation of custom filters using powerful regular expressions. * Can notify the user that his or her content was determined to be spam, preventing confusion over why their content doesn't show up. * Can notify the site administrator in an email when spam is detected. * Provides simple administrative interfaces for reviewing spam content. * Provides comprehensive logging to offer an understanding as to how and why content is determined to be or not to be spam. The new spam module can be found here: http://www.kerneltrap.org/jeremy/drupal/spam/ Regards, -Jeremy

1 0

[drupal-devel] [bug] Tablesort and $_REQUEST array handling
by rbrooks00 13 Aug '05

13 Aug '05

Issue status update for http://drupal.org/node/5371 Post a follow up: http://drupal.org/project/comments/add/5371 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: mathias Reported by: mathias Updated by: rbrooks00 Status: patch (code needs review) +1 for this patch Having the built in ability to sort tabular data is incredibly useful. rbrooks00 Previous comments: ------------------------------------------------------------------------ Fri, 23 Jan 2004 16:07:37 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays.patch (652 bytes) Tablesort tries its best to rebuild the URI from which it was called. The problem is when $_REQUEST holds arrays such as $edit. Tablesort will process this as: example.com?edit=Array This can be problematic since many module conditions rely on whether or not the $edit array is set. My patch disallows tablesort to append "array $_REQUEST variables" to it's new query string, since it serves no purpose. ------------------------------------------------------------------------ Sat, 31 Jan 2004 11:00:20 +0000 : Dries I'd like to hear Moshe's take on this as I'm not sure this is the proper fix. I'd think that the tablesort code knows exactly what fields it should add to the URL. ------------------------------------------------------------------------ Sat, 31 Jan 2004 13:24:36 +0000 : moshe weitzman I don't think this is the proper fix either. For example, - browse to the Drupal.org issue search page [1]. - press Search button - On the results page, look at the links in the table header and the pager. The query state gets passed along by tablesort. This patch would break this behavior. Changing Status to 'Active', since this patch shouldn't be applied without modification. [1] http://drupal.org/project/drupal/issues/search ------------------------------------------------------------------------ Sat, 31 Jan 2004 17:01:56 +0000 : mathias I'm pretty sure the project system is hacking the request object just like I am before sending it to tablesort: <?php if ($_SERVER['REQUEST_METHOD'] == 'POST' && is_array($_POST['edit'])) { foreach ($_POST['edit'] as $key => $value) { if (!empty($value) && in_array($key, $fields)) { $query->$key = $value; $_POST[$key] = is_array($value) ? implode(',', $value) : $value; } } unset($_POST['edit'], $_POST['op']); } ?> Basically, we're both moving the edit array into separate name / value pairs and then destroying it before handing it off to tablesort. I don't understand what the benefit is of passing an array into a GET query string since the key/value pairs aren't retained. We could set an option in tablesort to ask it to decompose the array into it's key/value pairs for us and pass them back in the query string. For example an edit array such as: <?php $edit['a'] => 1 $edit['b'] => 2 would have a query string of: &edit_a=1&edit_b=2 instead of the currently meaningless: &edit=Array ?> ------------------------------------------------------------------------ Sat, 31 Jan 2004 17:12:57 +0000 : Goba You mean: <?php $edit['a'] => 1 $edit['b'] => 2 would have a query string of: &edit[a]=1&edit[b]=2 instead of the currently meaningless: &edit=Array ?> This reuses the wonderful array handling provided by PHP (as long as nested arrays are handled properly). ------------------------------------------------------------------------ Sun, 01 Feb 2004 15:02:25 +0000 : Kjartan Indeed project module hacks the request data. I don't really like this as a general fix as it is a hack, but for now it is the best way. I dont think the original patch will break this as it just ignors arrays(). Project module loops through the array and converts them to normal array string elements; $_POST['edit']['status'] = value becomes $_POST['status'] = value, then I think I unset the edit array to clean up the urls. ------------------------------------------------------------------------ Thu, 05 Feb 2004 13:42:16 +0000 : moshe weitzman After feedback from Matias and Kjartan, I now consider this patch to be desireable ------------------------------------------------------------------------ Thu, 12 Aug 2004 15:10:30 +0000 : killes(a)www.drop.org Patch doesn't apply anymore. ------------------------------------------------------------------------ Fri, 08 Oct 2004 04:14:30 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_0.patch (2.55 KB) This patch resolves the hacks project module and the ecommerce package use to rebuild the $_REQUEST arrays when implementing paged resultsets and/or tablesorting. I know some other developers have ran into this bug as well usually after form submissions. Tablesort will now fully handle multi-dimensional $_REQUEST arrays and convert them to a valid querystring and preserve user-submitted data. ------------------------------------------------------------------------ Fri, 08 Oct 2004 04:34:12 +0000 : moshe weitzman very nicely done ... bonus points for converting pager to use the new array2uri() function ... tablesort's poor handling of arrays frustrated me in a recent client project. this will simplify almost all cases where a form post leads to a sortable "results" table, such as in project.module. ------------------------------------------------------------------------ Fri, 15 Oct 2004 14:01:42 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_1.patch (2.99 KB) Moved the new array2uri function to live with its array2* counterparts in common.inc and cleaned up the function documentation a wee bit. ------------------------------------------------------------------------ Tue, 21 Dec 2004 16:43:33 +0000 : moshe weitzman this is a nice code consolidation IMO ------------------------------------------------------------------------ Wed, 22 Dec 2004 10:50:33 +0000 : Dries This patch adds more code than it removes. That aside, it would be nice if the function's PHPdoc comments were extended. Looking at the patch, it is easy to see how/when this function is being used, but reading the PHPdoc comments it is not. I'd add a simple example and some context information. Also, it is unclear what $current_key is used for without inspecting the function's code. For readability, maybe rename $array to $attributes? If that makes sense, maybe rename 'array2uri' to 'attributes2uri'? Which makes me wonder: how does this stack with the existing drupal_attributes() function? Do we really need the recursive array handling? ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:25:06 +0000 : killes(a)www.drop.org Doesn't apply anymore. ------------------------------------------------------------------------ Tue, 12 Jul 2005 22:49:07 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_2.patch (3.19 KB) Updating this patch since I've been bitten by this issue again. The new array2uri() function really shines for tablesorting and/or paging search results after a $_POST request. The only coding change involved for developers is to use $_REQUEST['edit'] and $_REQUEST['op'] instead of $_POST['edit'] and $_POST['op'] within their function callback where this functionality is desired. As an added benefit these pages could also be bookmarked. Dries, I've updated the documentation to hopefully makes things a little easier to understand. I don't think renaming the $array variable to $attributes or calling the function attributes2uri instead of array2uri would make the function and its purpose more transparent. It's not used for HTML attributes like the drupal_attributes function, but rather for form submitted data (e.g., the $edit array) that needs to be tablesorted/paged. Project module and the ecommerce package still currently hack the $_POST vars to circumvent this problem. Not sure if other modules are also doing this. ------------------------------------------------------------------------ Fri, 15 Jul 2005 16:25:46 +0000 : Steven If we're going to have an array2uri() function, we might as well add urlencode() calls in there... this would centralize a lot of existing urlencode() calls, I think. ------------------------------------------------------------------------ Mon, 18 Jul 2005 13:51:56 +0000 : Dries Not going to commit this yet. 1. Let's wait for Mathias' feedback on Steven's comment. 2. I suggest using query or query_string, not querystring. 3. It is not clear why this can't be part of drupal_attributes($array)? If there is an important difference, it would be nice to make that clear in the PHPdoc comments. ------------------------------------------------------------------------ Fri, 22 Jul 2005 03:18:19 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_3.patch (3.23 KB) Agreed with Steven that this is a logical place to urlencode values. The patch has been updated accordingly. There was some IRC discussion as to whether this function should be called drupal_uri, drupal2uri or just plain ol' array2uri. I think since this code doesn't make any Drupal specific function calls, array2uri is the best name. Which brings up a whole new issue for a whole new thread: Perhaps drupal_attributes should be renamed array2attributes? ------------------------------------------------------------------------ Mon, 25 Jul 2005 08:50:00 +0000 : Steven Dries: drupal_attributes is used for outputting attributes of an HTML tag. This patch is about putting data into the query (GET) part of an URL. They are completely different. mathias: couldn't we use this function in drupal_get_destination() as well ? ------------------------------------------------------------------------ Mon, 25 Jul 2005 08:56:05 +0000 : Steven By the way, what exactly is the point of this check: + if (!strstr($query_string, $key_as_uri)) { + $query_string .= '&'. $key_as_uri. '='. urlencode($value); + } It seems to be there to avoid duplicates, but there is no similar check below. If it is required, it needs a comment to explain it. ------------------------------------------------------------------------ Tue, 26 Jul 2005 17:02:50 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_4.patch (3.39 KB) drupal_get_destination() now uses array2uri(). I discovered that the use of strstr(), was to compensate for the function calling itself at times when it didn't need to. This bug is fixed and strstr() is removed. Good catch! ------------------------------------------------------------------------ Tue, 26 Jul 2005 18:57:26 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_5.patch (3.58 KB) Allow a string, NULL or array to be passed into array2uri(), making life easier on developers per Steven's feedback on IRC. ------------------------------------------------------------------------ Thu, 28 Jul 2005 18:15:44 +0000 : moshe weitzman thanks for updating this patch, matt. looks good to me. i did not test it yet though. ------------------------------------------------------------------------ Fri, 29 Jul 2005 18:40:25 +0000 : Steven Err, passing strings to /array/2uri() is pretty non-sensical and not at all what I meant. I'll see if I can make a better patch later :P. ------------------------------------------------------------------------ Sat, 13 Aug 2005 18:37:23 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_6.patch (4 KB) * Further code optimizations. * Made drupal_get_destination() make better use of array2uri().

1 0

[drupal-devel] [bug] Tablesort and $_REQUEST array handling
by mathias 13 Aug '05

13 Aug '05

Issue status update for http://drupal.org/node/5371 Post a follow up: http://drupal.org/project/comments/add/5371 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: mathias Reported by: mathias Updated by: mathias -Status: patch (code needs work) +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_6.patch (4 KB) * Further code optimizations. * Made drupal_get_destination() make better use of array2uri(). mathias Previous comments: ------------------------------------------------------------------------ Fri, 23 Jan 2004 16:07:37 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays.patch (652 bytes) Tablesort tries its best to rebuild the URI from which it was called. The problem is when $_REQUEST holds arrays such as $edit. Tablesort will process this as: example.com?edit=Array This can be problematic since many module conditions rely on whether or not the $edit array is set. My patch disallows tablesort to append "array $_REQUEST variables" to it's new query string, since it serves no purpose. ------------------------------------------------------------------------ Sat, 31 Jan 2004 11:00:20 +0000 : Dries I'd like to hear Moshe's take on this as I'm not sure this is the proper fix. I'd think that the tablesort code knows exactly what fields it should add to the URL. ------------------------------------------------------------------------ Sat, 31 Jan 2004 13:24:36 +0000 : moshe weitzman I don't think this is the proper fix either. For example, - browse to the Drupal.org issue search page [1]. - press Search button - On the results page, look at the links in the table header and the pager. The query state gets passed along by tablesort. This patch would break this behavior. Changing Status to 'Active', since this patch shouldn't be applied without modification. [1] http://drupal.org/project/drupal/issues/search ------------------------------------------------------------------------ Sat, 31 Jan 2004 17:01:56 +0000 : mathias I'm pretty sure the project system is hacking the request object just like I am before sending it to tablesort: <?php if ($_SERVER['REQUEST_METHOD'] == 'POST' && is_array($_POST['edit'])) { foreach ($_POST['edit'] as $key => $value) { if (!empty($value) && in_array($key, $fields)) { $query->$key = $value; $_POST[$key] = is_array($value) ? implode(',', $value) : $value; } } unset($_POST['edit'], $_POST['op']); } ?> Basically, we're both moving the edit array into separate name / value pairs and then destroying it before handing it off to tablesort. I don't understand what the benefit is of passing an array into a GET query string since the key/value pairs aren't retained. We could set an option in tablesort to ask it to decompose the array into it's key/value pairs for us and pass them back in the query string. For example an edit array such as: <?php $edit['a'] => 1 $edit['b'] => 2 would have a query string of: &edit_a=1&edit_b=2 instead of the currently meaningless: &edit=Array ?> ------------------------------------------------------------------------ Sat, 31 Jan 2004 17:12:57 +0000 : Goba You mean: <?php $edit['a'] => 1 $edit['b'] => 2 would have a query string of: &edit[a]=1&edit[b]=2 instead of the currently meaningless: &edit=Array ?> This reuses the wonderful array handling provided by PHP (as long as nested arrays are handled properly). ------------------------------------------------------------------------ Sun, 01 Feb 2004 15:02:25 +0000 : Kjartan Indeed project module hacks the request data. I don't really like this as a general fix as it is a hack, but for now it is the best way. I dont think the original patch will break this as it just ignors arrays(). Project module loops through the array and converts them to normal array string elements; $_POST['edit']['status'] = value becomes $_POST['status'] = value, then I think I unset the edit array to clean up the urls. ------------------------------------------------------------------------ Thu, 05 Feb 2004 13:42:16 +0000 : moshe weitzman After feedback from Matias and Kjartan, I now consider this patch to be desireable ------------------------------------------------------------------------ Thu, 12 Aug 2004 15:10:30 +0000 : killes(a)www.drop.org Patch doesn't apply anymore. ------------------------------------------------------------------------ Fri, 08 Oct 2004 04:14:30 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_0.patch (2.55 KB) This patch resolves the hacks project module and the ecommerce package use to rebuild the $_REQUEST arrays when implementing paged resultsets and/or tablesorting. I know some other developers have ran into this bug as well usually after form submissions. Tablesort will now fully handle multi-dimensional $_REQUEST arrays and convert them to a valid querystring and preserve user-submitted data. ------------------------------------------------------------------------ Fri, 08 Oct 2004 04:34:12 +0000 : moshe weitzman very nicely done ... bonus points for converting pager to use the new array2uri() function ... tablesort's poor handling of arrays frustrated me in a recent client project. this will simplify almost all cases where a form post leads to a sortable "results" table, such as in project.module. ------------------------------------------------------------------------ Fri, 15 Oct 2004 14:01:42 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_1.patch (2.99 KB) Moved the new array2uri function to live with its array2* counterparts in common.inc and cleaned up the function documentation a wee bit. ------------------------------------------------------------------------ Tue, 21 Dec 2004 16:43:33 +0000 : moshe weitzman this is a nice code consolidation IMO ------------------------------------------------------------------------ Wed, 22 Dec 2004 10:50:33 +0000 : Dries This patch adds more code than it removes. That aside, it would be nice if the function's PHPdoc comments were extended. Looking at the patch, it is easy to see how/when this function is being used, but reading the PHPdoc comments it is not. I'd add a simple example and some context information. Also, it is unclear what $current_key is used for without inspecting the function's code. For readability, maybe rename $array to $attributes? If that makes sense, maybe rename 'array2uri' to 'attributes2uri'? Which makes me wonder: how does this stack with the existing drupal_attributes() function? Do we really need the recursive array handling? ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:25:06 +0000 : killes(a)www.drop.org Doesn't apply anymore. ------------------------------------------------------------------------ Tue, 12 Jul 2005 22:49:07 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_2.patch (3.19 KB) Updating this patch since I've been bitten by this issue again. The new array2uri() function really shines for tablesorting and/or paging search results after a $_POST request. The only coding change involved for developers is to use $_REQUEST['edit'] and $_REQUEST['op'] instead of $_POST['edit'] and $_POST['op'] within their function callback where this functionality is desired. As an added benefit these pages could also be bookmarked. Dries, I've updated the documentation to hopefully makes things a little easier to understand. I don't think renaming the $array variable to $attributes or calling the function attributes2uri instead of array2uri would make the function and its purpose more transparent. It's not used for HTML attributes like the drupal_attributes function, but rather for form submitted data (e.g., the $edit array) that needs to be tablesorted/paged. Project module and the ecommerce package still currently hack the $_POST vars to circumvent this problem. Not sure if other modules are also doing this. ------------------------------------------------------------------------ Fri, 15 Jul 2005 16:25:46 +0000 : Steven If we're going to have an array2uri() function, we might as well add urlencode() calls in there... this would centralize a lot of existing urlencode() calls, I think. ------------------------------------------------------------------------ Mon, 18 Jul 2005 13:51:56 +0000 : Dries Not going to commit this yet. 1. Let's wait for Mathias' feedback on Steven's comment. 2. I suggest using query or query_string, not querystring. 3. It is not clear why this can't be part of drupal_attributes($array)? If there is an important difference, it would be nice to make that clear in the PHPdoc comments. ------------------------------------------------------------------------ Fri, 22 Jul 2005 03:18:19 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_3.patch (3.23 KB) Agreed with Steven that this is a logical place to urlencode values. The patch has been updated accordingly. There was some IRC discussion as to whether this function should be called drupal_uri, drupal2uri or just plain ol' array2uri. I think since this code doesn't make any Drupal specific function calls, array2uri is the best name. Which brings up a whole new issue for a whole new thread: Perhaps drupal_attributes should be renamed array2attributes? ------------------------------------------------------------------------ Mon, 25 Jul 2005 08:50:00 +0000 : Steven Dries: drupal_attributes is used for outputting attributes of an HTML tag. This patch is about putting data into the query (GET) part of an URL. They are completely different. mathias: couldn't we use this function in drupal_get_destination() as well ? ------------------------------------------------------------------------ Mon, 25 Jul 2005 08:56:05 +0000 : Steven By the way, what exactly is the point of this check: + if (!strstr($query_string, $key_as_uri)) { + $query_string .= '&'. $key_as_uri. '='. urlencode($value); + } It seems to be there to avoid duplicates, but there is no similar check below. If it is required, it needs a comment to explain it. ------------------------------------------------------------------------ Tue, 26 Jul 2005 17:02:50 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_4.patch (3.39 KB) drupal_get_destination() now uses array2uri(). I discovered that the use of strstr(), was to compensate for the function calling itself at times when it didn't need to. This bug is fixed and strstr() is removed. Good catch! ------------------------------------------------------------------------ Tue, 26 Jul 2005 18:57:26 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_5.patch (3.58 KB) Allow a string, NULL or array to be passed into array2uri(), making life easier on developers per Steven's feedback on IRC. ------------------------------------------------------------------------ Thu, 28 Jul 2005 18:15:44 +0000 : moshe weitzman thanks for updating this patch, matt. looks good to me. i did not test it yet though. ------------------------------------------------------------------------ Fri, 29 Jul 2005 18:40:25 +0000 : Steven Err, passing strings to /array/2uri() is pretty non-sensical and not at all what I meant. I'll see if I can make a better patch later :P.

1 0

[drupal-devel] [bug] Prefix UPDATE statements in prefix.sh
by Axel 13 Aug '05

13 Aug '05

Issue status update for http://drupal.org/node/24749 Project: Drupal Version: cvs Component: other Category: bug reports Priority: minor Assigned to: Anonymous Reported by: Axel Updated by: Axel Status: patch Attachment: http://drupal.org/files/issues/prefix-update.patch (297 bytes) The patch adds prefixing for UPDATE SQL statements in prefix.sh. Axel

4 5