- Development - lists.drupal.org

[drupal-devel] [bug] Deleting Users & validate role name
by kubaZygmunt 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/27951 Post a follow up: http://drupal.org/project/comments/add/27951 Project: Drupal Version: cvs Component: user.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: kubaZygmunt Updated by: kubaZygmunt Status: patch (code needs review) Attachment: http://drupal.org/files/issues/user.module.patch.txt (914 bytes) I've found two bugs in user.module 1. Deleting button in admin section doesn't work ( deleting works only by url user/$id/delete ), I've upadated condition 2. If you change role name to 0 then system deletes role. (I've added isset($edit['name') ) kubaZygmunt

2 2

[drupal-devel] [feature] Enable override and extension of core module functions
by nedjo 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/29428 Post a follow up: http://drupal.org/project/comments/add/29428 Project: Drupal Version: cvs Component: module system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: nedjo Updated by: nedjo Status: patch (code needs review) Attachment: http://drupal.org/files/issues/module-extension-and-override.patch (1.38 KB) A frequently requested functionality is the ability to override or extend core module functions. This small patch is an initial take on how to enable function extension and overriding. It takes the existing module_invoke() function and adds tests for override and extension functions, in the forms modulename_originalfunction_override() and modulename_originalfunction_extension(). So, for example, an override function for the core taxonomy_node_form() function as defined by a module named testmodule would be called testmodule_taxonomy_node_form_override(), and would be run instead of taxonomy_node_form(). And testmodule_taxonomy_node_form_extension() would be run every time taxonomy_node_form() was called, hence "extending" it. Of course, this approach would only be useful if we converted most - or all! - of our current module function calls to use module_invoke()... nedjo

8 15

[drupal-devel] [feature] Enhance hook_settings - add an optional 'advanced' tab
by Bèr Kessels 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/30515 Post a follow up: http://drupal.org/project/comments/add/30515 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: moshe weitzman Reported by: moshe weitzman Updated by: Bèr Kessels Status: patch (code needs review) -1 for the concept. "Advanced" settings are bad UI. It is not up to us to decide what is "advanced" and what not. You also simpley split them up on an imaginary and non-existing border. Tehre is no difference between advanced and simple, so making usch a border is bad. On top of that, "advaced" allows lazy UI design. "I would like to have a better look over how to render these options, and I still need to write better descriptions" vs "hmm, dunno what to do with them yet, lets stick em in advanced". That off course is over siplified, but I hope it exlpains the problem. Please, please, do not introduce "advanced" tabs or areas. No modern environments (desktops, apps etc) have this, for the reason that "advanced" is a non-existing group. (well except FF, but it is a much debated issue, and is considered one of teh top UI mistakes in FF). If this does not convince you people, I will try to find some good online resources, for II only know this from what I read in usability books. Also, when discussing usability issues please just post a screenshot with the patch, or even jsut a mockup before a patch. Usability is *not* about code, thus a patch is not a good method of communication. A mockup also saves a lot of time when developing issues. And last, this, in fact is a duplicate of: http://drupal.org/node/30843 one of these two should be marked as dup. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Tue, 06 Sep 2005 16:05:53 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/hook_settings.patch (25.19 KB) This patch opens the door for advanced preferences in Drupal. If you have sufficient permissions, you will see an *advanced* tab on settings pages when a module chooses to put stuff there. The permission is global across all Drupal - 'access advanced functionality'. This patch enhances hook_settings so that it accepts a $type parameter. By default, $type='simple' and everything works as today. If modules return HTML when $type=advanced, a new tab appears on the settings page. To see this in action, visit admin/settings/aggregator applying the patch and after giving yourself the permission above. The patch appears long only because of whitespace changes. It is a minor change to all hook_settings() implementations. This 'advanced tab' paradigm can be used outside hook_settings as well. Just protect your tab/feature with the permission above. Contrib modules will continue to work without patching but will show an duplicate tab called 'advanced'. To fix, just add an if($type == 'simple') block in hook_settings(). I will add upgrading instructions to drupal.org and update hook_settings() docs if this is committed. I feel that this is a good balance between those who repeatedly yell for 'fewer settings' and those who want their site to work a specific way but are uneager to hack the source code. This 'advanced settings' paradigm has worked well for Mozilla project (about:config) and many others. If these advanced features clutter and bother you, don't enable this permission or don't click on the advanced tab. There is no way that Drupal becomes a cluttering steaming pile of configuration options - our culture precludes this. Lighten up, and give the people what they want. We can't expect everyone to hack the source to get commonly requested behavior. I also simplified upload config by renaming upload_admin() to upload_settings() and removing an unneeded menu entry. ------------------------------------------------------------------------ Sun, 11 Sep 2005 05:28:51 +0000 : moshe weitzman i'd like to hear some comments from CVS review team on this. If not desired, alternatives are appreciated. ------------------------------------------------------------------------ Sun, 11 Sep 2005 06:10:03 +0000 : Crell This was discussed recently on the -devel mailing list, and for reasons discussed there I have to -1 this in concept. An "Advanced" tab means the UI designer failed to properly design the UI to discourage the user from doing stupid things. That's not something we should encourage. If there are too many configuration options in some module for them to be just shown linearly (I don't actually know many modules with that problem), that's what <fieldset> is for. If some module has enough options that even <fieldset> isn't enough, then rather than hard-coding an "advanced" tab we should allow arbitrary module-developer-defined tabs that all get the extra magic of the settings page. The $delta methodology of hook_block() would be a good model to follow here. If you just want to split some options off as "Danger, Will Robinson!", frankly I can't think of any good examples. Perhaps you could offer some? ------------------------------------------------------------------------ Sun, 11 Sep 2005 14:16:23 +0000 : Jose A Reyero + 1 for the concept I think we are always trying to find what is an imposible balance between ease of use (less settings) and options/flexibility (more settings), so having simple and advanced settings would be a good thing. However, about the implementation, I wouldn't like having an 'advanced' tab, so from an advanced administrator's point of view you would have to guess which settings are in 'simple' and which ones are 'advanced' to find them. I'd rather have a single page having all settings -that would be more if you have that advanced permission. So you can just return all simple settings as deffault, and then a few more if the parameter is 'advanced', and then we'd need to rewrite that settings hook only when additional advanced settings are provided.

