- Development - lists.drupal.org

[drupal-devel] [bug] Handle full URLs in url() and l()
by gordon 25 May '05

25 May '05

Issue status update for http://drupal.org/node/10888 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: jhriggs Updated by: gordon Status: patch splitting into url() url_external is not a good idea. The main reason for this patch that I see is for putting external urls into the navigation menu using the menu.module, and if all goes to plan, the primary and secondary menus. 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. ------------------------------------------------------------------------ March 14, 2005 - 10:05 : gordon +1 This is a good thing for people who can't using clean urls. ------------------------------------------------------------------------ April 2, 2005 - 16:23 : TDobes This patch seems like a good idea, but does not apply anymore. Also, shouldn't the comments for l() and url() be updated to indicate that the functions will work with both internal and external URL's? ------------------------------------------------------------------------ April 2, 2005 - 17:25 : asimmonds Attachment: http://drupal.org/files/issues/external_urls_1.patch (897 bytes) The &'s had been changed to & amp; , here's a valid patch ------------------------------------------------------------------------ May 13, 2005 - 00:57 : adrian This is a new patch against cvs that allows url() to create external links, with the side-effect that menu can now be used to make external links. Also closes http://drupal.org/node/10177 This will be even more important with the primary links menu integration. (This patch already cleans up a special case in phptemplate for that). Killes benchmarked this, and it turns out the difference is negligible. Requests per second: 1.33 [#/sec] (mean) = with patch Requests per second: 1.31 [#/sec] (mean) = without patch. ------------------------------------------------------------------------ May 13, 2005 - 00:59 : killes(a)www.drop.org I had benchmarked /node as anon user without cache. ------------------------------------------------------------------------ May 25, 2005 - 14:50 : Steven +1 on the idea of this patch. It allows people to use l() for external links. However, I don't like the implementation. Running that complicated regexp for every url() call is simply wasteful. 95% or more of url() calls have a fixed string as parameter and point to Drupal's own paths. I suggest we create an url_external() wrapper around url() which allows external URLs. Any path which we want to allow external URLs in can use url_external() instead. The places where we use it will not be many anyway.

1 0

[drupal-devel] [bug] Handle full URLs in url() and l()
by Steven 25 May '05

25 May '05

Issue status update for http://drupal.org/node/10888 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: jhriggs Updated by: Steven Status: patch +1 on the idea of this patch. It allows people to use l() for external links. However, I don't like the implementation. Running that complicated regexp for every url() call is simply wasteful. 95% or more of url() calls have a fixed string as parameter and point to Drupal's own paths. I suggest we create an url_external() wrapper around url() which allows external URLs. Any path which we want to allow external URLs in can use url_external() instead. The places where we use it will not be many anyway. Steven Previous comments: ------------------------------------------------------------------------ September 16, 2004 - 17: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 - 13:09 : moshe weitzman +1. less HTML in code is a good thing for readability, if nothing else. ------------------------------------------------------------------------ January 24, 2005 - 22: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 - 00:36 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/external_urls_0.patch (909 bytes) Updated from Drupal root directory. ------------------------------------------------------------------------ March 14, 2005 - 02:05 : gordon +1 This is a good thing for people who can't using clean urls. ------------------------------------------------------------------------ April 2, 2005 - 08:23 : TDobes This patch seems like a good idea, but does not apply anymore. Also, shouldn't the comments for l() and url() be updated to indicate that the functions will work with both internal and external URL's? ------------------------------------------------------------------------ April 2, 2005 - 09:25 : asimmonds Attachment: http://drupal.org/files/issues/external_urls_1.patch (897 bytes) The &'s had been changed to & amp; , here's a valid patch ------------------------------------------------------------------------ May 12, 2005 - 16:57 : adrian This is a new patch against cvs that allows url() to create external links, with the side-effect that menu can now be used to make external links. Also closes http://drupal.org/node/10177 This will be even more important with the primary links menu integration. (This patch already cleans up a special case in phptemplate for that). Killes benchmarked this, and it turns out the difference is negligible. Requests per second: 1.33 [#/sec] (mean) = with patch Requests per second: 1.31 [#/sec] (mean) = without patch. ------------------------------------------------------------------------ May 12, 2005 - 16:59 : killes(a)www.drop.org I had benchmarked /node as anon user without cache.

1 0

[drupal-devel] [feature] Replace $_SERVER["REMOTE_ADDR"] with a function which understands X-Forwarded-For
by Steven 25 May '05

25 May '05

