- Development - lists.drupal.org

[drupal-devel] [feature] common.inc Move textarea pre hook between title and textarea
by Dries 03 Apr '05

03 Apr '05

Issue status update for http://drupal.org/node/18828 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: jbond Updated by: Dries Status: patch Anyone to test this? Dries Previous comments: ------------------------------------------------------------------------ March 13, 2005 - 11:52 : jbond Attachment: http://drupal.org/files/issues/common.inc_10.patch (1.8 KB) This patch moves the textarea hook for 'pre' between the title and the textarea. This makes it easier for simply textarea enhancements like quicktags to add a toolbar just above the textarea. It doesn't appear to have any effect on the fckeditor or htmlarea modules. It doesn't introduce any new hooks. ------------------------------------------------------------------------ March 13, 2005 - 19:56 : killes(a)www.drop.org Guess that is a patch. ------------------------------------------------------------------------ March 14, 2005 - 15:38 : Bèr Kessels +1 From me. Any module that adds something, will want to add it after the title. I really cannot think of any case where aditional content should sit above the title, but I can think of loads of cases where content needs to sit between the title and textarea. ------------------------------------------------------------------------ March 15, 2005 - 01:20 : stefan nagtegaal Indeed, nice and simple patch! +1 from me either.. ------------------------------------------------------------------------ March 15, 2005 - 16:13 : jbond http://www.voidstar.com/node.php?id=2263 [1] If this patch gets accepted I'll drop this into contrib. It's similar to quicktags but *I* like it better. ;-) [1] http://www.voidstar.com/node.php?id=2263 ------------------------------------------------------------------------ March 29, 2005 - 12:09 : TDobes +1... I'm pretty sure the current positioning was accidental anyway... it was always meant to go directly above the textarea. ------------------------------------------------------------------------ March 29, 2005 - 16:33 : Bèr Kessels Dries, All people who use this, seem to agree. Any chance this gets in soon? Bèr ------------------------------------------------------------------------ March 29, 2005 - 18:23 : Dries Attachment: http://drupal.org/files/issues/textarea.patch (1.88 KB) Ber: while all people seem to agree, it is unclear how many people tested/reviewed this patch. One ought to be careful. For example, the patch introduces a new variable, $form_element, without properly initializing it ... How about the attached patch? It is slightly cleaner, slightly faster and properly initializes all variables. I haven't tested it with the WYSIWYG editors but maybe someone else (Julian?) can? Will commit after it has been tested.

1 0

[drupal-devel] [feature] Request New Password Security
by Dries 03 Apr '05

03 Apr '05

