- Development - lists.drupal.org

[drupal-devel] [bug] Error with file api and open_basedir restriction
by Jose A Reyero 14 Mar '05

14 Mar '05

Issue status update for http://drupal.org/node/5961 Project: Drupal Version: cvs Component: node system Category: bug reports Priority: normal -Assigned to: Jose A Reyero +Assigned to: Anonymous Reported by: Jose A Reyero Updated by: Jose A Reyero -Status: active +Status: patch Attachment: http://drupal.org/files/issues/file-open-basedir_02.patch (810 bytes) Just resubmitted the patch. The problem is, from the time I sent the first patch , I've moved all my sites, no shared webhosting anymore, so I don't have that upload trouble anymore, and also cannot test it myself. That's also why I'm unassigning this issue to me. So please, somebody else test it. Jose A Reyero Previous comments: ------------------------------------------------------------------------ February 20, 2004 - 20:45 : Jose A Reyero Attachment: http://drupal.org/files/issues/file-open-basedir.patch (1018 bytes) I get this error when uploading a file through the file api, and the upload fails: warning: open_basedir restriction in effect. File is in wrong directory in... This happens when open_basedir restriction is activated and the temporary directory is not in the allowed directories. There's a related bug in the profile.module: http://drupal.org/node/view/2648 [1] FIX: The solution is to upload files using php's 'move_uploaded_file' -which works even when open_basedir restriction is in effect. This patch checks for the error conditions in 'file.inc::file_check_upload', and then uses move_uploaded_file to upload the file to the temporary folder with a random name. Otherwise -if the error conditions are not met-, does nothing. RESTRICTIONS: This patch introduces one additional restriction for the file api: Once you have called 'file_check_upload', you have to use the returned object when calling later 'file.api::file_save_upload'. This shouldn't be a problem, as the right way to upload files should be -I think, please correct me if I'm wrong-: - $file=file_check_upload(); - (Perform validations with $file) - file_save_upload($file,.....) I will post also a very simple patch for profile.module to work with this one. Anyway, as mentioned above, if the error conditions are not met, the patch does nothing, so I think it should be applied first and then check for other modules (profile.module) to use the file api the right way and then be able to upload files when open_basedir restriction in effect. [1] http://drupal.org/node/view/2648 ------------------------------------------------------------------------ November 28, 2004 - 18:38 : Goba This seems to be an interesting alternate fix to my dump patch for locale.module [2], which is the very same issue... Moving the file out of the upload folder right away to the Drupal temp folder seems to be a good idea to me. The only caveat is that files in the upload folder get removed automatically, while one needs to remove the file from the Drupal temp folder, once it is not needed anymore! [2] http://drupal.org/node/13285 ------------------------------------------------------------------------ December 8, 2004 - 20:26 : Goba Please look at this stuff. We need to solve open_basedir problems at least in core before the next point release. There are quite a few users trying to use Drupal on shared hosting here, where open_basedir is in effect. ------------------------------------------------------------------------ February 18, 2005 - 02:55 : clydefrog This functionality gets a +1 from me, the user. We solved this problem by changing upload_tmp_dir in the server config, but a fix in Drupal would be more elegant. ------------------------------------------------------------------------ March 13, 2005 - 20:41 : killes(a)www.drop.org Doesn't apply anymore.

1 0

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

14 Mar '05

Issue status update for http://drupal.org/node/5380 Project: Drupal Version: cvs Component: user system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: ax Updated by: stefan nagtegaal Status: patch 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? stefan nagtegaal Previous comments: ------------------------------------------------------------------------ January 24, 2004 - 00:33 : 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 ;) ------------------------------------------------------------------------ February 11, 2004 - 06:04 : 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. ------------------------------------------------------------------------ September 27, 2004 - 20:25 : 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. ------------------------------------------------------------------------ October 1, 2004 - 19:41 : 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? ------------------------------------------------------------------------ October 1, 2004 - 22:11 : 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. ------------------------------------------------------------------------ March 13, 2005 - 20:18 : 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. ------------------------------------------------------------------------ March 13, 2005 - 20:37 : 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' ------------------------------------------------------------------------ March 13, 2005 - 21:11 : killes(a)www.drop.org 'User ID '.$cron_uid.' does not exist' < == needs to be made translatable. Setting to active. ------------------------------------------------------------------------ March 14, 2005 - 06:56 : 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.