1 0

[drupal-devel] [feature] New themable function theme_search_form to unify search form and put logic at the correct level
by robertDouglass 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/27843 Post a follow up: http://drupal.org/project/comments/add/27843 Project: Drupal Version: cvs Component: search.module Category: feature requests Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: robertDouglass Status: patch Attachment: http://drupal.org/files/issues/make_search_themable.txt (3.99 KB) The search form should be themable, but it shouldn't be left to the theme('page') function to do this, as is the current implementation. This patch adds a theme_search_form function to theme.inc (not search module because that would cause a dependency) and removes the then unneeded variables from the phptemplate.engine. Pushbutton and Bluemarine are updated to take advantage of the new function. In the current implementation, several variables are created needlessly in phptemplate.engine (like the text for the button) and then pushed to the template. This seems messy and this patch fixes that. Remaining questions: should the function accept any parameters? I think not, but other opinions are welcome. robertDouglass

3 2

[drupal-devel] [feature] Multiple simple nodes
by chx 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/29794 Post a follow up: http://drupal.org/project/comments/add/29794 Project: Drupal Version: cvs Component: node system Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch (code needs review) Attachment: http://drupal.org/files/issues/simplenode.module (9.09 KB) Instead of page, story etc let's have a module with which you can add simple body only nodes. At least moshe and I frequently copy story.module to have a new simple module. For example, event needs this. For permissions it's great to have multiple node types. So I downloaded a copy of content.module from CCK, pressed delete a lot, copied hook_form from story and then rename the stuff to simplenode.module. Then I reviewed the thing but it was really not much, so credit goes to JonBob and jvandyk. MySQL is: CREATE TABLE node_type ( type_name varchar(31) NOT NULL default '', label varchar(255) NOT NULL default '', description mediumtext NOT NULL, help mediumtext NOT NULL, PRIMARY KEY (type_name) ) TYPE=MyISAM; chx

7 20

[drupal-devel] [bug] Remove PHP5 warnings about unassigned variables and functions being used
by fgm 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/30800 Post a follow up: http://drupal.org/project/comments/add/30800 Project: Drupal Version: 4.6.3 Component: base system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: fgm Updated by: fgm Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/missing_isset.zip (2.38 KB) This is a set of similar patches to several files: bootstrap.inc, common.inc, pager.inc, devel.module, filter.module, node.module, user.module, watchdog.module They are all there to maintain identical functionality but remove warnings caused by code like: $somevar = $somearray['somekey'] ; where the existing code does not check whether $somearray contains an entry for 'somekey', or even whether $somearray actually exists. Another change in this patch conditionally restores the missing timer_read function in devel.module, by first checking whether or not it is already defined. Packaged as two patches, one for the includes directory, the other for the modules directory fgm

2 6