Issue status update for http://drupal.org/node/18719 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: neofactor Updated by: Dries Status: patch factoryjoe: you can't reset your password when you are logged in. Dries Previous comments: ------------------------------------------------------------------------ March 11, 2005 - 04:52 : neofactor Problem: Any user can force another user's password to change... simply by selecting "request new password" and putting in their username. The user gets an email with the new.. but this feels like a violation to the user... and a pain. Solution? If someone requests a new password... Don't blindly change it... send an email that says...."Is this a real request authorized by you? Click here to confirm otherwise disregard this message" Please consider this critical for user by-in to Drupal as a secure system. I appreciate your consideration. http://drupal.org/node/18689 ------------------------------------------------------------------------ March 11, 2005 - 06:05 : neofactor I added some code to prevent the admin account from being reset... Please add as a patch. if ($account->uid == 1 { unset($account); form_set_error('name', t('Sorry. The username %name is not allowed to be changed.', array('%name' => ''. $edit['name'] .''))); } // Just above this code on line 911: if ($account) { $from = variable_get('site_mail', ini_get('sendmail_from')); $pass = user_password(); ------------------------------------------------------------------------ March 11, 2005 - 06:08 : killes(a)www.drop.org Please only set to "patch" if you are actually submitting one. The solution you proposed is rather a workaround and will not be acceptable for core. ------------------------------------------------------------------------ March 11, 2005 - 10:21 : Deno I was in charge of a web site with >10000 paying customers for some time, and the "customers care" was constantly troubled with pasword change requests, until we switched to a system that worked in the following way: 1) "I forgot the password" function accepts either users email, or users login name, generates a random phrase, and adds an entry to "pasword_change_request" table. This entry consists of: - User ID (corresponding to email or login) - random phrase - datestamp User ID is unique key, and the entries in this table older than MAX_TIME are regularly purged by a cron job. 2) System sends an email with a link to "change forgotten password" page with this random phrase as argument to the user. That would be something like: https://drupal.org/user/cfp/d438rwiuodw894732ehdw 3) When user follows the link, "cfp" page checks if the random phrase exists in the table, allows the user to immediately change the password, and sends another email with "your password was changed on RIGHT_NOW ... " note to the user in question. - Password change automatically triggers deleting of the corresponding entry in pasword_change_request table. - Password change automatically logs-in the user as well. This way, users weren't bothered by accidental password changes, and they also understood the system better than the one with automatically generated passwords. For additional security, we also built some brakes that assured that the number of attempts to change the password, such as: - "An email with instructions was sent to you X minutes ago. In case you don't receive it within N hours, please contact the site administrator" - "According to our logs, you have visited the CFP page more than NMAX times within last 24 hours. For security reasons, you will be denied access to this page for the next 24 hours. Please contact the site administrator." hope this helps... ------------------------------------------------------------------------ March 30, 2005 - 22:01 : Jose A Reyero Attachment: http://drupal.org/files/issues/user_one_time_login.patch (6.52 KB) This patch for -cvs- addresses this issue. It generates an *only one use* URL to login into user account, instead of a new password. Then the user clicks on the URL, logs in, and may change his old password if desired. It is a only one use URL, and has some additional security measures, like a configurable time-out -can be confgured in admin/user/settings- and also, if somebody uses it or password is changed in the meanwhile, previously generated login URLs of this type won't work. About how it works, it creates a new url with: - user id, - timestamp - a hash of current timestamp, changed date for account, and the old password hashed With this patch, this wont work for admin accounts, which I think is better, you never know... It has a number of features like: - User can be ignoring these emails if somebody else is requesting access, a new password request wont change current password. However, the user will be warned when seeing the emails - It reveals no sensible information about the user account in the URL itself - the generated hash is not predictable at all if you dont know the old password, as it depends on user's old password - there's a configurable timeout - The URL's are only one use, as they depend on two timestamps, one of which will be changing in case this login is used -or in case user logs in for other means - Any 'new password' request is independent of others, so no one can be interfering with one user asking for his password back. Also doesn't require any new db field nor keeping track of requested passwords. ------------------------------------------------------------------------ March 30, 2005 - 22:41 : chx Very nice patch. Please consider for 4.6. ------------------------------------------------------------------------ March 31, 2005 - 00:16 : drumm Couldn't those nested if statements be made into one with a bit cleaner indentation? It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? The url is quite long, what could be done to make it shorter? ------------------------------------------------------------------------ March 31, 2005 - 01:50 : Jose A Reyero /Couldn't those nested if statements be made into one with a bit cleaner indentation?/ Maybe, but I had to split that if conditions in two parts because the $account var didn't get the value before the rest of the conditions -latest two- where evaluated. Someone can help with PHP here? /It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting?/ Agreed, will be polishing this a little bit, just wating for some more suggestions... For the duration, I'll change to 'hours', ok? Just don't like to restrict options using unneccessary dropdowns, it's only intended for administrators anyway. Maybe I add also a '0' for unlimited. Hardcoding it, would need anyway to be mentioned in the module help, so it's about the same overhead, both for coding and for the admin interface, and harcoding in two places in code is not good anyway. And I like having options, am I the only one? / The url is quite long, what could be done to make it shorter?/ Well, the url has to be long, it has neccessary params and an MD5 hash, which yes, could be cut someway, but that adding inneccessary code and also reducing security -though half an MD5 hash may be secure enough- but I see no point in that. ------------------------------------------------------------------------ March 31, 2005 - 03:18 : FactoryJoe(a)civicspacelabs.org "Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? " +1 for making this *not* configurable! C'mon, Drupal should have standards about security -- and if best practices say that, say, 8 hours, is a good amount of time for that URL to be active, then do that. Cut out the confusion and extra brain juice it takes to decide... "Gee, is 4 hours better than 5?" Additionally, there's a simple way to make this workflow better for the user. If you visit an outdated change password URL, you should be told, "Sorry, but that link you followed to change your password has expired. If you would like to reset your password, please fill out this form: [form] If you did not request to change your password, please notify [administrator's name w/ PM link]". This way, if you missed your change password deadline, you can request your password again right from the timeout page. If you didn't request the change, it will be obvious that someone else was trying to change it, and then you can contact the administrator and find out who it was that was messin' with your account. ------------------------------------------------------------------------ March 31, 2005 - 07:55 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_0.patch (4.72 KB) I cleaned up the patch a bit. I hope code style is OK. So may -- there are no checks against the arguments? There is no need. user_load uses %d for 'uid', so that's dealt with. Comparison by < and > means an implicit cast to integer, so $timestamp is also dealt with automatically. I do not think timeout needs to be configured. Please comment on not letting uid 1 using this one. I am not sure whether this is necessary. ------------------------------------------------------------------------ March 31, 2005 - 08:12 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_1.patch (6.21 KB) Implemented Chris' idea, so if you use a one time URl which is expired, you are sent to user/password with a message to ask for another. I changed a few messages and deleted the "not for admin" feature. Do we really want one gazillion support requests "I locked myself out of my site, user/password tells me not for admin, what now"? ------------------------------------------------------------------------ March 31, 2005 - 09:24 : chx One more point on uid 1. Yes, uid 1 can reset his pwd over the database. But then we would need to lock out administer users privileged users. Then administer nodes, 'cos of XSS. Then administer comments... then administer filters... OK, this is pointless. If someone can eavesdrop on your emails, everything is lost, I think. This is not something Drupal shall address. Maybe we could employ a security question -- secret answer pair. ------------------------------------------------------------------------ March 31, 2005 - 12:52 : Bèr Kessels Eventhough I am a less-settings advocate, I think this must remain a setting :). Why? Sites on Drupal differ a lot, and have a very differnet userbase. I run very low level sites, where it sometimes takes days before someone will read his/her mail. But I also know sites (drupal.org) where days would certainly be too long. Sorry Chris et al, I +1 on configurable duration. (hell, we really need some about:config alike screen in drupal) ------------------------------------------------------------------------ March 31, 2005 - 13:46 : Dries IMO, that setting has nothing to do with a site being "low level". If you request a new password, you probably want to login ASAP. I vote against a configuration option. Either way, this patch needs work: 1. We don't glue words together (eg. $hashedpass, $currenttime, $loginurl). Similarly, I suggest to change 'user/newpassword' to 'user/reset' or 'user/request-password'. 2. The patch exposes technical details to the user; terminology like "one time login URL" is likely to confuse many users. 3. The login URL is not clean URL safe, I think. 4. Please rename the variable $pass1 in user_pass_rehash(). 5. The code comments are often inconsistent. Some end with a punctuation, some don't. Some span two lines, others don't. On a related note, can't we reuse this system for the initial password? That is, instead of mailing a password, we mail an URL and force the user to enter a password. ------------------------------------------------------------------------ March 31, 2005 - 19:20 : Jose A Reyero Attachment: http://drupal.org/files/issues/user_one_time_login_2.patch (12.77 KB) Ok, I've started with chx's reviewed patch, and done most of Dries suggestions Main changes are: ----------------------- - Rewording, variable names, path, etc... as pointed out by Dries. Now it's user/reset. And yes, it was not clean-URL safe; it is now. - Allowed also for admin account, I dont like it though, but seem to be the only one :( - Used also for user registration, but I also kept old style login/password, think it can be some use and anyway, it's up to the site admin to remove it from the welcome e-mail. However it is only for 'no admin approval', see below about this.... - Added back in the config option for time-out, though reworked completely for usability. Default is '1 day' which I think should be reasonable. However, there's no time out for firts time users. About this, read below, please... ----------------------- Using this for first time login has some problems when admin aproval is required, as the hashed pass depends on 'changed' timestamp, and it changes when the admin approves an account. Actually, there's no way to know -or I didn't find it- when it is the first login for an approved account. For no admin approval, I just used 'changed' == 'created' as a criterium. I think we should be separating this changed date and last login date creating a new field in the user table, but that maybe for a future patch. That would also increase security, as the Administrator's info would be more accurate, and 'last login date' could be shown when user logs in, which is also security+ Related with this I've experienced problems, as administrator, when editing user accounts, then last login info for the users gets lost -is reset-... About the config option for time out, though I agree that enforcing security can be good in general, this is one of this cases where more security means less usability. And that decission should be left to Site administrators -each site has its own security requirements-. So I think the target here should be a 'reasonable' default, and then some flexibility for site admins. IMHO this latest patch is quite well balanced. So... everybody happy? ------------------------------------------------------------------------ March 31, 2005 - 21:43 : Dries "So ... everyone happy?" Not quite. The patch is an improvement but it uses tabs, incorrect spacing, incorrect placement of brackets, and does not use theme('placeholder'). If a new database column is required to implement this properly, please add one. This patch won't get committed until it is implemented properly. I still vote to remove the configuration option. We're getting there ... ------------------------------------------------------------------------ April 2, 2005 - 20:45 : factoryjoe I’m going to have to stick by my earlier assertion on this one, and, and, as with most usability issues, none of us are “correct” until we test it 50 times and see whether anyone actually uses the option. Just like with my folksonomy review [1], more options is *not always better* nor does it always help people *get things done*! To further expand this point, Jose says [2]: About the config option for time out, though I agree that enforcing security can be good in general, this is one of this cases where more security means less usability. And that decission should be left to Site administrators -each site has its own security requirements. " But this is a misuse of the term “usability”. What you’re talking about is *functionality*—since usability has more to do with focusing single-mindedly on the task at hand and reducing distraction than it does with providing extraneous options that might be useful for a fraction of all Drupal admins. To speak to Ber’s point [3], I cannot imagine a situation where, if I want to change my password because I’ve forgotten it, that it will take more than a few hours to get the email and fulfill the process. And even still, if it does — for whatever reason — take me a week to get around to resetting my password, then starting the process over again isn’t all that time consuming. Plus, I have the added sense of security that Drupal doesn’t let things like password resetting links stay active forever! So again, I agree that we need a reasonable default—24 hours is very generous for the singular task that we’re trying to serve here, which is, to my knowledge, to make the process for resetting your password easier and more secure. Lastly, about the patch itself: I noticed a couple tags strewn around in lines 38, 43, 54 and 72 (as well as a on 54). Until we have a style guide for marking up usernames, should we really be using HTML like this? It seems to be that it would be better to use a span with a class of… say… “username” and let the themer determine whether the username should be bold, italicized, red, all caps and so on.*/ [1] http://drupal.org/node/19697#comment-25599 [2] http://drupal.org/node/18719#comment-25480 [3] http://drupal.org/node/18719#comment-25463 ------------------------------------------------------------------------ April 2, 2005 - 22:29 : factoryjoe One other quick question. How does this patch interact with distributed authentication? If I'm logged in with a remote account and ask to reset my password, what happens?