1 0

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

14 Mar '05

Issue status update for http://drupal.org/node/5380 Project: Drupal Version: cvs Component: user system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: ax Updated by: Chris Johnson -Status: active +Status: patch 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. Chris Johnson Previous comments: ------------------------------------------------------------------------ January 23, 2004 - 17:33 : 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 ;) ------------------------------------------------------------------------ February 10, 2004 - 23:04 : 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. ------------------------------------------------------------------------ September 27, 2004 - 13:25 : 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. ------------------------------------------------------------------------ October 1, 2004 - 12:41 : 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? ------------------------------------------------------------------------ October 1, 2004 - 15:11 : 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. ------------------------------------------------------------------------ March 13, 2005 - 13:18 : 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. ------------------------------------------------------------------------ March 13, 2005 - 13:37 : 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' ------------------------------------------------------------------------ March 13, 2005 - 14:11 : killes(a)www.drop.org 'User ID '.$cron_uid.' does not exist' < == needs to be made translatable. Setting to active.

1 0

[drupal-devel] taxonomy_dropmenu 1.0 (tested on IE6 sp1)
by Maykel Moya 14 Mar '05

14 Mar '05

Some time ago I announced the module taxonomy_dropmenu. It wasnt' working in IE. Now I'd reworked the module and it is now HTML/CSS for non IE users. In order to make it work under IE, a javascript trick is provided. Please, review if you have time. And if you have suggestions for any of the item in the TODO, please let me know. The most annoying to me now is the fact that one width in the styles rules the width of every menu. Regards maykel -- ,''`. Debian GNU/Linux : :' : The universal & free Operating System `. `' El Sistema Operativo universal & libre `- http://www.debian.org/

1 0

[drupal-devel] [bug] Preview themed as a node list (no way to detect preview)
by Geary 14 Mar '05

14 Mar '05

Issue status update for http://drupal.org/node/18792 Project: Drupal Version: cvs Component: node.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: elonen Updated by: Geary Status: patch Hi Moshe, Yes, I did both the node and comment. Just got a little mixed up about how to post the patches. They should both be in this thread, or the 4.5.2 versions are on the page I linked to above. Geary Previous comments: ------------------------------------------------------------------------ March 12, 2005 - 11:21 : elonen Function theme('node', $node, $teaser, $page) can't detect a preview and therefore often renders it incorrectly. The problem is that !$page is usually interpreted as "the node is part of a listing" in the themes and therefore the previewed page may look, depending on the theme, very different on the actual page. Perhaps theme() should get somewhat more detailed context info than just $page? ------------------------------------------------------------------------ March 13, 2005 - 09:02 : Geary Attachment: http://drupal.org/files/issues/node.module_5.patch (384 bytes) Here is a patch to node.module that adds a $node->preview flag which themes can use to display previews appropriately. This seems like the simplest possible core change to support styled previews. For more information, screen shots, and a working demo using an updated FriendsLight theme, see: http://mg.to/2005/03/13/drupal-preview-confusion and http://drupal.mg.to/2005/03/13/friendslight-theme-with-styled-preview I am submitting a similar patch to comment.module. ------------------------------------------------------------------------ March 13, 2005 - 09:03 : Geary Attachment: http://drupal.org/files/issues/comment.module_5.patch (290 bytes) Here is a patch to comment.module that adds a $comment->preview flag which themes can use to display previews appropriately. This seems like the simplest possible core change to support styled previews. For more information, screen shots, and a working demo using an updated FriendsLight theme, see: http://mg.to/2005/03/13/drupal-preview-confusion and http://drupal.mg.to/2005/03/13/friendslight-theme-with-styled-preview I am submitting a similar patch to node.module. ------------------------------------------------------------------------ March 13, 2005 - 09:07 : Geary Oops, one of my patches above says "4.5.2". Both of these patches are actually against the CVS code. Sorry about the mixup. (Other than line numbers, the same patch works for 4.5.2 or CVS.) ------------------------------------------------------------------------ March 13, 2005 - 09:16 : Geary It seems that when I enter a title in one of these comments, it changes the title on the main entry I'm replying to. Since my previous comment left a title that made no sense out of context, I'm adding this comment in the hope of changing the main title so it makes sense again. Sorry about that. ------------------------------------------------------------------------ March 13, 2005 - 15:31 : moshe weitzman seems like we'd want a similar flag for a node preview. yours is a comment preview. would be a nice enhancement to this patch.