[drupal-devel] [bug] Handle full URLs in url() and l()
by robertDouglass 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/10888 Post a follow up: http://drupal.org/project/comments/add/10888 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: Bèr Kessels Reported by: jhriggs Updated by: robertDouglass Status: patch (code needs work) You might update the bit in theme.inc as well and include that in the patch. robertDouglass Previous comments: ------------------------------------------------------------------------ Thu, 16 Sep 2004 15:01:24 +0000 : 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 ?> ------------------------------------------------------------------------ Thu, 30 Sep 2004 11:09:30 +0000 : moshe weitzman +1. less HTML in code is a good thing for readability, if nothing else. ------------------------------------------------------------------------ Mon, 24 Jan 2005 20:06:26 +0000 : 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. ------------------------------------------------------------------------ Sun, 13 Mar 2005 22:36:34 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/external_urls_0.patch (909 bytes) Updated from Drupal root directory. ------------------------------------------------------------------------ Mon, 14 Mar 2005 00:05:17 +0000 : gordon +1 This is a good thing for people who can't using clean urls. ------------------------------------------------------------------------ Sat, 02 Apr 2005 06:23:10 +0000 : 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? ------------------------------------------------------------------------ Sat, 02 Apr 2005 07:25:13 +0000 : 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 ------------------------------------------------------------------------ Thu, 12 May 2005 14:57:07 +0000 : 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. ------------------------------------------------------------------------ Thu, 12 May 2005 14:59:57 +0000 : killes(a)www.drop.org I had benchmarked /node as anon user without cache. ------------------------------------------------------------------------ Wed, 25 May 2005 04:50:08 +0000 : 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. ------------------------------------------------------------------------ Wed, 25 May 2005 05:05:55 +0000 : gordon 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. ------------------------------------------------------------------------ Wed, 25 May 2005 06:00:01 +0000 : Steven gordon: I still think this is a bit excessive. Perhaps instead of valid_url() we could use a simpler regexp !^[a-z]://!i or even strpos('://'). ------------------------------------------------------------------------ Wed, 25 May 2005 06:15:36 +0000 : gordon I am not says that it is or is not excessive. I was just saying that this is where it is going to be used, so creating url() and url_external() is not really an option ------------------------------------------------------------------------ Wed, 25 May 2005 06:57:53 +0000 : Bèr Kessels we /do/ need an external_url() thing. The links bundle (handling weblinks and the likes) willprovide this /and/ need this. People will get all sorts of tools to do /stuff/ with external links, like add permissions, track clicks etc. depending on the modules they have and the settings. All outgoing weblinks, should somehow respect those. Patches for core are coming, though. On top of that, a external_l() finction is simple. And if people want them in primary links they can do the regexping /before/ the function, not /in/ the function. The calling code can decide whether to call l() or external_l(), instead of putting logic in l() that is unneded 95% of the times. But i hereby promise to introduce such a function tbefore the end of this week :) ------------------------------------------------------------------------ Wed, 25 May 2005 07:17:47 +0000 : gordon The only thing I do not see in this plan is how with this work with the menu module and the navigation menu. ------------------------------------------------------------------------ Wed, 25 May 2005 10:04:12 +0000 : adrian Attachment: http://drupal.org/files/issues/url_external_links.patch (1.75 KB) What happened to the patch i uploaded ? It doesn't use a complex regexp, but uses strpos($url, '://') < 5. ------------------------------------------------------------------------ Thu, 26 May 2005 06:19:32 +0000 : Bèr Kessels Allthough this works, is clean AND does what it should do, I still am in favour of a better nitegrated solution. So please do not yet commit this, untill I upload my patch. We can then discuss what solution to take. It will not differ a big lot though. ------------------------------------------------------------------------ Thu, 26 May 2005 08:39:32 +0000 : adrian is this the patch where you want to introduce an el() to allow for counting of external links ? That's a different patch altogether. This is primarily about stopping url() from not handling external links when there is no good reason for it to do so. Your el() has a very specific use in the weblinks api, and should probably be included with it. ------------------------------------------------------------------------ Thu, 26 May 2005 16:18:31 +0000 : Bèr Kessels Yes, that el() But let me explain why i am anxious for my solution: el(), by default will not do much, in fact it will just be an URL()-clone but one that allows outgoing links. But it has a hook-caller. Again: by default it does nothing much, for there will not be any hooks around in core. It will allow, for example, weblinks, to track clicks, or to look up a permission and allow or disallow users to follow a link. Now, the whole issue, IMO is, that we should *not* provide two ways for handling external URLS, but rather a single one, that is weblink-safe. If we would introduce URL() with outgoing links-capabilities, we can no longer track any outgoing URLs trough them. We will then have two ways for handling outgoing URLs, resulting in inconsistencies, and confused users. (hey, my anonymous users can follow link foo, while i have "follow outgig links" permission disabled for them). (BTW weblink will soon be renamed into links, just for reference) ------------------------------------------------------------------------ Thu, 30 Jun 2005 11:06:11 +0000 : adrian Attachment: http://drupal.org/files/issues/url_external_links_0.patch (824 bytes) I've updated the patch. ------------------------------------------------------------------------ Mon, 18 Jul 2005 14:12:36 +0000 : adrian spoke to dries about this patch, and apart from the incomplete patch I uploaded, there are about a dozen instances where in-line html is used were l() would suffice. I will be cleaning them up and submitting a new patch soon. ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:44:03 +0000 : moshe weitzman hopefully adrian or someone else will chase down the remaining urls which should use this function. ------------------------------------------------------------------------ Fri, 05 Aug 2005 19:25:08 +0000 : jsloan I've come to this party late but would like to see a patch soon... I've used the patch from Adrian (#15) but this did not work correctly in my setup (IIS 5.0 / no clean urls) I applied it against 4.6.2 and cvs. Where is the latest patch that I can test ... I'll throw some time at this because I need it for a module used within our Intranet. ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:06:03 +0000 : jsloan Attachment: http://drupal.org/files/issues/external_urls_2.patch (3.67 KB) I'm not sure what the final plans are for this in the CVS so I have developed a patch for use on 4.6.2 I decided to add a parameter named $external that is passed to url() function. The default is FALSE so this should not break any existing calls to either l() or url(). I have also refactored url() so that it is much easier to understand and extend --- I'm thinking of Bèr Kessels intention to "allow, for example, weblinks, to track clicks, or to look up a permission and allow or disallow users to follow a link" " Since the parameter is passed to activate the external url there is no need for regex or strpos, this should be handled by the calling function and then set the $external parameter to TRUE For example I have patched the theme_menu_item_link() function in menu.inc to accept external urls: <?php function theme_menu_item_link($item, $link_item) { // this next line was added so that a menu item could point to the default home $link_item['path'] = preg_replace('/^\/$/','',$link_item['path']); // this next line was added so that a menu item could point to an external url $external = (preg_match('<^[a-z]+://>', $link_item['path'])) ? TRUE : FALSE ; return l($item['title'], $link_item['path'], array_key_exists('description', $item) ? array('title' => $items['description']) : array(),NULL,NULL,FALSE,FALSE,$external); } ?> I hope that this patch will spur some solutions - I'm using this now in production but have not tested extensively across every scenario. thanks jim sloan ------------------------------------------------------------------------ Fri, 19 Aug 2005 12:10:50 +0000 : jsloan ... are there any thoughts on this? What is the status of the final patch? ------------------------------------------------------------------------ Sun, 28 Aug 2005 15:48:08 +0000 : adrian The problem with this patch is that we still need to fix all the inline occurrences of the <a href tag. I don't think we should confuse the issue and implement a hook at this point, as url() breaking with external links is a bug, pure and simple. ------------------------------------------------------------------------ Thu, 08 Sep 2005 15:24:16 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/theme.inc.patch.txt (1.16 KB) Adrian, your second patch for theme.inc looks wrong to me. Don't you still need the line $settings[$type .'_links'][] = l($text, $link, $attributes); ? I also got rid of unset($attributes); Where was that supposed to come from that it needed unsetting? ------------------------------------------------------------------------ Thu, 08 Sep 2005 15:48:02 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/theme.common.inc.patch.txt (1.83 KB) Here is a unified patch that I updated to fix a bug in Adrian's common.inc code (everything was being handled as absolute), plus the update to theme.inc that I questioned in the previous post. In my initial testing, this works perfectly. I can put absolute URLs in for the paths in menu items and it handles them fine, and changes no other behavior. I searched core for all uses of url() and http:// and found no more violating code such as that in theme.inc. Net change in core with this patch: 0 lines of code. Please review quickly, I'm writing in the upcoming book about this functionality as if it is in. =) ------------------------------------------------------------------------ Thu, 08 Sep 2005 15:49:43 +0000 : robertDouglass Correction: net change to core -1 line of code. ------------------------------------------------------------------------ Thu, 08 Sep 2005 18:58:01 +0000 : adrian What needs to be removed from core are not uses of url() and l(), but all occurences of the anchor tag. The anchor tag was mostly used because l() didn't do external links. Also, in the case of phptemplate , that is the line that creates the array that is passed to the template. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:58:13 +0000 : robertDouglass Good - that needs to be removed too, but is there anything that doesn't work right if we don't remove them? Lots of the $output = '<a href....' instances in the code use url() for href and these are not broken. They can't be seen as a reason not to commit this particular patch. If anyone point out things that break with the patch I made, I'll fix them. I can't go through and clean up anchor tags on idealistic grounds at the moment (as much as I would otherwise like to). ------------------------------------------------------------------------ Sat, 10 Sep 2005 20:11:08 +0000 : jsloan I think that using strpos($path, '://') to trigger the external url is a bad idea. My patch implements a new parameter to url() and l() that will let the calling function decided if it is an external url. And it is backward compatible with all existing calls. I also rewrote the logic of builing the url to make it simple. Adrian, I do not agree that it was a bug; url() never intended to handle external url´s and also, the clean up of the inline anchor tags are not dependent on this feature. Can this be considered for the solution to this feature? ------------------------------------------------------------------------ Sun, 11 Sep 2005 02:14:10 +0000 : jsloan ... the patch I am referring to is above at # 23 ------------------------------------------------------------------------ Sun, 11 Sep 2005 07:27:14 +0000 : robertDouglass "I think that using strpos($path, '://') to trigger the external url is a bad idea. " Please explain why. The only thing that your regexp does that is different is check to see if the part of the string before the :// is within the range a-z, and for that you use a more expensive PHP function. Please indicate the cases where strpos($path, '://') won't work correctly. "My patch implements a new parameter to url() and l() that will let the calling function decided if it is an external url. And it is backward compatible with all existing calls. " This means that instead of doing one check in a centralized place, programmers now have to write checks anytime they want to call this function with an either/or case. I don't see why this is better. "I also rewrote the logic of builing the url to make it simple. " This is good. "Adrian, I do not agree that it was a bug; url() never intended to handle external url´s and also, the clean up of the inline anchor tags are not dependent on this feature. " I'm still unclear on why anything else in the code base needs to be changed before this functionality can be accepted. I stick with my patch as the preferred solution. ------------------------------------------------------------------------ Sun, 11 Sep 2005 14:58:55 +0000 : jsloan "Please explain why " My post #23 has confused you. The patch itself does not add a strpos() or regexp() ... the code snippet I posted is an example of the calling function´s responsibility of determining the use of the external link and then passing that parameter on to the l() or url() function. I believe it is a bad idea to use string parsing at this low level to determine functionality because it will impose this behavior on all calls. There could be a case in which this would be a problem; such as a url that could contain another url as a parameter. index.php?q=module&param=http://example.com ... and there are some who would balk at the processing overhead of pattern matching at his level. "This means that instead of doing one check in a centralized place, programmers now have to write checks anytime they want to call this function with an either/or case. I don't see why this is better. " This is better because the module author is the proper person to determine whether the link is external. Note that in the case of all internal links there is no change of coding practice at all. It is when the module function must handle external links that module´s author can include the various context and string pattern tests to set the $external parameter for the l() or url() call. This is what I was demonstrating in the code example of post #23. "I'm still unclear on why anything else in the code base needs to be changed before this functionality can be accepted. I stick with my patch as the preferred solution. " I think that we agree on this point. Nothing needs to be changed in the code base – my patch is backward compatible to all existing calls and the clean up work on the in-line anchor tags can (and should) take place regardless of this patch. ------------------------------------------------------------------------ Sun, 11 Sep 2005 15:04:13 +0000 : robertDouglass Thanks for the clarification. 1) does your patch still apply well? 2) could you please add the update to the theme menu item to the main patch so that menu items can be external? I had forgotten about urls containing urls. That's clearly a showstopper. -Robert