1 0

[drupal-devel] Is anybody doing any major work on subscriptions module?
by Robert Douglass 03 Apr '05

03 Apr '05

Hi all, I began rewriting the subscriptions module a bit today and wanted to check here if anyone else was also working on it? It would be a shame to double our efforts. Here is what I've done so far: You can now subscribe to nodes, users and categories (was nodes, blogs(?) and categories). The /subscriptions screen where all of the nodes and all of the terms for the entire site were listed in one gigantic table is gone. In its place is a screen with three tabs; categories, users, content. The categories tab lets you subscribe using normal select boxes. Content and users tabs only show you your subscriptions and offer you the chance to unsubscribe. I don't think it is a good idea to use these screens as a means for subscribing to nodes and users. Each node has the links (un)subscribe content, (un)subscribe user. Users can decide whether they want immediate notification or digest notification at an admin specified interval. What I hope to finish tomorrow is the digest mailing, the admin screen, and placing a lind (un)subscribe user on each user's page. I will introduce hook_cron to handle digest mailing and probably a second table to store the digest mails between crons. The original subscriptions table is unaltered and a smooth upgrade will be possible. cheers, Robert

10 15

[drupal-devel] [feature] Enable customizing of "Title" field on node forms
by chx 03 Apr '05

03 Apr '05