1 0

[drupal-devel] [feature] Handle full URLs in url() and l()
by gordon 14 Mar '05

14 Mar '05

Issue status update for http://drupal.org/node/10888 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: jhriggs Updated by: gordon Status: patch +1 This is a good thing for people who can't using clean urls. gordon Previous comments: ------------------------------------------------------------------------ September 17, 2004 - 01:01 : jhriggs Attachment: http://drupal.org/files/issues/full_url.patch (1.97 KB) This little patch allows full URLs to be used for the url() and l() functions, giving modules a single interface for creating links. Currently, modules must use l() for drupal links and raw HTML for external links. This patch changes url() to handle -- basically pass through -- full URLs while still doing the proper handling of internal paths and works with clean URLs enabled or disabled. It also shortens (and hopefully simplifies) the url() code a bit. Some examples of calls that now work: <?php l('a link', 'some/path'); // same as before l('an external link', 'http://drupal.org/'); l('full with query', 'http://google.com/search?q=define:foo'); l('full with separate query', 'http://google.com/search', array(), 'q=define:foo'); // same link as above l('full with fragment', 'http://drupal.org/node/5942#comment-12374'); l('full with separate fragment', 'http://drupal.org/node/5942', array(), NULL, 'comment-12374'); // same link as above ?> ------------------------------------------------------------------------ September 30, 2004 - 21:09 : moshe weitzman +1. less HTML in code is a good thing for readability, if nothing else. ------------------------------------------------------------------------ January 25, 2005 - 06:06 : mathias Attachment: http://drupal.org/files/issues/external_urls.patch (838 bytes) Revisiting this issue one year later shows we're almost there. The only thing that's left is the ability to handle external urls when clean_urls is disabled. Once implemented, menu.module can happily accept external urls and not just Drupal paths, making the menu system much more attractive to end users. I'll update the menu.module docs if this patch (or an alternative solution) is accepted. ------------------------------------------------------------------------ March 14, 2005 - 08:36 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/external_urls_0.patch (909 bytes) Updated from Drupal root directory.

1 0

[drupal-devel] [bug] Preview themed as a node list (no way to detect preview)
by moshe weitzman 14 Mar '05

14 Mar '05

Issue status update for http://drupal.org/node/18792 Project: Drupal Version: cvs Component: node.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: elonen Updated by: moshe weitzman Status: patch seems like we'd want a similar flag for a node preview. yours is a comment preview. would be a nice enhancement to this patch. moshe weitzman Previous comments: ------------------------------------------------------------------------ March 12, 2005 - 14:21 : elonen Function theme('node', $node, $teaser, $page) can't detect a preview and therefore often renders it incorrectly. The problem is that !$page is usually interpreted as "the node is part of a listing" in the themes and therefore the previewed page may look, depending on the theme, very different on the actual page. Perhaps theme() should get somewhat more detailed context info than just $page? ------------------------------------------------------------------------ March 13, 2005 - 12:02 : Geary Attachment: http://drupal.org/files/issues/node.module_5.patch (384 bytes) Here is a patch to node.module that adds a $node->preview flag which themes can use to display previews appropriately. This seems like the simplest possible core change to support styled previews. For more information, screen shots, and a working demo using an updated FriendsLight theme, see: http://mg.to/2005/03/13/drupal-preview-confusion and http://drupal.mg.to/2005/03/13/friendslight-theme-with-styled-preview I am submitting a similar patch to comment.module. ------------------------------------------------------------------------ March 13, 2005 - 12:03 : Geary Attachment: http://drupal.org/files/issues/comment.module_5.patch (290 bytes) Here is a patch to comment.module that adds a $comment->preview flag which themes can use to display previews appropriately. This seems like the simplest possible core change to support styled previews. For more information, screen shots, and a working demo using an updated FriendsLight theme, see: http://mg.to/2005/03/13/drupal-preview-confusion and http://drupal.mg.to/2005/03/13/friendslight-theme-with-styled-preview I am submitting a similar patch to node.module. ------------------------------------------------------------------------ March 13, 2005 - 12:07 : Geary Oops, one of my patches above says "4.5.2". Both of these patches are actually against the CVS code. Sorry about the mixup. (Other than line numbers, the same patch works for 4.5.2 or CVS.) ------------------------------------------------------------------------ March 13, 2005 - 12:16 : Geary It seems that when I enter a title in one of these comments, it changes the title on the main entry I'm replying to. Since my previous comment left a title that made no sense out of context, I'm adding this comment in the hope of changing the main title so it makes sense again. Sorry about that.