1 0

[drupal-devel] [bug] Handle full URLs in url() and l()
by robertDouglass 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/10888 Post a follow up: http://drupal.org/project/comments/add/10888 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: Bèr Kessels Reported by: jhriggs Updated by: robertDouglass Status: patch (code needs work) Thanks for the clarification. 1) does your patch still apply well? 2) could you please add the update to the theme menu item to the main patch so that menu items can be external? I had forgotten about urls containing urls. That's clearly a showstopper. -Robert robertDouglass Previous comments: ------------------------------------------------------------------------ Thu, 16 Sep 2004 15:01:24 +0000 : 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 ?> ------------------------------------------------------------------------ Thu, 30 Sep 2004 11:09:30 +0000 : moshe weitzman +1. less HTML in code is a good thing for readability, if nothing else. ------------------------------------------------------------------------ Mon, 24 Jan 2005 20:06:26 +0000 : 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. ------------------------------------------------------------------------ Sun, 13 Mar 2005 22:36:34 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/external_urls_0.patch (909 bytes) Updated from Drupal root directory. ------------------------------------------------------------------------ Mon, 14 Mar 2005 00:05:17 +0000 : gordon +1 This is a good thing for people who can't using clean urls. ------------------------------------------------------------------------ Sat, 02 Apr 2005 06:23:10 +0000 : 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? ------------------------------------------------------------------------ Sat, 02 Apr 2005 07:25:13 +0000 : 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 ------------------------------------------------------------------------ Thu, 12 May 2005 14:57:07 +0000 : 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. ------------------------------------------------------------------------ Thu, 12 May 2005 14:59:57 +0000 : killes(a)www.drop.org I had benchmarked /node as anon user without cache. ------------------------------------------------------------------------ Wed, 25 May 2005 04:50:08 +0000 : 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. ------------------------------------------------------------------------ Wed, 25 May 2005 05:05:55 +0000 : gordon 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. ------------------------------------------------------------------------ Wed, 25 May 2005 06:00:01 +0000 : Steven gordon: I still think this is a bit excessive. Perhaps instead of valid_url() we could use a simpler regexp !^[a-z]://!i or even strpos('://'). ------------------------------------------------------------------------ Wed, 25 May 2005 06:15:36 +0000 : gordon I am not says that it is or is not excessive. I was just saying that this is where it is going to be used, so creating url() and url_external() is not really an option ------------------------------------------------------------------------ Wed, 25 May 2005 06:57:53 +0000 : Bèr Kessels we /do/ need an external_url() thing. The links bundle (handling weblinks and the likes) willprovide this /and/ need this. People will get all sorts of tools to do /stuff/ with external links, like add permissions, track clicks etc. depending on the modules they have and the settings. All outgoing weblinks, should somehow respect those. Patches for core are coming, though. On top of that, a external_l() finction is simple. And if people want them in primary links they can do the regexping /before/ the function, not /in/ the function. The calling code can decide whether to call l() or external_l(), instead of putting logic in l() that is unneded 95% of the times. But i hereby promise to introduce such a function tbefore the end of this week :) ------------------------------------------------------------------------ Wed, 25 May 2005 07:17:47 +0000 : gordon The only thing I do not see in this plan is how with this work with the menu module and the navigation menu. ------------------------------------------------------------------------ Wed, 25 May 2005 10:04:12 +0000 : adrian Attachment: http://drupal.org/files/issues/url_external_links.patch (1.75 KB) What happened to the patch i uploaded ? It doesn't use a complex regexp, but uses strpos($url, '://') < 5. ------------------------------------------------------------------------ Thu, 26 May 2005 06:19:32 +0000 : Bèr Kessels Allthough this works, is clean AND does what it should do, I still am in favour of a better nitegrated solution. So please do not yet commit this, untill I upload my patch. We can then discuss what solution to take. It will not differ a big lot though. ------------------------------------------------------------------------ Thu, 26 May 2005 08:39:32 +0000 : adrian is this the patch where you want to introduce an el() to allow for counting of external links ? That's a different patch altogether. This is primarily about stopping url() from not handling external links when there is no good reason for it to do so. Your el() has a very specific use in the weblinks api, and should probably be included with it. ------------------------------------------------------------------------ Thu, 26 May 2005 16:18:31 +0000 : Bèr Kessels Yes, that el() But let me explain why i am anxious for my solution: el(), by default will not do much, in fact it will just be an URL()-clone but one that allows outgoing links. But it has a hook-caller. Again: by default it does nothing much, for there will not be any hooks around in core. It will allow, for example, weblinks, to track clicks, or to look up a permission and allow or disallow users to follow a link. Now, the whole issue, IMO is, that we should *not* provide two ways for handling external URLS, but rather a single one, that is weblink-safe. If we would introduce URL() with outgoing links-capabilities, we can no longer track any outgoing URLs trough them. We will then have two ways for handling outgoing URLs, resulting in inconsistencies, and confused users. (hey, my anonymous users can follow link foo, while i have "follow outgig links" permission disabled for them). (BTW weblink will soon be renamed into links, just for reference) ------------------------------------------------------------------------ Thu, 30 Jun 2005 11:06:11 +0000 : adrian Attachment: http://drupal.org/files/issues/url_external_links_0.patch (824 bytes) I've updated the patch. ------------------------------------------------------------------------ Mon, 18 Jul 2005 14:12:36 +0000 : adrian spoke to dries about this patch, and apart from the incomplete patch I uploaded, there are about a dozen instances where in-line html is used were l() would suffice. I will be cleaning them up and submitting a new patch soon. ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:44:03 +0000 : moshe weitzman hopefully adrian or someone else will chase down the remaining urls which should use this function. ------------------------------------------------------------------------ Fri, 05 Aug 2005 19:25:08 +0000 : jsloan I've come to this party late but would like to see a patch soon... I've used the patch from Adrian (#15) but this did not work correctly in my setup (IIS 5.0 / no clean urls) I applied it against 4.6.2 and cvs. Where is the latest patch that I can test ... I'll throw some time at this because I need it for a module used within our Intranet. ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:06:03 +0000 : jsloan Attachment: http://drupal.org/files/issues/external_urls_2.patch (3.67 KB) I'm not sure what the final plans are for this in the CVS so I have developed a patch for use on 4.6.2 I decided to add a parameter named $external that is passed to url() function. The default is FALSE so this should not break any existing calls to either l() or url(). I have also refactored url() so that it is much easier to understand and extend --- I'm thinking of Bèr Kessels intention to "allow, for example, weblinks, to track clicks, or to look up a permission and allow or disallow users to follow a link" " Since the parameter is passed to activate the external url there is no need for regex or strpos, this should be handled by the calling function and then set the $external parameter to TRUE For example I have patched the theme_menu_item_link() function in menu.inc to accept external urls: <?php function theme_menu_item_link($item, $link_item) { // this next line was added so that a menu item could point to the default home $link_item['path'] = preg_replace('/^\/$/','',$link_item['path']); // this next line was added so that a menu item could point to an external url $external = (preg_match('<^[a-z]+://>', $link_item['path'])) ? TRUE : FALSE ; return l($item['title'], $link_item['path'], array_key_exists('description', $item) ? array('title' => $items['description']) : array(),NULL,NULL,FALSE,FALSE,$external); } ?> I hope that this patch will spur some solutions - I'm using this now in production but have not tested extensively across every scenario. thanks jim sloan ------------------------------------------------------------------------ Fri, 19 Aug 2005 12:10:50 +0000 : jsloan ... are there any thoughts on this? What is the status of the final patch? ------------------------------------------------------------------------ Sun, 28 Aug 2005 15:48:08 +0000 : adrian The problem with this patch is that we still need to fix all the inline occurrences of the <a href tag. I don't think we should confuse the issue and implement a hook at this point, as url() breaking with external links is a bug, pure and simple. ------------------------------------------------------------------------ Thu, 08 Sep 2005 15:24:16 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/theme.inc.patch.txt (1.16 KB) Adrian, your second patch for theme.inc looks wrong to me. Don't you still need the line $settings[$type .'_links'][] = l($text, $link, $attributes); ? I also got rid of unset($attributes); Where was that supposed to come from that it needed unsetting? ------------------------------------------------------------------------ Thu, 08 Sep 2005 15:48:02 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/theme.common.inc.patch.txt (1.83 KB) Here is a unified patch that I updated to fix a bug in Adrian's common.inc code (everything was being handled as absolute), plus the update to theme.inc that I questioned in the previous post. In my initial testing, this works perfectly. I can put absolute URLs in for the paths in menu items and it handles them fine, and changes no other behavior. I searched core for all uses of url() and http:// and found no more violating code such as that in theme.inc. Net change in core with this patch: 0 lines of code. Please review quickly, I'm writing in the upcoming book about this functionality as if it is in. =) ------------------------------------------------------------------------ Thu, 08 Sep 2005 15:49:43 +0000 : robertDouglass Correction: net change to core -1 line of code. ------------------------------------------------------------------------ Thu, 08 Sep 2005 18:58:01 +0000 : adrian What needs to be removed from core are not uses of url() and l(), but all occurences of the anchor tag. The anchor tag was mostly used because l() didn't do external links. Also, in the case of phptemplate , that is the line that creates the array that is passed to the template. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:58:13 +0000 : robertDouglass Good - that needs to be removed too, but is there anything that doesn't work right if we don't remove them? Lots of the $output = '<a href....' instances in the code use url() for href and these are not broken. They can't be seen as a reason not to commit this particular patch. If anyone point out things that break with the patch I made, I'll fix them. I can't go through and clean up anchor tags on idealistic grounds at the moment (as much as I would otherwise like to). ------------------------------------------------------------------------ Sat, 10 Sep 2005 20:11:08 +0000 : jsloan I think that using strpos($path, '://') to trigger the external url is a bad idea. My patch implements a new parameter to url() and l() that will let the calling function decided if it is an external url. And it is backward compatible with all existing calls. I also rewrote the logic of builing the url to make it simple. Adrian, I do not agree that it was a bug; url() never intended to handle external url´s and also, the clean up of the inline anchor tags are not dependent on this feature. Can this be considered for the solution to this feature? ------------------------------------------------------------------------ Sun, 11 Sep 2005 02:14:10 +0000 : jsloan ... the patch I am referring to is above at # 23 ------------------------------------------------------------------------ Sun, 11 Sep 2005 07:27:14 +0000 : robertDouglass "I think that using strpos($path, '://') to trigger the external url is a bad idea. " Please explain why. The only thing that your regexp does that is different is check to see if the part of the string before the :// is within the range a-z, and for that you use a more expensive PHP function. Please indicate the cases where strpos($path, '://') won't work correctly. "My patch implements a new parameter to url() and l() that will let the calling function decided if it is an external url. And it is backward compatible with all existing calls. " This means that instead of doing one check in a centralized place, programmers now have to write checks anytime they want to call this function with an either/or case. I don't see why this is better. "I also rewrote the logic of builing the url to make it simple. " This is good. "Adrian, I do not agree that it was a bug; url() never intended to handle external url´s and also, the clean up of the inline anchor tags are not dependent on this feature. " I'm still unclear on why anything else in the code base needs to be changed before this functionality can be accepted. I stick with my patch as the preferred solution. ------------------------------------------------------------------------ Sun, 11 Sep 2005 14:58:55 +0000 : jsloan "Please explain why " My post #23 has confused you. The patch itself does not add a strpos() or regexp() ... the code snippet I posted is an example of the calling function´s responsibility of determining the use of the external link and then passing that parameter on to the l() or url() function. I believe it is a bad idea to use string parsing at this low level to determine functionality because it will impose this behavior on all calls. There could be a case in which this would be a problem; such as a url that could contain another url as a parameter. index.php?q=module&param=http://example.com ... and there are some who would balk at the processing overhead of pattern matching at his level. "This means that instead of doing one check in a centralized place, programmers now have to write checks anytime they want to call this function with an either/or case. I don't see why this is better. " This is better because the module author is the proper person to determine whether the link is external. Note that in the case of all internal links there is no change of coding practice at all. It is when the module function must handle external links that module´s author can include the various context and string pattern tests to set the $external parameter for the l() or url() call. This is what I was demonstrating in the code example of post #23. "I'm still unclear on why anything else in the code base needs to be changed before this functionality can be accepted. I stick with my patch as the preferred solution. " I think that we agree on this point. Nothing needs to be changed in the code base – my patch is backward compatible to all existing calls and the clean up work on the in-line anchor tags can (and should) take place regardless of this patch.