Issue status update for http://drupal.org/node/10056 Project: Drupal Version: cvs Component: node system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Anonymous Updated by: chx -Status: active +Status: patch Attachment: http://drupal.org/files/issues/node-move-form-title-field_0.patch (4.59 KB) The patch applies to ucrrent HEAD with some offset. I have rerolled it so it applies cleanly. chx Previous comments: ------------------------------------------------------------------------ August 14, 2004 - 19:17 : Anonymous Attachment: http://drupal.org/files/issues/node.set-title-field.patch (2.02 KB) The default "title" field in a node object is useful but not always appropriate. For some node types it would be preferable to rename the field for display to users (e.g., from "title" to "name" for a record of an organization or business), while in others the title might be best drawn from a combination of other fields (e.g., first_name and last_name fields, for a record of an individual). This patch enables two new alternatives for node titles: renamingThe title field can be renamed, e.g., from "title" to "name". substutingOther fields can be substituted for the title field. The functionality is implemented through a new proposed node hook, _title_field($op). There are two possible values for $op: "action" (either "rename" or "substitute") and "names" (a list of field names to be used in the title field). Sample uses: <?php PHP function example_title_field($node, $op) { switch ($op) { case 'action': $return = 'rename'; break; case 'names': $return = array('Name'); } return $return; } ?> In this case, since the requested action is "rename", the title field would be labelled "Name" (instead of "Title") in node add and edit forms for nodes of type "example". <?php PHP function example_title_field($node, $op) { switch ($op) { case 'action': $return = 'substitute'; break; case 'names': $return = array('first_name', 'last_name'); } return $return; } ?> In this case, no "Title" field would be displayed in node add or node edit forms of node type "example". Instead, the fields "first_name" and "last_name" would be concatenated to form a title. Implementing this patch would significantly increase the flexibility of the node system, making it feasible to store information of many types not currently supported because they don't have a "title" attribute. ------------------------------------------------------------------------ August 15, 2004 - 17:39 : Dries This is best accomplished through the existing nodeapi hook, using a separate module. ------------------------------------------------------------------------ August 16, 2004 - 00:02 : nedjo Thanks for the suggestion. I did look for a way to accomplish this through _nodeapi (and other existing hooks), but couldn't see how. The "title" field is hard-coded into node.module. The _nodeapi hook allows various types of manipulations, but not, so far as I could see, changes to elements that have been generated by, e.g., form_textfield() calls. Are there possibilities I'm missing? Other approaches? Any specific suggestions as to means we could use to change the hard-coded "title" field? ------------------------------------------------------------------------ August 19, 2004 - 19:27 : nedjo Attachment: http://drupal.org/files/issues/node.set-title-field_0.patch (741 bytes) Drawing on Dries's suggestion, I've substantially revised this patch to enable node title field customization with only three additional lines of code in node.module. In place of the existing line in function node_form() <?php $output .= form_textfield(t('Title'), 'title', $edit->title, 60, 128, NULL, NULL, TRUE); ?> the patch substitutes a the following: <?php $title = variable_get('node_titlefield_'. $edit->type, 'Title'); if(!($title == 'none')) { $output .= form_textfield(t($title), 'title', $edit->title, 60, 128, NULL, NULL, TRUE); } ?> With this change in place, node modules will be able to override the standard node title label by setting a variable, node_titlefield_nodetype, where nodetype is the type of node. This could be done, e.g., in a _settings hook, so that the first time settings were accessed the module would set the variable: <?php if (variable_get('node_titlefield_example', '') == '') { // since none is already set, set this variable variable_set('node_titlefield_example', 'Name'); drupal_set_message('Example module initialization completed.'); } ?> The special value 'none' would suppress the title field altogether in node forms, used as follows: <?php if (variable_get('node_titlefield_example', '') == '') { // since none is already set, set this variable variable_set('node_titlefield_example', 'none'); drupal_set_message('Example module initialization completed.'); } ?> In this case, a _validate hook would be used to set the title field value. Assuming a module where a first name-last name combination was to be used for a title: <?php function example_validate(&$node) { if(!(isset($node->title))) { $node->title = $node->first_name . ' ' . node->last_name; } } ?> In short, this three-line addition to the node.module would significantly increase the flexibility of the node system by removing the hard-coded 'Title' field in node add and edit forms and instead enabling custom labelling or concatenation of the field from other sources. ------------------------------------------------------------------------ August 19, 2004 - 19:50 : moshe weitzman I like this functionality. But the implementation still seems unclean. I would prefer to simply remove the 'title' form field from node_form() and require that modules present this field in whatever way they please. in some cases, they will use a hidden form field because no customer interaction is required. This causes a break with backward compatibility, but cleanliness is facored over compatibilityness (when you must choose). I'm moving this back to 'Active', since I don't think this patch is committable as is. ------------------------------------------------------------------------ August 28, 2004 - 00:46 : nedjo Attachment: http://drupal.org/files/issues/node.set-title-field_1.patch (830 bytes) Thanks for looking at this. The suggestion - dropping the title field altogether and leaving this for module authors - has the advantage of simplicity. However, this change would imply significant new work as it would would require all or nearly all existing modules - core and contributed - be modified. While I think enabling title field customization is important for Drupal as a whole, and while this is a change I need for two modules I'm working on, I'm not immediately convinced that the benefits of this change justify this cost. Also, in most cases, the present title field works fine, so keeping it as a default option would I feel be desirable. Hence the following patch. In place of my (poorly conceived) initial hook proposals, above, this patch introduces a single very straightforward new node hook, _title_field. By setting this hook to return FALSE, node modules authors will suppress the default title field, and be free therefore to substitute what they wish or need. Returning TRUE, or not using the hook, will result in the default title field being used. Sample usage: <?php function example_title_field() { return FALSE; } ?> This is, I feel, a much better solution than the previous two I suggested--proof, if it's true, that the Drupal issue system works! ------------------------------------------------------------------------ September 6, 2004 - 16:50 : nedjo Are there any objections to or issues raised by this small patch (introducing a new hook to make the title field optional in node forms)? If not, can it be applied? ------------------------------------------------------------------------ September 7, 2004 - 14:46 : Anonymous I think I agree with Moshe's opinion that this should be accomplished by having node modules display the title field. In any event, this is a new feature and shouldn't go in during the feature freeze for 4.5. ------------------------------------------------------------------------ September 7, 2004 - 14:58 : ccourtne +1 for just moving the title field to be a responsibility of the implementing module instead of node_form. There is already to some extent "hook overload", in addition there may be several modules which don't want to even display a title module. Why add that only fixes some of the problems just to have to remove it later when you make a fix that address the whole problem. ------------------------------------------------------------------------ September 7, 2004 - 22:09 : nedjo Attachment: http://drupal.org/files/issues/node.remove-title-field.patch (636 bytes) Thanks for comments. As per feedback, the attached patch removes the title field from node forms, leaving this instead to node modules. Understood that possible approval of this patch this will need to wait until code unfrozen. ------------------------------------------------------------------------ October 24, 2004 - 13:06 : moshe weitzman please patch all core node modules as well. ------------------------------------------------------------------------ November 28, 2004 - 02:00 : nedjo Attachment: http://drupal.org/files/issues/node-move-form-title-field.patch (4.29 KB) Good point Moshe. This patch of node.module plus all core node modules simply moves the title field out of node.module and into the node modules, enabling (if desired) customization of the title field by a module. Patches to contributed modules will in most cases be as simple as adding the line <?php $output = form_textfield(t('Subject'), 'title', $node->title, 60, 128, NULL, NULL, TRUE); ?> at the beginning of a typename_form() function (and, if necessary changing an existing $output = to $output .=). In the attached patch, I've customized the title field for only one of the node types, calling the title of a forum topic the "Subject". We could consider calling the title of a blog a "Subject" or "Topic" as well. I feel this is a good solution (thanks for the feedback and discussion) and, for minimal work (minor updates to contributed modules), will have the significant benefit of enabling flexibility in content types beyond those that have "title" attributes. ------------------------------------------------------------------------ November 28, 2004 - 18:45 : nedjo Ability to customize title as provided in this patch was requested here [1]: "When adding/editing, the 'title' field is manditory, which is fine. Except that its label is hardcoded to 'title'. A custom node modelling, say, a 'Company' would much rather this field be presented to the user as 'Company Name'. " [1] http://drupal.org/node/13596 ------------------------------------------------------------------------ March 4, 2005 - 04:44 : nedjo Here's another patch that's sat for months without being either applied or turned down, although there was expressed support and, after several iterations, it addressed all issues that had been raised. The point is to remove the hard-coded "Title" field - inapprioriate for many node types, e.g., companies - from node forms, instead allowing node modules to treat this as appropriate (e.g., a "Subject", "Name" or "Topic", or nothing at all). Are there problems with this approach that were not raised? Is there a reason this wasn't accepted? ------------------------------------------------------------------------ March 4, 2005 - 04:54 : Steven nedjo: if you read the backlog you can see that this feature was delayed for after the 4.6 release. "I've customized the title field for only one of the node types, calling the title of a forum topic the "Subject". " This is really inconsistent, as what you call the title of a blog, story or forum node is purely subjective. "Subject" "Topic" and "Title" are pretty much interchageable and any subtle differences will surely be eroded by translation. The only thing I can agree on so far is naming a polls' title "Question". ------------------------------------------------------------------------ March 4, 2005 - 06:15 : nedjo Thanks for the note. I'm not sure what you mean by the "backlog". Where would I find this? I agree that a "Question" field would make sense for polls. ------------------------------------------------------------------------ March 4, 2005 - 15:12 : JonBob Actually UnConeD, it was postponed until after the 4.5 release, not 4.6, and looks like everyone forgot about it when the release happened. And no, Nedjo, I don't take this as proof that Drupal needs more bureaucracy. :-) Marking this active again; I'm sure the patch will have to be updated once the code branches. We had rolled this function into the needs for CCK, but in our discussions split it off again.