Issue status update for http://drupal.org/node/20471 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: degerrit Reported by: degerrit Updated by: Steven Status: patch Sorry didn't look at the patch carefully enough. Yes, we need XSS protection: in practice, we always strore remote_addr() in the hostname column in the db. Wherever that column is displayed, it needs to be check_plain()ed. Steven Previous comments: ------------------------------------------------------------------------ April 14, 2005 - 01:42 : degerrit In several modules, replace $_SERVER["REMOTE_ADDR"] with a function which understands X-Forwarded-For. Proxy servers often (sometimes, always?) insert "X-Forwarded-For" headers which contains the "real" client IP address. I think it is useful to log the client IP address in that situation. This little snippet works for me, and I'd suggest something similar be made into a function? $headers = apache_request_headers(); if (array_key_exists('X-Forwarded-For', $headers)){ $hostname=$headers['X-Forwarded-For'] . ' via ' . $_SERVER["REMOTE_ADDR"]; } else { $hostname=$_SERVER["REMOTE_ADDR"]; } I can make a patch proposal if people like the idea. ------------------------------------------------------------------------ May 21, 2005 - 23:44 : degerrit Attachment: http://drupal.org/files/issues/remote_addr.diff (6.07 KB) Here's my proposal for showing user IP addresses behind proxy servers, especially in log files. It adds a function remote_addr() and replaces all the uses of $_SERVER["REMOTE_ADDR"] with that function. This is my first patch for Drupal, don't be too harsh one me :-) A few notes: - I patched session.inc because $_SERVER["REMOTE_ADDR"] is also called in session.inc and common.inc is not available at that time it seems - maybe I replaced too vigorously in common.inc and sessions.inc, it works for me though - maybe it should be renamed http_remote_addr or apache_remote_addr ------------------------------------------------------------------------ May 22, 2005 - 10:37 : Dries Couple comments: Drupal supports non-Apache webservers: apache_request_headers() might not always be available. Does this patch need input checking? Depends on how/when/where the remote address data is used. Fact is that we don't want people to inject XSS-stuff through HTTP headers. Make sure to double-check this. I'd rename the function to remote_address(). We usually don't abbreviate words like 'address'. The word 'via' can't be translated. We write } else { and not } else {Just thinking up loud. Is it easy to fake or spoof X-Forwarded-For-headers? If it is, it would be easy to trick Drupal (eg. in the statistics.module's log pages we group by remote address). For the watchdog it doesn't matter, but for the statistics module it does. Maybe we need a toggle so remote_address() can be told not to return the 'true IP address'. ------------------------------------------------------------------------ May 25, 2005 - 06:37 : Steven X-Forwarded-For can be spoofed without any trouble. Just connect and send the fake header. I don't think we should pay attention to it. If we do, we need to also store the original IP address. Perhaps we could do "real.ip.#.# (forwarded.ip.#.#)"

1 0

[drupal-devel] [feature] Replace $_SERVER["REMOTE_ADDR"] with a function which understands X-Forwarded-For
by Steven 25 May '05

25 May '05

Issue status update for http://drupal.org/node/20471 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: degerrit Reported by: degerrit Updated by: Steven Status: patch X-Forwarded-For can be spoofed without any trouble. Just connect and send the fake header. I don't think we should pay attention to it. If we do, we need to also store the original IP address. Perhaps we could do "real.ip.#.# (forwarded.ip.#.#)" Steven Previous comments: ------------------------------------------------------------------------ April 14, 2005 - 01:42 : degerrit In several modules, replace $_SERVER["REMOTE_ADDR"] with a function which understands X-Forwarded-For. Proxy servers often (sometimes, always?) insert "X-Forwarded-For" headers which contains the "real" client IP address. I think it is useful to log the client IP address in that situation. This little snippet works for me, and I'd suggest something similar be made into a function? $headers = apache_request_headers(); if (array_key_exists('X-Forwarded-For', $headers)){ $hostname=$headers['X-Forwarded-For'] . ' via ' . $_SERVER["REMOTE_ADDR"]; } else { $hostname=$_SERVER["REMOTE_ADDR"]; } I can make a patch proposal if people like the idea. ------------------------------------------------------------------------ May 21, 2005 - 23:44 : degerrit Attachment: http://drupal.org/files/issues/remote_addr.diff (6.07 KB) Here's my proposal for showing user IP addresses behind proxy servers, especially in log files. It adds a function remote_addr() and replaces all the uses of $_SERVER["REMOTE_ADDR"] with that function. This is my first patch for Drupal, don't be too harsh one me :-) A few notes: - I patched session.inc because $_SERVER["REMOTE_ADDR"] is also called in session.inc and common.inc is not available at that time it seems - maybe I replaced too vigorously in common.inc and sessions.inc, it works for me though - maybe it should be renamed http_remote_addr or apache_remote_addr ------------------------------------------------------------------------ May 22, 2005 - 10:37 : Dries Couple comments: Drupal supports non-Apache webservers: apache_request_headers() might not always be available. Does this patch need input checking? Depends on how/when/where the remote address data is used. Fact is that we don't want people to inject XSS-stuff through HTTP headers. Make sure to double-check this. I'd rename the function to remote_address(). We usually don't abbreviate words like 'address'. The word 'via' can't be translated. We write } else { and not } else {Just thinking up loud. Is it easy to fake or spoof X-Forwarded-For-headers? If it is, it would be easy to trick Drupal (eg. in the statistics.module's log pages we group by remote address). For the watchdog it doesn't matter, but for the statistics module it does. Maybe we need a toggle so remote_address() can be told not to return the 'true IP address'.