1 0

[drupal-devel] [bug] Handle full URLs in url() and l()
by jsloan 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/10888 Post a follow up: http://drupal.org/project/comments/add/10888 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: Bèr Kessels Reported by: jhriggs Updated by: jsloan Status: patch (code needs work) "Please explain why " My post #23 has confused you. The patch itself does not add a strpos() or regexp() ... the code snippet I posted is an example of the calling function´s responsibility of determining the use of the external link and then passing that parameter on to the l() or url() function. I believe it is a bad idea to use string parsing at this low level to determine functionality because it will impose this behavior on all calls. There could be a case in which this would be a problem; such as a url that could contain another url as a parameter. index.php?q=module&param=http://example.com ... and there are some who would balk at the processing overhead of pattern matching at his level. "This means that instead of doing one check in a centralized place, programmers now have to write checks anytime they want to call this function with an either/or case. I don't see why this is better. " This is better because the module author is the proper person to determine whether the link is external. Note that in the case of all internal links there is no change of coding practice at all. It is when the module function must handle external links that module´s author can include the various context and string pattern tests to set the $external parameter for the l() or url() call. This is what I was demonstrating in the code example of post #23. "I'm still unclear on why anything else in the code base needs to be changed before this functionality can be accepted. I stick with my patch as the preferred solution. " I think that we agree on this point. Nothing needs to be changed in the code base – my patch is backward compatible to all existing calls and the clean up work on the in-line anchor tags can (and should) take place regardless of this patch. jsloan Previous comments: ------------------------------------------------------------------------ Thu, 16 Sep 2004 15:01:24 +0000 : 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 ?> ------------------------------------------------------------------------ Thu, 30 Sep 2004 11:09:30 +0000 : moshe weitzman +1. less HTML in code is a good thing for readability, if nothing else. ------------------------------------------------------------------------ Mon, 24 Jan 2005 20:06:26 +0000 : 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. ------------------------------------------------------------------------ Sun, 13 Mar 2005 22:36:34 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/external_urls_0.patch (909 bytes) Updated from Drupal root directory. ------------------------------------------------------------------------ Mon, 14 Mar 2005 00:05:17 +0000 : gordon +1 This is a good thing for people who can't using clean urls. ------------------------------------------------------------------------ Sat, 02 Apr 2005 06:23:10 +0000 : 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? ------------------------------------------------------------------------ Sat, 02 Apr 2005 07:25:13 +0000 : 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 ------------------------------------------------------------------------ Thu, 12 May 2005 14:57:07 +0000 : 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. ------------------------------------------------------------------------ Thu, 12 May 2005 14:59:57 +0000 : killes(a)www.drop.org I had benchmarked /node as anon user without cache. ------------------------------------------------------------------------ Wed, 25 May 2005 04:50:08 +0000 : 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. ------------------------------------------------------------------------ Wed, 25 May 2005 05:05:55 +0000 : gordon 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. ------------------------------------------------------------------------ Wed, 25 May 2005 06:00:01 +0000 : Steven gordon: I still think this is a bit excessive. Perhaps instead of valid_url() we could use a simpler regexp !^[a-z]://!i or even strpos('://'). ------------------------------------------------------------------------ Wed, 25 May 2005 06:15:36 +0000 : gordon I am not says that it is or is not excessive. I was just saying that this is where it is going to be used, so creating url() and url_external() is not really an option ------------------------------------------------------------------------ Wed, 25 May 2005 06:57:53 +0000 : Bèr Kessels we /do/ need an external_url() thing. The links bundle (handling weblinks and the likes) willprovide this /and/ need this. People will get all sorts of tools to do /stuff/ with external links, like add permissions, track clicks etc. depending on the modules they have and the settings. All outgoing weblinks, should somehow respect those. Patches for core are coming, though. On top of that, a external_l() finction is simple. And if people want them in primary links they can do the regexping /before/ the function, not /in/ the function. The calling code can decide whether to call l() or external_l(), instead of putting logic in l() that is unneded 95% of the times. But i hereby promise to introduce such a function tbefore the end of this week :) ------------------------------------------------------------------------ Wed, 25 May 2005 07:17:47 +0000 : gordon The only thing I do not see in this plan is how with this work with the menu module and the navigation menu. ------------------------------------------------------------------------ Wed, 25 May 2005 10:04:12 +0000 : adrian Attachment: http://drupal.org/files/issues/url_external_links.patch (1.75 KB) What happened to the patch i uploaded ? It doesn't use a complex regexp, but uses strpos($url, '://') < 5. ------------------------------------------------------------------------ Thu, 26 May 2005 06:19:32 +0000 : Bèr Kessels Allthough this works, is clean AND does what it should do, I still am in favour of a better nitegrated solution. So please do not yet commit this, untill I upload my patch. We can then discuss what solution to take. It will not differ a big lot though. ------------------------------------------------------------------------ Thu, 26 May 2005 08:39:32 +0000 : adrian is this the patch where you want to introduce an el() to allow for counting of external links ? That's a different patch altogether. This is primarily about stopping url() from not handling external links when there is no good reason for it to do so. Your el() has a very specific use in the weblinks api, and should probably be included with it. ------------------------------------------------------------------------ Thu, 26 May 2005 16:18:31 +0000 : Bèr Kessels Yes, that el() But let me explain why i am anxious for my solution: el(), by default will not do much, in fact it will just be an URL()-clone but one that allows outgoing links. But it has a hook-caller. Again: by default it does nothing much, for there will not be any hooks around in core. It will allow, for example, weblinks, to track clicks, or to look up a permission and allow or disallow users to follow a link. Now, the whole issue, IMO is, that we should *not* provide two ways for handling external URLS, but rather a single one, that is weblink-safe. If we would introduce URL() with outgoing links-capabilities, we can no longer track any outgoing URLs trough them. We will then have two ways for handling outgoing URLs, resulting in inconsistencies, and confused users. (hey, my anonymous users can follow link foo, while i have "follow outgig links" permission disabled for them). (BTW weblink will soon be renamed into links, just for reference) ------------------------------------------------------------------------ Thu, 30 Jun 2005 11:06:11 +0000 : adrian Attachment: http://drupal.org/files/issues/url_external_links_0.patch (824 bytes) I've updated the patch. ------------------------------------------------------------------------ Mon, 18 Jul 2005 14:12:36 +0000 : adrian spoke to dries about this patch, and apart from the incomplete patch I uploaded, there are about a dozen instances where in-line html is used were l() would suffice. I will be cleaning them up and submitting a new patch soon. ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:44:03 +0000 : moshe weitzman hopefully adrian or someone else will chase down the remaining urls which should use this function. ------------------------------------------------------------------------ Fri, 05 Aug 2005 19:25:08 +0000 : jsloan I've come to this party late but would like to see a patch soon... I've used the patch from Adrian (#15) but this did not work correctly in my setup (IIS 5.0 / no clean urls) I applied it against 4.6.2 and cvs. Where is the latest patch that I can test ... I'll throw some time at this because I need it for a module used within our Intranet. ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:06:03 +0000 : jsloan Attachment: http://drupal.org/files/issues/external_urls_2.patch (3.67 KB) I'm not sure what the final plans are for this in the CVS so I have developed a patch for use on 4.6.2 I decided to add a parameter named $external that is passed to url() function. The default is FALSE so this should not break any existing calls to either l() or url(). I have also refactored url() so that it is much easier to understand and extend --- I'm thinking of Bèr Kessels intention to "allow, for example, weblinks, to track clicks, or to look up a permission and allow or disallow users to follow a link" " Since the parameter is passed to activate the external url there is no need for regex or strpos, this should be handled by the calling function and then set the $external parameter to TRUE For example I have patched the theme_menu_item_link() function in menu.inc to accept external urls: <?php function theme_menu_item_link($item, $link_item) { // this next line was added so that a menu item could point to the default home $link_item['path'] = preg_replace('/^\/$/','',$link_item['path']); // this next line was added so that a menu item could point to an external url $external = (preg_match('<^[a-z]+://>', $link_item['path'])) ? TRUE : FALSE ; return l($item['title'], $link_item['path'], array_key_exists('description', $item) ? array('title' => $items['description']) : array(),NULL,NULL,FALSE,FALSE,$external); } ?> I hope that this patch will spur some solutions - I'm using this now in production but have not tested extensively across every scenario. thanks jim sloan ------------------------------------------------------------------------ Fri, 19 Aug 2005 12:10:50 +0000 : jsloan ... are there any thoughts on this? What is the status of the final patch? ------------------------------------------------------------------------ Sun, 28 Aug 2005 15:48:08 +0000 : adrian The problem with this patch is that we still need to fix all the inline occurrences of the <a href tag. I don't think we should confuse the issue and implement a hook at this point, as url() breaking with external links is a bug, pure and simple. ------------------------------------------------------------------------ Thu, 08 Sep 2005 15:24:16 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/theme.inc.patch.txt (1.16 KB) Adrian, your second patch for theme.inc looks wrong to me. Don't you still need the line $settings[$type .'_links'][] = l($text, $link, $attributes); ? I also got rid of unset($attributes); Where was that supposed to come from that it needed unsetting? ------------------------------------------------------------------------ Thu, 08 Sep 2005 15:48:02 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/theme.common.inc.patch.txt (1.83 KB) Here is a unified patch that I updated to fix a bug in Adrian's common.inc code (everything was being handled as absolute), plus the update to theme.inc that I questioned in the previous post. In my initial testing, this works perfectly. I can put absolute URLs in for the paths in menu items and it handles them fine, and changes no other behavior. I searched core for all uses of url() and http:// and found no more violating code such as that in theme.inc. Net change in core with this patch: 0 lines of code. Please review quickly, I'm writing in the upcoming book about this functionality as if it is in. =) ------------------------------------------------------------------------ Thu, 08 Sep 2005 15:49:43 +0000 : robertDouglass Correction: net change to core -1 line of code. ------------------------------------------------------------------------ Thu, 08 Sep 2005 18:58:01 +0000 : adrian What needs to be removed from core are not uses of url() and l(), but all occurences of the anchor tag. The anchor tag was mostly used because l() didn't do external links. Also, in the case of phptemplate , that is the line that creates the array that is passed to the template. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:58:13 +0000 : robertDouglass Good - that needs to be removed too, but is there anything that doesn't work right if we don't remove them? Lots of the $output = '<a href....' instances in the code use url() for href and these are not broken. They can't be seen as a reason not to commit this particular patch. If anyone point out things that break with the patch I made, I'll fix them. I can't go through and clean up anchor tags on idealistic grounds at the moment (as much as I would otherwise like to). ------------------------------------------------------------------------ Sat, 10 Sep 2005 20:11:08 +0000 : jsloan I think that using strpos($path, '://') to trigger the external url is a bad idea. My patch implements a new parameter to url() and l() that will let the calling function decided if it is an external url. And it is backward compatible with all existing calls. I also rewrote the logic of builing the url to make it simple. Adrian, I do not agree that it was a bug; url() never intended to handle external url´s and also, the clean up of the inline anchor tags are not dependent on this feature. Can this be considered for the solution to this feature? ------------------------------------------------------------------------ Sun, 11 Sep 2005 02:14:10 +0000 : jsloan ... the patch I am referring to is above at # 23 ------------------------------------------------------------------------ Sun, 11 Sep 2005 07:27:14 +0000 : robertDouglass "I think that using strpos($path, '://') to trigger the external url is a bad idea. " Please explain why. The only thing that your regexp does that is different is check to see if the part of the string before the :// is within the range a-z, and for that you use a more expensive PHP function. Please indicate the cases where strpos($path, '://') won't work correctly. "My patch implements a new parameter to url() and l() that will let the calling function decided if it is an external url. And it is backward compatible with all existing calls. " This means that instead of doing one check in a centralized place, programmers now have to write checks anytime they want to call this function with an either/or case. I don't see why this is better. "I also rewrote the logic of builing the url to make it simple. " This is good. "Adrian, I do not agree that it was a bug; url() never intended to handle external url´s and also, the clean up of the inline anchor tags are not dependent on this feature. " I'm still unclear on why anything else in the code base needs to be changed before this functionality can be accepted. I stick with my patch as the preferred solution.