1 0

[drupal-devel] [bug] Menu cache is not locale aware
by stefan nagtegaal 14 Mar '05

14 Mar '05

Issue status update for http://drupal.org/node/13503 Project: Drupal Version: cvs Component: menu system Category: bug reports Priority: normal Assigned to: chx Reported by: chx Updated by: stefan nagtegaal Status: patch Tested against 4.6 RC I and works perfectly! +1 for applying this bug-fix.. stefan nagtegaal Previous comments: ------------------------------------------------------------------------ November 26, 2004 - 08:13 : chx Attachment: http://drupal.org/files/issues/menu_locale.patch (522 bytes) If you have a multi language site, and change languages, the menu is already cached, thus it can not display the translated version. This simple patch makes the menu cache locale aware. Do I need to file the same patch against HEAD? 'Cos it applies, too. ------------------------------------------------------------------------ November 26, 2004 - 10:13 : Dries Isn't it better to flush the menu cache when someone (or the admin) switches languages? Keeps the cache size down. ------------------------------------------------------------------------ November 26, 2004 - 12:20 : chx There could be several points where the language is changed. i18n for example displays a language selection block which lets you switch anytime. Thus I think this solution is the best. ------------------------------------------------------------------------ November 26, 2004 - 13:26 : Goba See also this issue [1], which is still (again) open. The locale is also changed, when the admin changes it as the default on the admin interface, and the menu cache needs to be flushed then, and the admin needs to be redirected. I don't know how these problems are solved currently, so I would only be able to submit a placebo patch to make that issue pop up again... [1] http://drupal.org/node/11312 ------------------------------------------------------------------------ November 26, 2004 - 14:28 : Bèr Kessels I solved it by adding some logic to the swithcer function in i18n: when a language is switched the cache is flushed. The negative side of this, is that if you have hundred various roles of users reading English and one switching to German, the complete cahce is flushed. But then again, who has so many roles, that this might become an issue? ------------------------------------------------------------------------ November 27, 2004 - 10:59 : Dries I just fixed bug 11312 [2]. I don't see what's wrong with invalidating the user's menu cache when he or she switches language. Just do: <?php cache_clear_all("menu:$user->uid"); ?> [2] http://drupal.org/node/11312 ------------------------------------------------------------------------ January 30, 2005 - 20:26 : Jose A Reyero This is needed also for i18n. Invalidating cache is not a solution here, as we would need to do it also for anonymous users. So I think this is a simple solution and will work fine for both cases, user/locale and i18n. ------------------------------------------------------------------------ January 31, 2005 - 16:15 : Bèr Kessels I found out what is wrong with this: Swithcing and cache are two completely differetn things: The language is a per-session setting. The cache is a per-role setting. So any user can switch langauge on any moment, resulting in a cahce that will get flushed on nearly every page-load. And since roles and sessions are tow different thingsm we should not flush a role based cache because a variable in the session changes. The only correct option would be to have per-locale cache. (as per Jose's patch) ------------------------------------------------------------------------ January 31, 2005 - 16:27 : Bèr Kessels Correction, It was not José that submitteed the patch for locale aware menu's but Károly Négyesi, I got confused because José was the last to comment on that patch. ------------------------------------------------------------------------ February 2, 2005 - 21:44 : moshe weitzman i can't make heads or tails of this patch request. it seems that some folks are confusing menu cache and page cache. and we don't even have a role cache. dries' question remains unanaswered. why can we not just rebuild a user's menu cache when needed? ------------------------------------------------------------------------ February 2, 2005 - 22:17 : Jose A Reyero /why can we not just rebuild a user's menu cache when needed?/ This way you cannot have menus in multiples languages for anonymous users, unles they're not cached at all. ------------------------------------------------------------------------ February 2, 2005 - 22:27 : moshe weitzman Aha! Makes sense now. +1 for this simple patch. ------------------------------------------------------------------------ March 13, 2005 - 23:10 : killes(a)www.drop.org I agree, this should get patched. Still applies.