1 0

[drupal-devel] [feature] protect against breaking the bootstrap
by moshe weitzman 03 Apr '05

03 Apr '05

Issue status update for http://drupal.org/node/19895 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: moshe weitzman Reported by: moshe weitzman Updated by: moshe weitzman Status: patch Attachment: http://drupal.org/files/issues/module_list.patch (1.02 KB) currently modules can inadvertently load all other modules during the bootstrap. this is easy to do by calling a node_load() or module_invoke_all(). this degrades performance for anonymous users when caching is enabled. with this patch, the module writer will see an php error like "Call to undefined function: drupal_set_html_head()" if this is attempted. basically, unavailable functions are actually reported as unavailable. this patch is only 1 line long, and probably worthy of 4.6 consideration. If it goes in for 4.7, that would be OK with me too. in order to test this patch, you could view a cached page as anonymous when an offending modeule is enabled (e.g. sections.module, taxonomy_context.module, etc.). I will help fix all these modules after this patch is accepted. moshe weitzman

2 2

[drupal-devel] [bug] Multiple-Select Vocabulary Forms Broken
by jhriggs 02 Apr '05

02 Apr '05

Issue status update for http://drupal.org/node/19888 Project: Drupal Version: cvs Component: taxonomy.module Category: bug reports Priority: critical Assigned to: Anonymous Reported by: jhriggs Updated by: jhriggs Status: patch Attachment: http://drupal.org/files/issues/taxonomy_multiple.patch (740 bytes) Selecting taxonomy terms is broken for multiple-select vocabularies. The form is using the name 'edit[taxonomy][][]'. It should not have the second set of brackets. The attached patch fixes this. jhriggs

2 4

[drupal-devel] Take the FLOSSPOLS survey
by K B 02 Apr '05

02 Apr '05

Please take a few minutes to participate in the this FLOSS survey. http://drupal.org/node/19898 As developers in an open source software project, let your views/experiences be known. Regards

1 0

[drupal-devel] [feature] Request New Password Security
by factoryjoe 02 Apr '05

02 Apr '05