1 0

[drupal-devel] [feature] Enable customizing of "Title" field on node forms
by Steven 25 May '05

25 May '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: Steven Status: patch To make this patch work you also need to move the title validation out of node.module and into the node modules. I don't think this is that much of a problem, it is code that is not going to be touched much. Also, in light of the upcoming CCK it makes sense to do it like this. Steven Previous comments: ------------------------------------------------------------------------ August 14, 2004 - 20: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 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 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 - 18:39 : Dries This is best accomplished through the existing nodeapi hook, using a separate module. ------------------------------------------------------------------------ August 16, 2004 - 01: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 - 20: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 - 20: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 - 01: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 - 17: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 - 15: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 - 15: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 - 23: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 - 14:06 : moshe weitzman please patch all core node modules as well. ------------------------------------------------------------------------ November 28, 2004 - 03: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 - 19: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 - 05: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 - 05: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 - 07: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 - 16: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. ------------------------------------------------------------------------ April 3, 2005 - 07:28 : chx 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. ------------------------------------------------------------------------ May 20, 2005 - 12:09 : chx Patch still applies (yes, with some offset, but applies). ANd yes, we need this very badly. ------------------------------------------------------------------------ May 20, 2005 - 12:18 : Thox I'd benefit greatly from having a feature like this. The example already given is one of the cases I need it: I'd like the title field to be a concatenation of two fields: "First name" and "Last name". ------------------------------------------------------------------------ May 24, 2005 - 05:39 : gsperk I wonder if someone could help me with the following problem. After applying the patch I made the output for 'title' in my own_node.module this: $output .= form_hidden('title', $node->firstname . ' ' . $node->surname); When the form is first submitted I get the error 'You have to specify a title'. When I submit again the firstname/surname variables are obviously in $node, and the form submits with the desired title. So, I suppose my question is, how can get the values for firstname and surname into $node without submitting twice? (If it's not obvious I can confirm that I dont have much experience with this kind of stuff. Any suggestions would be a big help.)

1 0

[drupal-devel] [bug] non-standard form fields
by Steven 25 May '05

25 May '05

Issue status update for http://drupal.org/node/23531 Project: Drupal Version: cvs Component: user.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Alex Reisner Updated by: Steven Status: patch We can use container-inline to put the two password boxes on one row, no? Steven Previous comments: ------------------------------------------------------------------------ May 24, 2005 - 19:40 : Alex Reisner User.module manually creates several form fields (on lines 1091 and 1664) rather than calling the form_ functions in common.inc. The fields on line 1664 are lacking a class attribute and all are missing id attributes, making their appearances difficult to control. It strikes me as better if the form_ functions were used, but I don't know enough about user.module to know if this is possible. At least the class and id info should be added manually. Relevant code: *Line 1091* $group .= form_item(t('Password'), '<input type="password" class="form-password" name="edit[pass1]" size="12" maxlength="24" /> <input type="password" class="form-password" name="edit[pass2]" size="12" maxlength="24" />', t('Enter your new password twice if you want to change your current password, or leave it blank if you are happy with your current password.'), NULL, TRUE); *Line 1664* $rows[] = array('<input type="text" size="32" maxlength="64" name="edit[name]" />', '<input type="submit" name="op" value="'. t('Add role') .'" />'); ------------------------------------------------------------------------ May 24, 2005 - 20:12 : Robin Monks Attachment: http://drupal.org/files/issues/hard.coded.form.fields.patch (2.19 KB) This patch should take care of that. I was unable to get someone to review it, so it may have a rough spot. Robin ------------------------------------------------------------------------ May 24, 2005 - 22:31 : Dries IMO, the usability of the forms degrades with this patch. It makes them look weird-ish.

1 0

[drupal-devel] [bug] Invalid form HTML due to ][ characters not allowed in ID attribute
by Thox 25 May '05

25 May '05

Issue status update for http://drupal.org/node/23516 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Thox Updated by: Thox Status: patch Attachment: http://drupal.org/files/issues/form_clean_id.patch (7.96 KB) Attached patch makes Drupal forms slightly more valid. It removes "][" from ID attributes in forms and replaces it with a dash (-). In the future, we might clean out more characters, but these ones are used throughout Drupal. Thox

2 1

[drupal-devel] [bug] Clean up pager code
by Steven 25 May '05

25 May '05

Issue status update for http://drupal.org/node/23495 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: Steven Reported by: Steven Updated by: Steven Status: patch Attachment: http://drupal.org/files/issues/pager_1.patch (20.22 KB) FactoryJoe recently asked me about theming the pager. Looking at the code, I decided it was way too much uncommented voodoo. So here's the cleanup patch. The biggest change is that before this patch, the pager would count items. When there are 5 items per page, the urls would go from=0, from=5, from=10, etc. The code also handled in between values, e.g. from=2 or from=13. IMO this 'in between' feature is useless (you can only use it by typing in the url yourself, and once you do the pager remembers the in-page offset) and it complicates the code. Instead of this, I made the pager count page numbers. 'from' is simply an integer per pager: 0, 1, 2, 3... This makes the code simpler. I also commented it some more, and cleaned up some inconsistencies in the code. Finally, the code had a lot of (int) casts in it. Most of these were unnecessary as the variable was either the result of a calculation, or used in a calculation. I got rid of those that had no effect. Functionally, nothing changes except the meaning of "from" in the pager URLs. I hardly think anyone would link to a specific pager page. Steven

7 10

[drupal-devel] Where to put contact.module
by neil＠civicspacelabs.org 25 May '05

25 May '05

I used to keep the contact module on Drupal CVS. Approximately November of last year, CivicSpace decided to stop development on the contact module in favor of waiting for CivicCRM. Its code is written for Drupal 4.4 and it still doesn't have full support for the node permissions system and is not compatible with the latest version of the forms module. To get it fully working with the features of Drupal 4.6 would probably be equivalent to a full rewrite and testing. Probably a solid week of coding and testing which I don't have. I'd like to move this back to Drupal CVS since the volunteer module requires it and I'd like for all CivicSpace modules to have Drupal.org projects. However I need some way of dealing with the forked forms module and I do not plan on considering any feature requests at this point. Any objections or other comments? -Neil

2 4

[drupal-devel] [bug] Forum module's use of if in Postgresql
by patrickslee 25 May '05

25 May '05

Issue status update for http://drupal.org/node/23499 Project: Drupal Version: 4.6.0 Component: forum.module Category: bug reports Priority: normal Assigned to: patrickslee Reported by: cchamilt Updated by: patrickslee Status: patch Oops... my <>s are stolen. IF(l.last_comment_uid <> 0 AND l.last_comment_uid <> NULL, cu.name, l.last_comment_name) patrickslee Previous comments: ------------------------------------------------------------------------ May 24, 2005 - 16:32 : cchamilt Postgresql function "if" is defined to return a boolean value, yet forum requests an integer - last_comment_uid. user error: query: SELECT n.nid, l.last_comment_timestamp, IF(l.last_comment_uid, cu.name, l.last_comment_name) as last_comment_name, l.last_comment_uid FROM node n, node_comment_statistics l /*! USE INDEX (node_comment_timestamp) */, users cu, term_node r WHERE n.nid = r.nid AND r.tid = 1 AND n.status = 1 AND n.type = 'forum' AND l.last_comment_uid = cu.uid AND n.nid = l.nid ORDER BY l.last_comment_timestamp DESC LIMIT 1 OFFSET 0 in /var/www/hosts/www.ambigc.com/htdocs/includes/database.pgsql.inc on line 62. warning: pg_query() [function.pg-query]: Query failed: ERROR: function if(integer, character varying, character varying) does not exist HINT: No function matches the given name and argument types. You may need to add explicit type casts. in /var/www/hosts/www.ambigc.com/htdocs/includes/database.pgsql.inc on line 45. ------------------------------------------------------------------------ May 25, 2005 - 10:07 : patrickslee Attachment: http://drupal.org/files/issues/forum.module_3.patch (2.69 KB) I have also encountered this bug and made a possible patch by replacing IF statements to CASE. A patch file is attached. ------------------------------------------------------------------------ May 25, 2005 - 10:28 : adrian There is an IF function defined in the database.pgsql function. On what page is this happening, as what might be happening is that the type isn't being picked up correctly. It's defined as IF(anyelement, anyelement, text) returns anyelement. IIRC. What I have seen happen is that it wants the text field to return integer, or vice versa. ------------------------------------------------------------------------ May 25, 2005 - 10:32 : patrickslee I found the definition in database.pgsql. It is like this: CREATE FUNCTION "if"(boolean, anyelement, anyelement) RETURNS anyelement AS ' SELECT CASE WHEN $1 THEN $2 ELSE $3 END; ' LANGUAGE 'sql'; So it is actually wanting boolean. ------------------------------------------------------------------------ May 25, 2005 - 10:43 : patrickslee So my patch will work, and we can make it easier by replacing the IF function to: IF(l.last_comment_uid 0 AND l.last_comment_uid NULL, cu.name, l.last_comment_name) BTW there is another place in forum.module use IF like this.

1 0