1 0

[drupal-devel] [feature] Handle full URLs in url() and l()
by killes 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/10888 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: jhriggs Updated by: killes(a)www.drop.org Status: patch Attachment: http://drupal.org/files/issues/external_urls_0.patch (909 bytes) Updated from Drupal root directory. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ September 16, 2004 - 16:01 : jhriggs Attachment: http://drupal.org/files/issues/full_url.patch (1.97 KB) This little patch allows full URLs to be used for the url() and l() functions, giving modules a single interface for creating links. Currently, modules must use l() for drupal links and raw HTML for external links. This patch changes url() to handle -- basically pass through -- full URLs while still doing the proper handling of internal paths and works with clean URLs enabled or disabled. It also shortens (and hopefully simplifies) the url() code a bit. Some examples of calls that now work: <?php l('a link', 'some/path'); // same as before l('an external link', 'http://drupal.org/'); l('full with query', 'http://google.com/search?q=define:foo'); l('full with separate query', 'http://google.com/search', array(), 'q=define:foo'); // same link as above l('full with fragment', 'http://drupal.org/node/5942#comment-12374'); l('full with separate fragment', 'http://drupal.org/node/5942', array(), NULL, 'comment-12374'); // same link as above ?> ------------------------------------------------------------------------ September 30, 2004 - 12:09 : moshe weitzman +1. less HTML in code is a good thing for readability, if nothing else. ------------------------------------------------------------------------ January 24, 2005 - 21:06 : mathias Attachment: http://drupal.org/files/issues/external_urls.patch (838 bytes) Revisiting this issue one year later shows we're almost there. The only thing that's left is the ability to handle external urls when clean_urls is disabled. Once implemented, menu.module can happily accept external urls and not just Drupal paths, making the menu system much more attractive to end users. I'll update the menu.module docs if this patch (or an alternative solution) is accepted.

1 0