Issue status update for http://drupal.org/node/18719 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: neofactor Updated by: factoryjoe Status: patch One other quick question. How does this patch interact with distributed authentication? If I'm logged in with a remote account and ask to reset my password, what happens? factoryjoe Previous comments: ------------------------------------------------------------------------ March 11, 2005 - 02:52 : neofactor Problem: Any user can force another user's password to change... simply by selecting "request new password" and putting in their username. The user gets an email with the new.. but this feels like a violation to the user... and a pain. Solution? If someone requests a new password... Don't blindly change it... send an email that says...."Is this a real request authorized by you? Click here to confirm otherwise disregard this message" Please consider this critical for user by-in to Drupal as a secure system. I appreciate your consideration. http://drupal.org/node/18689 ------------------------------------------------------------------------ March 11, 2005 - 04:05 : neofactor I added some code to prevent the admin account from being reset... Please add as a patch. if ($account->uid == 1 { unset($account); form_set_error('name', t('Sorry. The username %name is not allowed to be changed.', array('%name' => ''. $edit['name'] .''))); } // Just above this code on line 911: if ($account) { $from = variable_get('site_mail', ini_get('sendmail_from')); $pass = user_password(); ------------------------------------------------------------------------ March 11, 2005 - 04:08 : killes(a)www.drop.org Please only set to "patch" if you are actually submitting one. The solution you proposed is rather a workaround and will not be acceptable for core. ------------------------------------------------------------------------ March 11, 2005 - 08:21 : Deno I was in charge of a web site with >10000 paying customers for some time, and the "customers care" was constantly troubled with pasword change requests, until we switched to a system that worked in the following way: 1) "I forgot the password" function accepts either users email, or users login name, generates a random phrase, and adds an entry to "pasword_change_request" table. This entry consists of: - User ID (corresponding to email or login) - random phrase - datestamp User ID is unique key, and the entries in this table older than MAX_TIME are regularly purged by a cron job. 2) System sends an email with a link to "change forgotten password" page with this random phrase as argument to the user. That would be something like: https://drupal.org/user/cfp/d438rwiuodw894732ehdw 3) When user follows the link, "cfp" page checks if the random phrase exists in the table, allows the user to immediately change the password, and sends another email with "your password was changed on RIGHT_NOW ... " note to the user in question. - Password change automatically triggers deleting of the corresponding entry in pasword_change_request table. - Password change automatically logs-in the user as well. This way, users weren't bothered by accidental password changes, and they also understood the system better than the one with automatically generated passwords. For additional security, we also built some brakes that assured that the number of attempts to change the password, such as: - "An email with instructions was sent to you X minutes ago. In case you don't receive it within N hours, please contact the site administrator" - "According to our logs, you have visited the CFP page more than NMAX times within last 24 hours. For security reasons, you will be denied access to this page for the next 24 hours. Please contact the site administrator." hope this helps... ------------------------------------------------------------------------ March 30, 2005 - 20:01 : Jose A Reyero Attachment: http://drupal.org/files/issues/user_one_time_login.patch (6.52 KB) This patch for -cvs- addresses this issue. It generates an *only one use* URL to login into user account, instead of a new password. Then the user clicks on the URL, logs in, and may change his old password if desired. It is a only one use URL, and has some additional security measures, like a configurable time-out -can be confgured in admin/user/settings- and also, if somebody uses it or password is changed in the meanwhile, previously generated login URLs of this type won't work. About how it works, it creates a new url with: - user id, - timestamp - a hash of current timestamp, changed date for account, and the old password hashed With this patch, this wont work for admin accounts, which I think is better, you never know... It has a number of features like: - User can be ignoring these emails if somebody else is requesting access, a new password request wont change current password. However, the user will be warned when seeing the emails - It reveals no sensible information about the user account in the URL itself - the generated hash is not predictable at all if you dont know the old password, as it depends on user's old password - there's a configurable timeout - The URL's are only one use, as they depend on two timestamps, one of which will be changing in case this login is used -or in case user logs in for other means - Any 'new password' request is independent of others, so no one can be interfering with one user asking for his password back. Also doesn't require any new db field nor keeping track of requested passwords. ------------------------------------------------------------------------ March 30, 2005 - 20:41 : chx Very nice patch. Please consider for 4.6. ------------------------------------------------------------------------ March 30, 2005 - 22:16 : drumm Couldn't those nested if statements be made into one with a bit cleaner indentation? It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? The url is quite long, what could be done to make it shorter? ------------------------------------------------------------------------ March 30, 2005 - 23:50 : Jose A Reyero /Couldn't those nested if statements be made into one with a bit cleaner indentation?/ Maybe, but I had to split that if conditions in two parts because the $account var didn't get the value before the rest of the conditions -latest two- where evaluated. Someone can help with PHP here? /It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting?/ Agreed, will be polishing this a little bit, just wating for some more suggestions... For the duration, I'll change to 'hours', ok? Just don't like to restrict options using unneccessary dropdowns, it's only intended for administrators anyway. Maybe I add also a '0' for unlimited. Hardcoding it, would need anyway to be mentioned in the module help, so it's about the same overhead, both for coding and for the admin interface, and harcoding in two places in code is not good anyway. And I like having options, am I the only one? / The url is quite long, what could be done to make it shorter?/ Well, the url has to be long, it has neccessary params and an MD5 hash, which yes, could be cut someway, but that adding inneccessary code and also reducing security -though half an MD5 hash may be secure enough- but I see no point in that. ------------------------------------------------------------------------ March 31, 2005 - 01:18 : FactoryJoe(a)civicspacelabs.org "Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? " +1 for making this *not* configurable! C'mon, Drupal should have standards about security -- and if best practices say that, say, 8 hours, is a good amount of time for that URL to be active, then do that. Cut out the confusion and extra brain juice it takes to decide... "Gee, is 4 hours better than 5?" Additionally, there's a simple way to make this workflow better for the user. If you visit an outdated change password URL, you should be told, "Sorry, but that link you followed to change your password has expired. If you would like to reset your password, please fill out this form: [form] If you did not request to change your password, please notify [administrator's name w/ PM link]". This way, if you missed your change password deadline, you can request your password again right from the timeout page. If you didn't request the change, it will be obvious that someone else was trying to change it, and then you can contact the administrator and find out who it was that was messin' with your account. ------------------------------------------------------------------------ March 31, 2005 - 05:55 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_0.patch (4.72 KB) I cleaned up the patch a bit. I hope code style is OK. So may -- there are no checks against the arguments? There is no need. user_load uses %d for 'uid', so that's dealt with. Comparison by < and > means an implicit cast to integer, so $timestamp is also dealt with automatically. I do not think timeout needs to be configured. Please comment on not letting uid 1 using this one. I am not sure whether this is necessary. ------------------------------------------------------------------------ March 31, 2005 - 06:12 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_1.patch (6.21 KB) Implemented Chris' idea, so if you use a one time URl which is expired, you are sent to user/password with a message to ask for another. I changed a few messages and deleted the "not for admin" feature. Do we really want one gazillion support requests "I locked myself out of my site, user/password tells me not for admin, what now"? ------------------------------------------------------------------------ March 31, 2005 - 07:24 : chx One more point on uid 1. Yes, uid 1 can reset his pwd over the database. But then we would need to lock out administer users privileged users. Then administer nodes, 'cos of XSS. Then administer comments... then administer filters... OK, this is pointless. If someone can eavesdrop on your emails, everything is lost, I think. This is not something Drupal shall address. Maybe we could employ a security question -- secret answer pair. ------------------------------------------------------------------------ March 31, 2005 - 10:52 : Bèr Kessels Eventhough I am a less-settings advocate, I think this must remain a setting :). Why? Sites on Drupal differ a lot, and have a very differnet userbase. I run very low level sites, where it sometimes takes days before someone will read his/her mail. But I also know sites (drupal.org) where days would certainly be too long. Sorry Chris et al, I +1 on configurable duration. (hell, we really need some about:config alike screen in drupal) ------------------------------------------------------------------------ March 31, 2005 - 11:46 : Dries IMO, that setting has nothing to do with a site being "low level". If you request a new password, you probably want to login ASAP. I vote against a configuration option. Either way, this patch needs work: 1. We don't glue words together (eg. $hashedpass, $currenttime, $loginurl). Similarly, I suggest to change 'user/newpassword' to 'user/reset' or 'user/request-password'. 2. The patch exposes technical details to the user; terminology like "one time login URL" is likely to confuse many users. 3. The login URL is not clean URL safe, I think. 4. Please rename the variable $pass1 in user_pass_rehash(). 5. The code comments are often inconsistent. Some end with a punctuation, some don't. Some span two lines, others don't. On a related note, can't we reuse this system for the initial password? That is, instead of mailing a password, we mail an URL and force the user to enter a password. ------------------------------------------------------------------------ March 31, 2005 - 17:20 : Jose A Reyero Attachment: http://drupal.org/files/issues/user_one_time_login_2.patch (12.77 KB) Ok, I've started with chx's reviewed patch, and done most of Dries suggestions Main changes are: ----------------------- - Rewording, variable names, path, etc... as pointed out by Dries. Now it's user/reset. And yes, it was not clean-URL safe; it is now. - Allowed also for admin account, I dont like it though, but seem to be the only one :( - Used also for user registration, but I also kept old style login/password, think it can be some use and anyway, it's up to the site admin to remove it from the welcome e-mail. However it is only for 'no admin approval', see below about this.... - Added back in the config option for time-out, though reworked completely for usability. Default is '1 day' which I think should be reasonable. However, there's no time out for firts time users. About this, read below, please... ----------------------- Using this for first time login has some problems when admin aproval is required, as the hashed pass depends on 'changed' timestamp, and it changes when the admin approves an account. Actually, there's no way to know -or I didn't find it- when it is the first login for an approved account. For no admin approval, I just used 'changed' == 'created' as a criterium. I think we should be separating this changed date and last login date creating a new field in the user table, but that maybe for a future patch. That would also increase security, as the Administrator's info would be more accurate, and 'last login date' could be shown when user logs in, which is also security+ Related with this I've experienced problems, as administrator, when editing user accounts, then last login info for the users gets lost -is reset-... About the config option for time out, though I agree that enforcing security can be good in general, this is one of this cases where more security means less usability. And that decission should be left to Site administrators -each site has its own security requirements-. So I think the target here should be a 'reasonable' default, and then some flexibility for site admins. IMHO this latest patch is quite well balanced. So... everybody happy? ------------------------------------------------------------------------ March 31, 2005 - 19:43 : Dries "So ... everyone happy?" Not quite. The patch is an improvement but it uses tabs, incorrect spacing, incorrect placement of brackets, and does not use theme('placeholder'). If a new database column is required to implement this properly, please add one. This patch won't get committed until it is implemented properly. I still vote to remove the configuration option. We're getting there ... ------------------------------------------------------------------------ April 2, 2005 - 18:45 : factoryjoe I’m going to have to stick by my earlier assertion on this one, and, and, as with most usability issues, none of us are “correct” until we test it 50 times and see whether anyone actually uses the option. Just like with my folksonomy review [1], more options is *not always better* nor does it always help people *get things done*! To further expand this point, Jose says [2]: About the config option for time out, though I agree that enforcing security can be good in general, this is one of this cases where more security means less usability. And that decission should be left to Site administrators -each site has its own security requirements. " But this is a misuse of the term “usability”. What you’re talking about is *functionality*—since usability has more to do with focusing single-mindedly on the task at hand and reducing distraction than it does with providing extraneous options that might be useful for a fraction of all Drupal admins. To speak to Ber’s point [3], I cannot imagine a situation where, if I want to change my password because I’ve forgotten it, that it will take more than a few hours to get the email and fulfill the process. And even still, if it does — for whatever reason — take me a week to get around to resetting my password, then starting the process over again isn’t all that time consuming. Plus, I have the added sense of security that Drupal doesn’t let things like password resetting links stay active forever! So again, I agree that we need a reasonable default—24 hours is very generous for the singular task that we’re trying to serve here, which is, to my knowledge, to make the process for resetting your password easier and more secure. Lastly, about the patch itself: I noticed a couple tags strewn around in lines 38, 43, 54 and 72 (as well as a on 54). Until we have a style guide for marking up usernames, should we really be using HTML like this? It seems to be that it would be better to use a span with a class of… say… “username” and let the themer determine whether the username should be bold, italicized, red, all caps and so on.*/ [1] http://drupal.org/node/19697#comment-25599 [2] http://drupal.org/node/18719#comment-25480 [3] http://drupal.org/node/18719#comment-25463