1 0

[drupal-devel] [feature] Authentication hook for local users
by nedjo 11 Sep '05

11 Sep '05

Issue status update for http://drupal.org/node/29147 Post a follow up: http://drupal.org/project/comments/add/29147 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: naudefj Updated by: nedjo Status: patch (code needs work) "The fix allows local users (i.e. just "user") to be authenticated against an external source. " Why? I assume authentication should be either local or external--not both. Therefore, we need a way to distinguish between the two. Currently, the username@domain convention is used. Removing the @domain part of the DrupalID would remove this disctinction. Is the issue just that this convention is confusing for new users? In this case, I'd suggest pulling the domain part out into a different field. So, when drupal.module is enabled, there is a third user login field, "site" or "domain", that would include "local" and any registered associates. nedjo Previous comments: ------------------------------------------------------------------------ Thu, 18 Aug 2005 20:46:57 +0000 : naudefj Please implement a hook to authenticate local users - similar to what hook_auth does for remote users. Alternatively, extend hook_auth to authenticate both local and remote users. Current behaviour: - If a user logs in with "user@domain", hook_auth gets activated; - If a user logs in with "user", hook_auth is bypassed. More details: http://drupal.org/node/27959 ------------------------------------------------------------------------ Sat, 20 Aug 2005 20:11:29 +0000 : killes(a)www.drop.org use hook_user(login) ------------------------------------------------------------------------ Sat, 20 Aug 2005 20:38:53 +0000 : naudefj I just tried hook_user(login), but it only fires when a valid drupal userid/password is entered. ------------------------------------------------------------------------ Sat, 20 Aug 2005 21:03:38 +0000 : naudefj I found the reason why hook_auth only works for remote users. Look at this code from users.module: // Try each external authentication source in series. Register user if // successful. else if (!$user->uid && $server) { Is this something that should be fixed, or should I just comment out the the "&& $server" part myself? ------------------------------------------------------------------------ Sun, 28 Aug 2005 19:03:24 +0000 : naudefj Attachment: http://drupal.org/files/issues/user_31.patch (1.7 KB) Here is a patch that will allow HOOK_AUTH to work for both local and remote users. If would be great if it can be reviewed and applied to HEAD. ------------------------------------------------------------------------ Sun, 11 Sep 2005 05:32:55 +0000 : moshe weitzman hook_auth deliberately is ineffective for local logins. if you enable it for all logins, you will slow down the experience for many people who guess their passwords wrong on login page. in that case, code would call all remote auth modules an dthey in turn would contact their home base. thats too slow. we want to return a 'sorry try again' message very quickly. what are you trying to accomplish here? ------------------------------------------------------------------------ Sun, 11 Sep 2005 06:59:52 +0000 : naudefj Hi, > hook_auth deliberately is ineffective for local logins. Not true. HOOK_AUTH cannot be used for local logins. Look at this line in user.module: else if (!$user->uid && $server) { > what are you trying to accomplish here? I would like my users to use their existing forum userids and passwords to login to Drupal. However, I quickly discovered that external authentication only works if you append a domain/servername to your userid. For example, if a username like "user*@domain*" is entered, HOOK_AUTH gets activated. Problem is that if a userid like "user" is used, HOOK_AUTH will be bypassed. This fix allows local users (i.e. just "user") to be authenticated against an external source. Any other solutions/ suggestions would be greatly appreciated! Best regards. Frank

1 0