[drupal-devel] [bug] new revisions not created if user doesn't have administer nodes permission
by killes 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/12401 Project: Drupal -Version: 4.5.0 +Version: cvs Component: node system Category: bug reports -Priority: critical +Priority: normal Assigned to: wazdog Reported by: wazdog Updated by: killes(a)www.drop.org Status: patch Attachment: http://drupal.org/files/issues/node-rev_0.patch (981 bytes) Still applies, but I updated it anyway. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ November 1, 2004 - 09:33 : wazdog If you enable revisions for a node type via content > configure > default workflow, and the user who is editing a node of that type doesn't have the administer nodes permission, then a new revision is not created. I see this a huge bug. I am trying to have all edits to a node type saved as revisions (in this case a flexinode, but I also tested with blog types). I want to recreate the book pages, but with more fields (hence, flexinode). But if one of my authenticated users creates a node, and then later edits it, a revision is not made. However, if an admin or moderator (who both have the administer nodes permission) edits the node, a new revision is created. I don't want my regular users to have administer nodes permission, that gives them too much power, but their revisions need to be kept (just like I give them all publish permission via default workflow). If this is supposed to be how it works, what is the point of the default workflow setting? (and why do the other default workflow settings behave differently? i.e. they all apply no matter the users permissions) ------------------------------------------------------------------------ November 3, 2004 - 01:48 : wazdog Attachment: http://drupal.org/files/issues/node-rev.patch (721 bytes) Okay, here's a short little patch. It seems like node_create_revision() was being called before $node->revision was set. Since $node->revision is what tells Drupal whether or not to make a new revision, this was causing nodes to not keep revisions. I moved node_create_revision() to after this check. I'm not sure exactly why only regular users without 'administer nodes' permission were affected, but that seems the case. Now, if a particular node type has 'revision' checked in content > default workflow, a new revision is always created, no matter who is doing the editing... I'm setting this to critical because I think the current behavior goes against logic, and what admins may expect. And if they don't notice it's not working like they expect (i.e. revisions aren't being kept always), then content is lost... The patch is against 4.5.0, but always applies to current cvs (with a slight offset error). ------------------------------------------------------------------------ January 14, 2005 - 21:54 : mcd I tried this patch against 4.5.1 and it worked, but it took me some time to figure out that it was working because it's still not doing the intuitive thing. The user still can't see the revisions tab unless they have node administration privileges (which makes it unlike the outline tab). They can't see the checkbox to decide whether their edit is a new revision, so every edit they do becomes a revision from which they derive no benefit. I would let them see the revision checkbox and the revisions tab, and also roll back (non-destructively as suggested in http://drupal.org/node/12479) I don't think there is an intuitive way to handle the five node administration Options checkboxes as a group while appearing to enable them separately in the default workflow. I've had similar issues with the defaults, like a page I made sticky coming unstuck every time the owner edited it (with the default non-sticky in the workflow). Now I see why that was happening, but it's still highly counterintuitive. ------------------------------------------------------------------------ January 14, 2005 - 21:56 : mcd Sorry, I didn't mean to change the main title. ------------------------------------------------------------------------ January 14, 2005 - 22:11 : chx IMHO in some situations it is a good thing if a new revision is created every time a user edits the node. It's not her privilege to roll back, it's something a moderator should do. It'd be even better if the new revision would not be automatically published only after approval by the moderator. ------------------------------------------------------------------------ January 14, 2005 - 22:18 : mcd Rolling back nondestructively is just a means of editing a page. Why should a user with permission to edit be prevented from that edit? They can certainly do more destructive things. As for creating new revisions every time, it depends on usage patterns. You may not want too many almost indistinguishable revisions collecting in the revisions tab. ------------------------------------------------------------------------ January 14, 2005 - 23:32 : robertDouglass My opinions: 1. administer nodes is the wrong permission to determine revisions behavior. At least one revision-specific permission is necessary, perhaps two or more (view, create, delete, rollback revisions). 2. If the default workflow says revisions are made, this should definitely make it so that normal users create revisions every time. 3. Users with admin rights (either a revisions specific permission or administer nodes) can override the default. All others should not see the checkbox. 4. Whether revisions are rolled back destructively or non-destructively *could* be a setting, but if not, should be non-destructive by nature as we already have a mechanism for deleting revisions. ----- That much is the minimum I consider necessary to be a useable system. I would additionally wish for the following: 5. Since I would like to have a site where normal users can view any of the revisions, but not delete and not roll back, I would really like to see the following permissions: read revision, rollback revision, delete revision 6. The ability to compare two or more revisions [1] 7. The possibility for moderation of revisions ie. people with administer nodes (or moderate revisions) look at a queue of revisions and rollbacks and approve or disapprove them. 8. The possibility to vote on revisions. 7 could be an extension of 8. If only administrators can vote on revisions, it would be the same as moderating them. I know the last bits sounds like a bunch of pipe dreams, and may be off topic for this thread, but the system I imagine would be for a site where the wording of entries must be considered very carefully by many people and a concensus about the best version be found. The above features would achieve this. [1] http://drupal.org/node/15596 ------------------------------------------------------------------------ January 18, 2005 - 08:10 : wazdog Ultimately, an "edit" permission would encompass the permission needed in a "rollback revision" permission, as the latter is a subset of the former. Therefore, if a user can edit a node, they should be able to rollback a revision (though deleting a revision should still be a different permission, because it is destructive).

1 0