1 0

[drupal-devel] [bug] throttle settings a bit confusing
by Gerhard Killesreiter 02 Apr '05

02 Apr '05

Issue status update for http://drupal.org/node/19894 Project: Drupal Version: cvs Component: throttle.module Category: bug reports Priority: normal Assigned to: Gerhard Killesreiter Reported by: Gerhard Killesreiter Updated by: Gerhard Killesreiter Status: patch The texts on the settings page are a bit confusing, added an example. Also, having '' as default isn't helpfull, changed to '0'. Gerhard Killesreiter

2 1

[drupal-devel] [bug] Make block configuration understandable
by killes 02 Apr '05

02 Apr '05

Issue status update for http://drupal.org/node/11875 Project: Drupal -Version: <none> +Version: cvs Component: block.module -Category: tasks +Category: bug reports Priority: normal -Assigned to: drumm +Assigned to: killes(a)www.drop.org Reported by: drumm Updated by: killes(a)www.drop.org -Status: closed +Status: patch Attachment: http://drupal.org/files/issues/update_2.patch (823 bytes) The upgrade simply dropped all the stored regexes. I understand that an upgrade patch was difficult to provide, but we should at least document this. Attached patch does this. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ October 21, 2004 - 20:42 : drumm Attachment: http://drupal.org/files/issues/tmp (42.68 KB) The primary goal of this patch is to take the 'custom' and 'path' columns of the block overview page and make them into something understandable. As of Drupal 4.5 'custom' lacked an explanation which wasn't buried in help text and path required dealing with regular expressions. Every block now has a configuration page to control these options. This gives more space to make form controls which do not require a lengthy explanation. This page also gives modules a chance to put their block configuration options in a place that makes sense using new operations in the block hook. The only required changes to modules implementing hook_block() is to be careful about what is returned. Do not return anything if $op is not 'list' or 'view'. Once this change is made, modules will still be compatible with Drupal 4.5. Required changes to core modules are included in this path. An additional optional change to modules is to implement the additional $op options added. 'configure' should return a string containing the configuration form for the block with the appropriate $delta. 'configure save' will come with an additional $edit argument, which will contain the submitted form data for saving. These changes to core modules are also included in this patch. I have posted screenshots at http://foo.delocalizedham.com/tmp/blocks/. Additional work, such as further rearrangement of the block overview page, is as always a task for another patch. ------------------------------------------------------------------------ October 21, 2004 - 22:56 : drumm Attachment: http://drupal.org/files/issues/block_0.diff (42.79 KB) Now with a few more comments. ------------------------------------------------------------------------ October 21, 2004 - 23:14 : drumm Attachment: http://drupal.org/files/issues/blocks.sql (211 bytes) And the DB changes. ------------------------------------------------------------------------ October 23, 2004 - 18:06 : Dries This patch marks a significant usability improvement. Excellent job. I had a quick look at the patch: 1. Taken from the patch: <?php + else if ($op == 'configure' && $delta = 3) { ?> There are a number of '$delta = 3's that probably should be '$delta == 3's. 2. I would use 'save' rather than 'configure save'. 3. block_box_add() is a little weird: 3.a. Is the $bid parameter being used? 3.b. <?php + drupal_set_message(block_box_save($edit)); + drupal_goto('admin/block'); ?> I would move the drupal_set_message() and the drupal_goto() inside block_box_save(), if possible without having to add extra code. 3.c. <?php + $form = block_box_form(); + $form .= form_submit(t('Save block')); + $output .= form($form); ?> Any reason we can't move all form stuff into block_box_form()? 3.d. It would be good if we could get rid of the block vs box thing. 4. We need proper database updates. 5. <?php + php block['pages_show'] = $old_blocks[$module][$delta]->pages_show; + $block['pages'] = $old_blocks[$module][$delta]->pages; ?> What is the difference between 'pages' and 'pages_show'? It is not clear from the names so maybe we should try to come up with more explanatory names. 6. The changes will need to be documented in a new 'Converting modules from 4.5.x to CVS' page. 7. Has the block API Doxygen comments? ------------------------------------------------------------------------ October 23, 2004 - 21:40 : moshe weitzman this is an improvement. one nit: don't use a form_group() around all the form fields on a page. There is no need to group if everything is of the same type. If you need to name that single group of forms, use the page title ------------------------------------------------------------------------ October 23, 2004 - 22:13 : Steven I agree, this is a very nice patch. 1) One thing I do miss is that there is no (longer?) a clear overview of blocks (which blocks are always enabled, which are contextual, ...). However, this is really the fault of the giant configuration table. Reworking that is a different patch. 2) I have a usability issue with block.module which could go in this patch: the description field is really only useful for module-defined blocks where the title is not descriptive. Custom blocks have static titles, so the description is really redundant. I know I never fill it in on my own site, only to curse myself when I see the administration screen and have a bunch of unknown blocks sitting there. So either: - Get rid of the description field for custom blocks and use the title instead. - Keep the description field, but use the block title if the description field was left empty. I prefer the first. 3) Here's a reworked contextual help text for the block admin. The giant paragraph that we have now is not going to be read by anyone: "Blocks are the boxes in the left and right side bars of the web site. They are made available by modules or created manually. Only enabled blocks are shown. You can position the blocks by deciding which side of the page they will show up on (region) and in which order they appear (weight). If you want certain blocks to disable themselves temporarily during high server loads, check the 'Throttle' box. You will need to enable the auto-throttle on the [[throttle configuration page]] after having enabled the throttle module. " 4) The general module help needs work: "If the block\'s throttle checkbox is checked, the throttle level must me low." (me -> be) The paragraph about "A block is visible when:" is also structured oddly. The part about the throttle is confusing and the last option does not match the preceding fragment at all (it starts a new sentence). Maybe it's better to say: " A block's visibility depends on: - Its enabled checkbox. Disabled blocks are never shown. - Its throttle checkbox. Throttled blocks are hidden during high server loads. - Its path options. Blocks can be configured to only show/hide on certain pageS. - User settings. You can choose to let your users decide whether to show/hide certain blocks. - Its function. Dynamic blocks (such as those defined by modules) may be empty on certain pages and will not be shown. " This is a bit vaguer, but shorter and easier to understand. Where the exact options are located is not so important, because they are in a very logical place, and people are bound to run into them when making their own blocks. ------------------------------------------------------------------------ October 24, 2004 - 01:05 : Dries I'm OK with removing the description. ------------------------------------------------------------------------ October 29, 2004 - 01:42 : drumm Attachment: http://drupal.org/files/issues/block_1.diff (44.79 KB) #3: 1. Changed '=' to '=='. 2. Changed 'configure save' to 'save'. 3. Cleaned up block_box_add(), specific code is there because block_box_form() and block_box_save() are reused for editing. The distinction between block and box is useful, boxes are user created blocks, however box might not be the best name. 4. Added database updates for mysql. 5. Pages stores the list of pages for the textarea, pages_show stores whether those are pages to show, or not show, on for the radio buttons. 6. Yes it will. Text can probably be copied from #1. 7. Finishing those up now. I can commit them to contrib cvs myself. #4: It works well on blocks with specific configurations. I expect a limited number of additional options such as block name will be added before 4.6. That will create more groupings. #5: 1. Yes, lets save that for another patch. 2. I have a good idea for the description's needed removal which I plan to work into a patch which allows every block to be renamed. 3. Replaced with the suggested help text. 4. Changed 'me' to 'be'. #6: See #5.2. ------------------------------------------------------------------------ October 29, 2004 - 08:39 : stefan nagtegaal I think - to be more consistent with the rest of drupal - that if the throttle.module is not enabled, you should _not_ display the throttle fields at all. We do this all the time inside drupal, right? I suggest you do something like: <?php - $header = array(t('Block'), t('Enabled'), t('Throttle'), t('Weight'), t('Region'), array('data' => t('Operations'), 'colspan' => 2)); + $header = array(t('Block'), t('Enabled'), (module_exist('throttle') ? t('Throttle')), t('Weight'), t('Region'), array('data' => t('Operations'), 'colspan' => 2)); ?> and: <?php - $rows[] = array($block['info'], array('data' => form_checkbox(NULL, $block['module'] .']['. $block['delta'] .'][status', 1, $block['status']), 'align' => 'center'), array('data' => form_checkbox(NULL, $block['module'] .']['. $block['delta'] .'][throttle', 1, $block['throttle'], NULL, module_exist('throttle') ? NULL : array('disabled' => 'disabled')), 'align' => 'center'), form_weight(NULL, $block['module'] .']['. $block['delta'] .'][weight', $block['weight']), form_radios(NULL, $block['module'] .']['. $block['delta'] .'][region', $block['region'], array(t('left'), t('right'))), l(t('configure'), 'admin/block/configure/'. $block['module'] .'/'. $block['delta']), $operation); + $rows[] = array($block['info'], array('data' => form_checkbox(NULL, $block['module'] .']['. $block['delta'] .'][status', 1, $block['status']), 'align' => 'center'), (module_exist('throttle') ? array('data' => form_checkbox(NULL, $block['module'] .']['. $block['delta'] .'][throttle', 1, $block['throttle'], NULL)), 'align' => 'center'), form_weight(NULL, $block['module'] .']['. $block['delta'] .'][weight', $block['weight']), form_radios(NULL, $block['module'] .']['. $block['delta'] .'][region', $block['region'], array(t('left'), t('right'))), l(t('configure'), 'admin/block/configure/'. $block['module'] .'/'. $block['delta']), $operation); ?> The above patch is untested, and i'm not fully trusted with the fact that it works, but your should get an idea of what I mean.. ------------------------------------------------------------------------ October 29, 2004 - 21:41 : drumm Actually this patch leaves the overview page as is, excpet for removing the columns that are expanded into the configuration page. I believe the standard is to show the throttle configuration, but disabled if not applicable. See the modules administration page. ------------------------------------------------------------------------ October 29, 2004 - 22:15 : Dries I tried the patch: running the update.php script disabled all blocks. Can't look into it right now (lunch) but it is probably an easy fix. user error: Unknown column 'pages_show' in 'field list' query: INSERT INTO blocks (module, delta, status, weight, region, pages_show, pages, custom, throttle) VALUES ('user', '3', 0, 5, 1, 0, '', 0, 1) in /mnt/redhat/foo/cvs/web/drupal.org/cvs/includes/database.mysql.inc on line 125. ------------------------------------------------------------------------ October 30, 2004 - 07:20 : stefan nagtegaal Drumm, I know how the current overview looks and that's why I am asking you to make this fix.. The throttle-checkbox should _not_ be displayed instead of being disabled when the module is not enabled.. If a module is disabled, we don't display their help text and configuration options at all, right? So why are we doing that for the throttle module? Stefan. ------------------------------------------------------------------------ October 31, 2004 - 08:38 : Dries Committed. The UI could do with a tad more polish though, but that can be part of a new patch. Please make sure to update the documentation.

1 0