September 2005 - Development - lists.drupal.org

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

12 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) Attachment: http://drupal.org/files/issues/external_urls_463.patch (4.16 KB) Here is the patch for 4.6.3; It contains a rewrite of the url() function and the $external parameter is added to the l() function. I have tested on Apache2/Linux and IIS5/WIndws 2000 and with clean url's on Apache2/Linux. A little later I will submit a patch for menu.inc to enable external url's. 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. ------------------------------------------------------------------------ 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 ------------------------------------------------------------------------ Sun, 11 Sep 2005 15:07:36 +0000 : robertDouglass You might update the bit in theme.inc as well and include that in the patch. ------------------------------------------------------------------------ Mon, 12 Sep 2005 19:33:15 +0000 : jsloan Attachment: http://drupal.org/files/issues/external_urls_cvs.patch (4.24 KB) Here is the patch for CVS; It contains a rewrite of the url() function and the $external parameter is added to the l() function. I have tested on Apache2/Linux and IIS5/WIndws 2000 and with clean url's on Apache2/Linux. The patch fails on version 4.6.3 so that will be posted seperate. A little later I will submit a patch for menu.inc to enable external url's.

1 0

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

12 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) Attachment: http://drupal.org/files/issues/external_urls_cvs.patch (4.24 KB) Here is the patch for CVS; It contains a rewrite of the url() function and the $external parameter is added to the l() function. I have tested on Apache2/Linux and IIS5/WIndws 2000 and with clean url's on Apache2/Linux. The patch fails on version 4.6.3 so that will be posted seperate. A little later I will submit a patch for menu.inc to enable external url's. 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. ------------------------------------------------------------------------ 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 ------------------------------------------------------------------------ Sun, 11 Sep 2005 15:07:36 +0000 : robertDouglass You might update the bit in theme.inc as well and include that in the patch.

1 0

[drupal-devel] [feature] Replace core archive.module w/ codemonkeyx archive.module
by m3avrck 12 Sep '05

12 Sep '05

Issue status update for http://drupal.org/node/29676 Post a follow up: http://drupal.org/project/comments/add/29676 Project: Drupal Version: cvs Component: archive.module Category: feature requests Priority: normal Assigned to: Morbus Iff Reported by: Morbus Iff Updated by: m3avrck Status: patch (ready to be committed) Bump to the top, freeze coming soon!!! m3avrck Previous comments: ------------------------------------------------------------------------ Thu, 25 Aug 2005 21:08:49 +0000 : Morbus Iff Over at http://drupal.org/node/8287, Berkes mentions that the core archive.module was considered being removed, per a discussion at the Drupal Sprint. Kjartan also mentions he would "love to have the archive module improved in general." In chatting with chx about this, he mentioned codemonkeyx's rewrite sitting in contrib/modules/archive/. I'll be doing some work with the archive.module over the next few days, and will be basing my changes around codemonkeyx's version, and making it compatible with HEAD. This general Issue is to move codemonkeyx's version into HEAD as a replacement to the existing archive.module. An unknown version of his replacement can be seen at http://www.codemonkeyx.net/archive. I'll be running a live HEAD version soon as well. These patches were made during the customization of Drupal by http://www.NHPR.org. In loving support of open source software, http://www.NHPR.org will continue to contribute patches they feel the community will benefit from. Questions about this patch should be directed to morbus(a)disobey.com. ------------------------------------------------------------------------ Fri, 26 Aug 2005 19:45:59 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues_2 (9.56 KB) As an example of a very early revision, see the attached, with the following changes from the current contrib CVS: * removed the year offset from theme_archive_navigation_years, which controlled how many year links to show at once in the top nav. For those with sites with more than five years, they'll WANT people to notice that they have five years, not to have to click on the earliest date and then have their expectations changed. * made the "created > $date" in archive_buildQuery "created >= $date" instead, to allow posts that were created at exactly midnight that day (like me, by design). * since there's no block, I made the menu item visible upon first load. this menu item is given "access content" permissions. More to come, including doxygen and gmt considerations. ------------------------------------------------------------------------ Fri, 26 Aug 2005 19:47:41 +0000 : Morbus Iff Might as well start getting a review of it so I can fix 'em as they come in. ------------------------------------------------------------------------ Fri, 26 Aug 2005 19:56:49 +0000 : Tobias Maier cant you provide a patch file? thanks for your work ------------------------------------------------------------------------ Fri, 26 Aug 2005 20:04:01 +0000 : Morbus Iff The codemonkeyx version is a complete rewrite of the core archive.module. If I were to create a patch file against core, every line would be deleted, and every line would be new. Once I finish my revisions to codemonkey's version, I'll post the final version here, as well as a patch against his current CVS. ------------------------------------------------------------------------ Fri, 26 Aug 2005 20:09:13 +0000 : Tobias Maier ok, thanks again :D ------------------------------------------------------------------------ Mon, 29 Aug 2005 13:41:43 +0000 : Junyor +1 for this change. The archive.module in core is dead. ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:14:30 +0000 : adrian What is the progress on this morbus ? ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:29:13 +0000 : Morbus Iff Adrian - I'll be attaching a new version either later today or tomorrow, with a CHANGELOG. I'll also be running a live version of it over on NHPR.org for people to play with. The three major things I'm worried about right now are a) doxygen, b) variable/function naming, c) GMT considerations. After those, I'll be exploring a patch for my own needs: the ability to get archives for particular term. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:53:34 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive.module (9.93 KB) Here's the latest, with the following changelog: * reordered some routines to be a little more workflowish. * renamed archive_buildQuery to archive_build_query. * general whitespace and formatting cleanup. * HEADish update: returning $output, not page templating it. * removed the reference of &$ad in archive_build_query. * test for the existence of arg(#)'s before validating them. * archive_validSomething changed to archive_valid_something. * removed unused vars: cur_date, cur_date_end. * renamed archive_buildURL to archive_build_url. * removed the HTML whitespace from the theming. * twiddled a lot of quotes and apostraphes. * removed 'future' CSS class. ill-defined. * reordered/renamed the CSS classes. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:54:08 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_29676_archive_css.patch (1.56 KB) And the drupal.css patch. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:56:29 +0000 : Morbus Iff This version of the module is currently running live at http://www.nhpr.org/archive/. ------------------------------------------------------------------------ Mon, 29 Aug 2005 19:59:57 +0000 : Tobias Maier if i click for example on 2003 it would be good if this would go to january or december and marks them that this one will be shown now as you can see it if you click on january 2003. it has to select * on the first: the first month of writing * on the last: the last month of writing * on every else: january I hope you can understand what I mean... greets tobias ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:17:40 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_0.module (11.28 KB) Alright. I've attached another new version that adds a new feature that wasn't part of the original codemonkeyx CVS, but was chatted about on the devel list back in April. If this particular feature has bad code or needs heavy refactoring, certainly consider ONLY the version in comment #9 (and the matching drupal.css patch in #10). This new version supports dated archives based on taxonomy tids. It was a quick addition which NHPR.org needed (for the date nav; the normal tid archive pager wasn't strong enough for our needs). Since it was a quick addition, it supports only ONE tid at a time - the 'and/or' syntax for the taxonomy.module was not brought over. If that syntax was desired, it'd make more sense to create some sort of API for archive.module so that other nodes can take advantage of the dated nav in their normal pages (like node types, users, forums, etc.) The added code supports term matches at any granularity: # all node types that match tid 15000 ('The Front Porch') http://www.nhpr.org/archive/term/15000 # all 2005 node types that match tid 20 ('Health') http://www.nhpr.org/archive/2005/term/20 # all March, 2003 node types that match tid 9 ('Education') http://www.nhpr.org/archive/2003/3/term/9 # all July 11, 2003 node types that match tid 49 ('Economy') http://morbus.totalnetnh.net/nhpr/archive/2002/7/11/term/49 ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:27:26 +0000 : Tobias Maier what does this mean? "Story Archives of 'archives' " on http://www.nhpr.org/archive/term/15000 should this maybe named? "Archive of 'The Front Porch'" if I go to http://www.nhpr.org/archive/term/20 I can only read "archives"... why is there a difference? - I never tested this module on my test site, because I'm not at home - ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:29:34 +0000 : Morbus Iff Tobias: that wouldn't be possible, at least not accurately. The new archive.module supports browsing by year, month, and day, as you know. archive/2005 loads up all the data from a particular year and starts creating a pager out of it. Consider if you have 3 posts in December, and 15 posts in November. It wouldn't be "right" to highlight December because the pager display for the entire year would also include some of November's entries (since 3 is less than the pager increment). Likewise, if we ONLY showed the items from December, then we wouldn't have a "pager by year" feature, only a "pager by month (defaulting to December when none is selected)" feature. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:30:41 +0000 : Morbus Iff Tobias: regarding #14, that's an artifact of the templates that I'm using for NHPR, and has nothing to do with archive.module itself (in fact, once the anonymous cache expires, you'll see that little oopsie go away). ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:31:47 +0000 : Tobias Maier I can see your right :) I hope it comes in HEAD before tomorrow :D ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:20:12 +0000 : dtan I apologize if this is already a known issue. http://www.nhpr.org/archive/2005/9 does not create a link for september 1st, even though there are 2 nodes listed (http://www.nhpr.org/archive/2005/9/1) ------------------------------------------------------------------------ Tue, 30 Aug 2005 17:11:21 +0000 : Morbus Iff dtan: I'm pretty sure I know what this is - I'll address it either later today or tomorrow. ------------------------------------------------------------------------ Tue, 30 Aug 2005 20:49:58 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_1.module (11.53 KB) Alright, I've attached a new archive.module - this is the version WITH term filtering enabled. I can make one without terms if necessary - otherwise, I'll just work from this one for now. This version fixes the bug that dtan saw, as a well as a bunch of other off-by-one errors. Of primary importance, however, is that all mktime's that mattered have been switched to gmmktime, which was one of the oft-reported Issues with the old archive.module. I want to eyeball them all again and make sure they're right though. The URLs from #13 are still operational and the CSS from #10 is still required. ------------------------------------------------------------------------ Wed, 31 Aug 2005 00:02:06 +0000 : Morbus Iff In testing with a few people in #drupal, we've discovered a much bigger problem, which affects this rewritten module as well as the current core archive.module. In a nutshell, the node.created time is stored with time(). PHP's time() bases itself on the server time, NOT on GMT. Thus, for archive.module to work correctly, it must ALWAYS use mktime (relative to server time) and never consider the $user->timezone (relative to GMT). For archive.module, this would cause dates to always be considered via server time, which isn't good, but is better than the craziness going on now. Alternatively, we could try to convert server time to GMT first, and then work with that. The proper solution is to fix node.module to use gmmktime without any arguments for node.created, then have an update path that modifies all node .created and .modified values to GMT, not server time. ------------------------------------------------------------------------ Wed, 31 Aug 2005 05:53:51 +0000 : Junyor Since that is also a problem with the old archive.module, I don't see why it should stop this from getting into core. ------------------------------------------------------------------------ Wed, 31 Aug 2005 06:37:28 +0000 : stefan nagtegaal "Since that is also a problem with the old archive.module, I don't see why it should stop this from getting into core. " Well, I think it's better to only accept the best code rather than accepting bugs getting in core. ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:05:51 +0000 : Kobus I am with Junyor on this. If this can be fixed, great, but if not, it's not a train smash, as the old one exhibits the same problem. I say add the new archive module, if there are no other ciritical bugs with it. It is much more robust and usable than the old one. We desperately need a new archive module. I couldn't find any other bugs while testing with the links Morbus posted. I don't have a HEAD installation anywhere, so can't test it locally at this moment. Regards, Kobus ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:07:49 +0000 : Kobus BTW, code freeze means no new code added, right? Can't this module put in Core as is for the code freeze and the bug sorted out before the official release? Or is that just mean of me to suggest that? Kobus ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:13:09 +0000 : Junyor Stefan: The bug is already in core, since node.module is in core. Archive.module (old and new!) just shows that bug. ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:20:35 +0000 : Tobias Maier I want to have it in core for 4.7, too!!! ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:25:24 +0000 : stefan nagtegaal Junyor: I know that.. Though I think it's not good to accept code which we are aware of that it has bugs in it.. Offcourse this is very double, because drupal contains (probably) a lot of bugs, only they weren't spotted yet.. But, accepting code which has bugs in unacceptable imo.. For example, the node revisions patch had almost 40 reviews/rewritings from Gerhard and several others before it was accepted to be in core.. If we do not allow bugs to go into core, we don't have to bughunt and fix later which is a good thing.. So, imo we should first sort out the problem, then discuss what the best way is to fix the problem, and after that Squeeze that moron! ;-) ------------------------------------------------------------------------ Wed, 31 Aug 2005 09:11:06 +0000 : adrian I vote we include this module, and open up a new issue for the bug. It's not archive's fault that this occurs, it is just showing the data it has access to. The bug already exists in core too. ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:50:59 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_2.module (10.63 KB) Attached is a new, and most likely final, version of the archive.module. I've removed all user->timezone references - all date determinations are based on server time, which is also the time used in the "created" column of the node table. This is "more accurate" than what the core archive.module currently does (which'll always be wrong because it's treating server time as GMT, which isn't always the case). When node.module does start saving times as GMT properly, archive.module will have be to be tweaked with timezones and blah blah blah. I did fiddle around with determining the server offset in an attempt to get to a GMT base, but I didn't have much luck with that. The NHPR.org URLs above are still valid for testing. I need testers and reviewers. ------------------------------------------------------------------------ Wed, 31 Aug 2005 14:55:16 +0000 : m3avrck I get these two PHP errors when I have *no* content in my Drupal install (just did a clean install to test it): warning: mktime() [<a href='function.mktime'>function.mktime</a>]: Windows does not support negative values for this function in websites\drupal_cvs\drupal\modules\archive.module on line 274. warning: date() [<a href='function.date'>function.date</a>]: Windows does not support dates prior to midnight (00:00:00), January 1, 1970 in websites\drupal_cvs\drupal\modules\archive.module on line 274. Once I add content, these go away. Need some better checks to make sure if there is *no* content you don't get weird errors and what not :) ------------------------------------------------------------------------ Wed, 31 Aug 2005 14:55:45 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_29676_archive_css_0.patch (1.56 KB) Updated drupal.css patch for HEAD. ------------------------------------------------------------------------ Wed, 31 Aug 2005 15:26:01 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_3.module (10.75 KB) Attached is the complete module, fixed for m3avrck's #31. ------------------------------------------------------------------------ Wed, 31 Aug 2005 17:30:36 +0000 : m3avrck Reviewed patch and further test, running great over here! Definetly +1 for this one! ------------------------------------------------------------------------ Wed, 31 Aug 2005 20:03:36 +0000 : Morbus Iff Note: The NHPR folks preferred their archives to be sorted from earliest to latest (SQL ASC vs. DESC) for month/day listings, so no longer use the NHPR.org URLs above as representative of the module itself. ------------------------------------------------------------------------ Wed, 31 Aug 2005 21:26:37 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/drupal.css (10.35 KB) CVS updated for HEAD again. ------------------------------------------------------------------------ Wed, 31 Aug 2005 21:27:00 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_29676_archive_css_1.patch (1.56 KB) GRRR. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:18:48 +0000 : m3avrck Attachment: http://drupal.org/files/issues/archive_0.patch (20.72 KB) Rerolled patch against head. Also created one patch that fixes both archive.module and drupal.css so it's a clean and simple process now :) ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:23:55 +0000 : m3avrck Just retested with the latest patch I made, applies cleanly against HEAD and *everything* works great and looks great too. No errors of any kind, even works on PHP 5.0.5 with no call by reference errors, :D Let's get this baby in! ------------------------------------------------------------------------ Thu, 08 Sep 2005 21:26:06 +0000 : Souvent22 +1. I like the new patch, makes things easier to patch. Very slick. I like the break down of years, to months, to days. Very nice. Ready to go I say.

1 0

[drupal-devel] [bug] theme_image() doesn't actually output width and height of image
by m3avrck 12 Sep '05

12 Sep '05

Issue status update for http://drupal.org/node/30935 Post a follow up: http://drupal.org/project/comments/add/30935 Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck -Status: fixed +Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/system.module_10.patch (1.53 KB) Dries, great catch! What prompted this originally was that the screenshots on the theme page didn't have dimensions, didn't realize that index [3] returned this. Anyways, this patch fixes the screen shots and adds widths/heights. m3avrck Previous comments: ------------------------------------------------------------------------ Sun, 11 Sep 2005 16:22:28 +0000 : m3avrck Function theme_image() doesn't actually return a width and height for an image like it claims to do. ------------------------------------------------------------------------ Sun, 11 Sep 2005 19:30:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_11.patch (2.74 KB) Ok patched attached, which fixes this issue. Also, included a patch for system.module which sets the screen shots to 'TRUE' so image dimensions will also be outputted there as well (which they should be!). ------------------------------------------------------------------------ Sun, 11 Sep 2005 19:33:41 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_12.patch (2.75 KB) Fixed a tab issue. ------------------------------------------------------------------------ Sun, 11 Sep 2005 19:35:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_13.patch (2.75 KB) Fixed a spacing issue. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:18:32 +0000 : Souvent22 Used the patch, and did a quick test. Worked well for me. +1. ------------------------------------------------------------------------ Mon, 12 Sep 2005 16:21:16 +0000 : Robrecht Jacques I don't see why this patch is needed, "theme_image" returns a img tag with the width and height set if $getsize = TRUE. Eg: $node->body = theme('image', file_create_path('druplicon.png'), 'no alt', 'no title', array(), TRUE) . theme('image', file_create_path('druplicon.png'), 'no alt', 'no title', array(), FALSE) will return: <img src="files/druplicon.png" alt="no alt" title="no title" width="88" height="100" /> <img src="files/druplicon.png" alt="no alt" title="no title" /> (if druplicon.png is copied to the files/ directory). I don't see what you are fixing... You are right about the use of theme('image') in system.module though. The "false" should be "true". ------------------------------------------------------------------------ Mon, 12 Sep 2005 16:40:56 +0000 : m3avrck Well the actual code in theme_image() returned this: return '<img src="'. check_url($path) .'" alt="'. check_plain($alt) .'" title="'. check_plain($title) .'" '. $image_attributes . $attributes .'/>'; There is *no* mention of the $width and $height variables that are assigned above, not used at all. I checked images in the themes directory and none had this information, unless I was missing something obvious, I see no way that is being generated... nothing in the above img src about it. ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:41:25 +0000 : Dries Taken from http://php.net/getimagesize: "Index 3 is a text string with the correct height="yyy" width="xxx" string that can be used directly in an IMG tag.". $image_attributes contains this information. Marking this fixed. Please reopen if not.

1 0

[drupal-devel] MySQL HEAP-tables
by Dries Buytaert 12 Sep '05

12 Sep '05

Did someone experiment with MySQL's memory storage engine (HEAP tables)? If not, I'll add it to my TODO list. Sounds fun and from the looks, the cache table would be a good candidate ... See http:// dev.mysql.com/doc/mysql/en/memory-storage-engine.html for more information. -- Dries Buytaert :: http://www.buytaert.net/

5 5

[drupal-devel] [feature] Module enabling/initialization hook for initial setup
by nedjo 12 Sep '05

12 Sep '05

Issue status update for http://drupal.org/node/31005 Post a follow up: http://drupal.org/project/comments/add/31005 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_enable_hook.patch (1.05 KB) A module often has some one-time setup initialization to do, some of which can be easily automated (e.g., set a system variable), some of which may require site admin input. But there's no available method to detect and act on module enabling. So the attached patch implements one possible approach: a new _enable() hook, that would be called when a module is enabled. I suppose an alternate approach would be simply to redirect to the module's setting's page, if available; but, because multiple modules can be enabled at the same time, that might be impractical. I know, three days before code freeze is not the best time to be bringing up new proposed hooks! I'll be happy to wait until next cycle to work the idea through. nedjo

2 1

[drupal-devel] [feature] add inline functionaliy to uploads
by Bèr Kessels 12 Sep '05

12 Sep '05

Issue status update for http://drupal.org/node/26288 Post a follow up: http://drupal.org/project/comments/add/26288 Project: Drupal Version: cvs Component: upload.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Bèr Kessels Updated by: Bèr Kessels Status: patch (code needs review) Attachment: http://drupal.org/files/issues/upload_inline_2_4.patch (14.13 KB) No, please review /this/ patch. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:03:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline.patch (19.05 KB) One of the most often asked features is proper inlnie handling of files. Look at the amount of solutions, the popularity of image_assist, and the amount of peolpe dowloading image.module! That alone should be enough proof that Drupal lacks proper inline image support. This patch adds that to core. In fact, it does little more then appending a link of img tag to the body or the teaser. Off course that is configurable per file. Next to the [] list checkbox, this patch adds an [] inline checkbox. Simplicity is the foundation of this patch. I want no stles for inline editing, no fancy html wrappers, no tokens, just $node->body or teaser appended with a small html string. Another small themable funtion is introduced, (hey, you cannot expect me to develop something without adding more power for themers, now, can you? ;) ), that allows people to theme the string that is appended to the body or the teaser. Oh, and also note hat the biggest part of this patch is some cleaning I had to do in order to be able to develop properly. I dont like Ifs inside cases in foreaches inside swiches. in other words: nodeapi now calls functions instead of executing code directly. ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:25 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot.png (26.68 KB) here is how the form now looks ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:58 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot3.png (30.53 KB) and this is an example of inlined images and a .doc file. ------------------------------------------------------------------------ Sun, 03 Jul 2005 20:44:00 +0000 : sepeck changing to patch per request from berkes ------------------------------------------------------------------------ Tue, 05 Jul 2005 07:46:13 +0000 : Kobus This gets a +1 from me in principle, however, the [inline:xx] tag with inline.module gives you greater freedom as to where the inline image must be displayed. If you can add this functionality (I haven't checked the code, I don't know if it is in there) it would be a great addition for Core. This same strategy can be used for inline blocks, I am sure. Regards, Kobus ------------------------------------------------------------------------ Tue, 05 Jul 2005 08:57:50 +0000 : Bèr Kessels there are a couple of reasons why i did not include the [inline] tags. * I aimed for extreme simplicity: a checkbox shows an image inline: its up to the theme where it appears (if one does not like it before/above the body and teaser.). Simplicity was the main goal. * We don't have any tokens in core. And we should not have them. * Tokens are a very bad substitute for a good interface. They give less power then plain HTLM. Are much worst documented then HTML, but in the mean time, they are still as hard to learn as HTML. (Yes, I know people _think_ they are easier, but there _is_ not difference between [ ] and , only that its a different ascii char. So, no. I don't allow any placement of the image. I leave that that for dedicated modules, or the themer to decide. ------------------------------------------------------------------------ Tue, 05 Jul 2005 09:34:06 +0000 : Kobus So you will provide an API to do this? For example, with perhaps minor modifications, the inline.module will be able to display these files? In that case I am happy. If not, then I can't give my support to this patch (as if that matters...). A themer can't do this task as inline images (and blocks for that matter) is too dynamic for theming; it can be placed anywhere in a node where the user pleases. This means for me that there should be a module for this, such as inline module that allows you to define [inline:xx] tags. If your module emits an array of uploaded files (such as upload.module), inline.module can be, with minor changes, adapted to show these files inline. To show you what I mean with the content is too dynamic for a themer to perform this task, look at this screenshot related to inline blocks (inline images can follow the same pattern): http://drupal.org/files/issues/regions---possibility-3.png ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:26:15 +0000 : Bèr Kessels Kobus, We are dealing with different issues here: You want a method to place images, files or so anywhere in the post. That is fine, but certainly NOT addressed in the patch! I made patch that *only* adds marked files to the body. its really nothing more then <?php $node->body = $filestring . $node->body ?> No APIS, no, dynamic tokens, no filters, nothing. However, what I meant with themers, is that there now is a theme_upload_inline available, so you can theme the abovementioned $filestring. On top of that $node->files[FILEID]->inline is TRUE if a file is flagged for inline. So in node.tpl.php, or wherever you want to theme a node, you can print nice images inline, when that flag is set. And about the comment that a themer cant do this: Simply not true. On most sites images are always placed in the same places. REally, even the sites see which use img_assist or inline, use them to place the images on the exact same places in every node. People /think/ they want the power to place images anywhere, but they hardly ever use that power. Just look at all the big news/publishing sites out there (BBC, CNN, BoigBoing, r even freshmeat) images are all placed acc. to the theme. They are not placed in random places by authors. So if you are one of the few that still want that power, there aer mots of power tools like inline module or img_assist. We should offer a good default, one that is simple. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:41:34 +0000 : Gerhard Killesreiter I fully agree that tags are evil. Kobus: You can always look for tags in $node->body in your theme's node function. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:50:52 +0000 : stefan nagtegaal Ber, the theme-function in your code looks like: <?php +/** + * Theme function for rendering of inline images + * @param $file a file object. + * @param $image a Boolean, indicating whether an img tag (TRUE) or an anchor tag (FALSE) should be used. + * @ingroup themable + */ +function theme_upload_inline($file, $image) { + if ($image) { + return '<img src="'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'" alt="'. check_plain($file->filename) .'" class="upload inline">'; + } + else { + return ''. check_plain($file->filename) .' [1]'; + } + +} ?> After looking at your code it's not clear to me when $image is TRUE or FALSE, can you elaborate on me please? [1] http://drupal.org/'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'\" class=\"upload inline ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:06:39 +0000 : Bèr Kessels The function calling the theme function should decide whether its an image. IMO that is far too hardcore code for a themer ;) <?php $image = ereg('^(image/)', $file->filemime); ?> inside _upload_inline() does the trick. I did find one issue, though, with svgs, which are image/svg so maybe we should limit this to really only inline jpg, png, and gif, by extension? But I am no fan of determining files by extension, and IMO getimgsize is too heavy; ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:13:52 +0000 : stefan nagtegaal Can't you make use of PHP's <?php image_type_to_mime_type(); ?> or <?php image_type_to_extension(); ?> ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:45:43 +0000 : Kobus Well, if not in the patch, I need inline functionality one way or another. inline.module works perfectly with the current upload.module to fulfill my needs, and if there is a way where someone can extend your proposed upload.module, that would get my +1, otherwise I will just remain neutral about this issue. inline.module makes use of $node->files and displays that image. If your patch still uses $node->files, then this is a non-issue. In other words, if I can, with inline.module, or with minor modifications to inline.module still show files inline, I am +1 for this. As for the themability question... I still disagree. You say "Simply not true. On most sites images are always placed in the same places." That is ONLY true because they have NO alternatives. Means the content is in the same places, because they have no other places to put them. I have made tens of static sites where the content does not resemble a fixed pattern or structure. For me, free-flow layout is my expression of my creativity. This is why I still even BOTHER with static sites; when Drupal don't do what I want it to do. If Drupal could do inline display of content properly, I'd never even build another static site. It is a great mistake people make (including myself), and that is they design the site before ANY content is available, and this happens a lot in static sites. But in dynamic sites, you have far less control over this, unless you have a VERY clean structure with limited information that can be added, and only a few people posting content. If you can have your content before you start the design, you will have a better fit. I can't see how you can anticipate every possible posting posted on your site if you have a diversity of users. Especially not if you're using a site such as an designer's showcase site where your creativity is shown by your web sites. Possible, but not easy. If I claim that I have "unique, fresh designs" I can certainly NOT give them a "box-like" site with "left and right side bars only". That's why I need inline content, this includes images AND boxes. Gerhard: Your concern about tags is answered as well above, if I am not mistaken. The patch need not provide the tags, but should provide for someone to develop a module that provide the filters or tags. To summarize: I don't need your patch to do the inline images. I just need your patch to be able to allow someone else to develop a module that can display inline files. Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 09:56:38 +0000 : Bèr Kessels okay, so for all clarity, let me sumarise: * this patch introduces a chackbox "inline". If checked, the file will show up inline. IT will be appended to the teaser and the body. * this patch does *not* allow one to place images of files in a userdefined place in the body. * this patch does *not* do any resizing nor any thumbnailing. * This patch is meant to be simple, clear and transparant. No tokens, no javascript, no nothing. ------------------------------------------------------------------------ Mon, 18 Jul 2005 11:21:54 +0000 : Kobus Then I can't see how this patch could replace the existing upload.module, which allows you to use inline.module to place images inline. I therefore withdraw my +1 and go -1 on this (not that it matters, I believe). I think you didn't properly read my previous message. I said I don't need your patch to do all this, but I can't live without the functionality currently provided by the upload.module and inline.module combination. I agree that your patch don't need to have that functionality, but if your patch takes away this functionality, in other words, I can't (by hook or by crook) manage what I need to do, my vote is -1. Regards, Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 19:58:34 +0000 : Bèr Kessels Kobus, And all the others looking fofr advanced inline systems: please do not -1 this. It will hep inline module a lot too. allthough not yet in this patch, but can you imagine a perission system for inline module, that comes for free? Or a core system, that makes inline module ten times smaller? it is this path! so, please, if it is not /exactly/ what you are looking for, at least look at the advantages for Joe Average, and even for the possibilties for your favorite inline-module. even imag_assist will gain an anomous usability impcact when usiong this module, for it can then finally use the upload module in full scale. Ber ------------------------------------------------------------------------ Sat, 23 Jul 2005 18:59:10 +0000 : Bèr Kessels Sohodjo jim: " I hope it matters. A big -1 on enforcing over-simplification at the expense of important and existing functionality. If automatic, uncontrollable placement of images becomes the standard, it will mean fewer not more image-rich Drupal sites as site owners/developers get complaints about "Why can't I control my images!" I _strongly_ encourage Ber to provide a "have it both ways" solution. Why not have an admin configuration setting for "allow inline tags" with default off. Change it to on and you get an additional upload checkbox for 'inline at tag' to accommodate the current functionality of the inline module while simultaneously allowing for default placement. " NOOO. people; please; look at the PATCH. Again: and hopefully last time: Simplicity. This patch does NOT replace ANYTHING inline module or img_assist wants to do. This patch hands better data to such modules. It hands a variable over to these modules, $node->file[FID]->inline, which tells these sorts of modules if people wnat that file to appear inline. AND it CAN (by default will) render a file in the most simple way inline. So really, if you want to -1 this patch, fine. But do not -1 it, because it does too little IYO. It still allows MUCH more than what you can do no, without that patch! And any solution, for core, that allows advanced handling of inline images will either require an enourmous amount of work, or it will simply no get in. So, unless you come up with a good core-worthy patch, this is still a big leap forward from what we have now. ------------------------------------------------------------------------ Sat, 23 Jul 2005 19:01:51 +0000 : Bèr Kessels :$ this is an attempt to fix the borken HML in the previous follow up. ------------------------------------------------------------------------ Sun, 24 Jul 2005 04:07:34 +0000 : TDobes Please don't combine unrelated changes together in the same patch. Moving stuff in/out of the _nodeapi hook has absolutely nothing to do with the patch and makes it much harder to read through the patch and see what it's actually doing. I'd appreciate it if you could separate the changes made here which are really relevant and move the other changes into another issue. Also: You change form_group_collapsible(t('File attachments') to form_group_collapsible(t('File Attachments'). A quick glance at some other core code seems to indicate that we've standardized on capitalizing only the first word in multi-word group titles. As for the functionality itself, I don't think this belongs in upload.module. It would be useful to have the functionality in core, but as a separate module which can be switched on and off. (upload_inline.module or something like that) If a site were set up using an inlining system from contrib and the functionality you're proposing could not be switched off, it would be extremely confusing to users that there are two different ways of inlining images. A minor comment: I'm not so sure whether the results of this patch, when inlining non-image attachments, will be the one users expect. A non-web-savvy person may expect his/her Word document (for instance) to appear inline with the node when they click "inline" -- just like their images do. I'd suggest that we limit inlining to images only. I'm not as adamant about this as the other issues, so I could be convinced otherwise if others disagree. ------------------------------------------------------------------------ Sun, 24 Jul 2005 07:58:19 +0000 : Bèr Kessels Thank you for your thoughts Tdobbes. As for the "moving things in and out of nodeapi" This was necessary, because otherwise the patch would have made the code even more unreadable. I agree with you on this point, but I simply had to clean it up a little, in order to be able to work in it. As it stands now, it is simply not possible to make this a separate module. We cannot hook into the files table. In a next stage, I might work out some hook for that table. But first things first. Can Dries or Steven please comment on his. I want to know If i should bother spending this enormous amount of time on commenting and debating this issue. If the chances are small this is accepted, Ill just drop it This ridiculous long thread reminds me again why I try to avoid making patches for core. ------------------------------------------------------------------------ Mon, 25 Jul 2005 05:49:37 +0000 : Steven Overall I like the concept, but I think it at least needs some default styling to make the image appear okay, like floating it. Right now it appears inline between whatever was there... very ugly. Sure you can say "leave it up to the themer" but if the goal is to provide a simple default, we can't expect people to dive into theming if they are going to use this feature instead of img_assist or whatever. Also, I wonder about the classes "upload" and "image". Is there a reason to use two classes? Isn't "image" a bit too generic? Oh and there is a missing XHTML / for the img tag. ------------------------------------------------------------------------ Mon, 25 Jul 2005 12:42:26 +0000 : Bèr Kessels I will fix the broken Xhttml. I chose "image" because it is exactly the same as the .image class used by image.module. But I will see to it, to add more functionality. I agree that it is not up to the themer for some good defaults, thus I will add a line of CSS to drupal.css (*shiver* ;) ) upload, IMO does not communicate well enough what the Image is doing. Not sure yet how I ill fix this, but I will see to it to come up with a better CSS/theme solution. Thanks! ------------------------------------------------------------------------ Tue, 26 Jul 2005 14:18:23 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_0.patch (12.24 KB) Suggestions by Steven are now included. I very simple float is added to the inline images in drupal.css. XHTML fixed. And I cleaned up some old crufty comments, as suggested by Dries. ------------------------------------------------------------------------ Tue, 26 Jul 2005 17:05:45 +0000 : fax8 What is proposed here is a good idea. But I think that this is done in not good way. I think that upload.module should be only a layer between file system and modules. So modify upload module to add a specific feature like your inline for me is not good. but I like the idea to add checkboxes for enable interaction modules between modules and upload.module. Maybe we could design an of api wich let modules tell upload.module that they can interact with it. I make an example: I'm a developer of video.module. Now to add a video file to video.module one need to put the file into drupal folders using ftp and then insert the path to the file during node creation. One of the features our users are asking a lot is the possibility to directly upload a video file to dupal. So I should write a lot of code, settings, security buggy code.. that for me isn't useful. What about if only calling an upload.module api I should enable a new checkbox on upload form called "use as video file" which let me use the file as a video file on my module??? What do you think about it???? ------------------------------------------------------------------------ Tue, 26 Jul 2005 20:00:03 +0000 : Bèr Kessels fax: upload /is/ such a layer. Just look at how image module does it. You need not write any additional 'buggy' code, you can use all teh upload functionality. The upload interface (the table and list), however, is tightly integrated, so that drupal offers an out-of-the-box file system. Nox, my patch simply adds an "out of the box" inline functionality. Small and simple. For any more advanced file features you should use or write a module that, in turn, uses the uplaod.module. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:22:40 +0000 : stefan nagtegaal After discussing with Ber, I think this patch has some great potential though the simplicity of it.. For the people who don't completely agree what this patch does: - It adds an extra checkbox for each attachment. If the 'Inline' checkbox is checked, images are displayed inline in your post and other attachments (like .pdf, .ppt, or whatever) were transformed into links. The potential of this patch is in the fact that this makes it much, much easier to have another contrib module which makes it possible to let you choose at the node submission how you would like your node to be displayed, incl. the selected inline attachments to be displayed (using _nodeapi()). So, a big +1 from me on this.. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:27:52 +0000 : Kobus > - It adds an extra checkbox for each attachment. If the 'Inline' > checkbox is checked, images are displayed inline in your post and other > attachments (like .pdf, .ppt, or whatever) were transformed into links. Does this mean it is done automatically or do I have control over this? I don't want the images to be inline if I can't control where I put it. Let me just clarify my position on this short-and-sweet-like: If it breaks the existing functionality that upload.module + inline.module provides without opening the door for a replacement, then I am -1, otherwise +1. :-) Kobus ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:51:50 +0000 : Bèr Kessels Kobus: Again: it does not break anything! It opens up enormous potential for modules such as inline/module. Because inline module now has access to a variable $node->files[fid]->inline. in other words: modules such as inline.module can now find out what images a user wants to see inline! ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:59:53 +0000 : Kobus Ber: Great, then I +1 it. From our previous talks about this, it seemed as if this functionality was being taken away. ------------------------------------------------------------------------ Wed, 27 Jul 2005 16:18:21 +0000 : Sohodojo Jim Agreed, +1. The most on-going and especially most recent discussion has clarified that we can have our simple cake and contrib mod cake as well. Does this mean that inline module will work as is, or will a post-patch update be required? --Sohodojo Jim-- ------------------------------------------------------------------------ Wed, 27 Jul 2005 17:33:47 +0000 : Bèr Kessels inline module will continue working. but in order for it to use this inline variable (and become even better) it needs to be patched. ------------------------------------------------------------------------ Thu, 28 Jul 2005 18:12:09 +0000 : moshe weitzman I read through the code and it looks good to me, though it is hard to know whats new given all the code thats moved around. this is definately needed functionality. If you were still up for enhancements, I'd like to see the 'inline' box get checked by default when uploading an image. when the attachment is not an image, the box should be disabled and unchecked. ------------------------------------------------------------------------ Thu, 28 Jul 2005 18:32:40 +0000 : Bèr Kessels disabling the box for non-images would break the link function (i.e add a link in the text. But, on second thought: that is fine, since it defeats the list checkbox. I will add that. As well as making it checked by default. ------------------------------------------------------------------------ Thu, 28 Jul 2005 19:54:14 +0000 : Junyor @Bèr: Why not simplify this patch to only add the hooks to a contrib module would need to add this functionality? You said that your patch would allow inline.module to work much more easily. But then there is a lot of extra stuff there that inline.module won't need. This functionality can't replace inline.module, so anything that it doesn't use just gets in the way. I'm afraid that users will think this is the best Drupal can provide without looking for a contrib module. I'd prefer we spent our time and effort adding the whole functionality of inline.module or making it easy to add and position inline content in nodes to core rather than adding some functionality to upload.module that doesn't cut it for most users. So, either strip this patch of everything but the functionality that inline.module (and similar modules) can use or allow me to position images in my text through this patch. Otherwise, -1. If this is the first step in a plan to add the functionality I want to core, I'd like to hear your full plan. ------------------------------------------------------------------------ Thu, 28 Jul 2005 21:19:37 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_1.patch (19.71 KB) to all: everyone has Big Plans[tm] and even worse: everyone has Bigger Plans for my patch, for inline module. Why not code it? Hooks? fine! module integration, even better. Sorry if I sound a bit grumpy here. But so far I have spent nearly three times as much hours debbating the patch, then I spent creating it. Hooks sound great. but are not the intention of this patch. I want to add a *default* way, that works. But that does not break other modules, nd in fact tries to help them. But I am not inventing some advanced image handling system here. Here is an updated patch with renewed updates.inc (got lost in the previous patch) Moshes suggestion about disabling non-images-checkboxes is also implemented. I like it a lot more now. I did not, however, implement Moshes idea about defaulting to "checked". for a few reasons: * It looks a bit silly if you have more then one image inline, And defaulting all uploads to inline will inline any uploaded image. * When uploading large images they will default to inline, thus pushing your sites layout to oblivian * None of the other options default to TRUE, so for consistancy I prefer to default it to not inline. ------------------------------------------------------------------------ Thu, 28 Jul 2005 21:24:26 +0000 : Bèr Kessels oh, bytheway: you can test adn play with this patch here: http://fixme.remixreading.org/?q=add_content note that it is a sandbox, so your account will get lost, and pages might not work :) ------------------------------------------------------------------------ Fri, 29 Jul 2005 03:47:35 +0000 : ejort +1 from me for this feature. It's frustrating that core doesn't include some basic image display functionality, and this will cover a large percentage of users needs without introducing complexity. People wanting arbitrary image placement still have inline.module etc., so it's nothing but an improvement on the current situation. ------------------------------------------------------------------------ Fri, 29 Jul 2005 06:38:46 +0000 : Steven Bèr: one thing is not clear to me... you are right that other modules can access the inline value. But upload.module will always add the image to the body. So, how can other modules use the inline checkbox/option meaningfully? ------------------------------------------------------------------------ Tue, 02 Aug 2005 18:33:30 +0000 : Bèr Kessels Steven: what I mean, is that they have access to a variable that tells them a user wants something inline. Nothing more. Its up to these modules to use that meaningfully. ------------------------------------------------------------------------ Thu, 01 Sep 2005 15:26:36 +0000 : Bèr Kessels The patch still applies. Dries, please let me know if I must bother trying to get this in before 4.7? ------------------------------------------------------------------------ Thu, 01 Sep 2005 15:30:09 +0000 : m3avrck Ber, check this latest commit: http://drupal.org/node/28483 ... I think your patch no longer applies since this JS based does do inline functionality. ------------------------------------------------------------------------ Thu, 01 Sep 2005 19:46:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_2.patch (20.02 KB) updated patch works with revisions and JS upload. ------------------------------------------------------------------------ Thu, 01 Sep 2005 20:32:11 +0000 : m3avrck got an extra 'favico' in the patch file there. ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:24 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_2_0.patch (19.22 KB) favicon removed. ------------------------------------------------------------------------ Thu, 01 Sep 2005 23:06:11 +0000 : killes(a)www.drop.org I think I'd like to have this feature in core. The patch is quite big, though. ;-/ Also I noticed that the pgsql part is bogus. ------------------------------------------------------------------------ Thu, 01 Sep 2005 23:27:42 +0000 : Bèr Kessels how would the pgsql look then? I am a complete pgSQL nitwit :) ------------------------------------------------------------------------ Fri, 02 Sep 2005 15:18:47 +0000 : telex4 Just a quick +1 from a user who is hoping that this gets into core for 4.7. We're using the inline functionality on our Remix Reading test site at the moment, and it's very handy for our theme where the user uploads any number of files and may want one or two images to be previewed in the node. ------------------------------------------------------------------------ Fri, 02 Sep 2005 17:26:49 +0000 : m3avrck Patch needs to be rerolled. Latest 2_0 version does indeed fix JS-based uploads commit, however it is missing all of the database changes and updates from the last _1 patch. Also, upload_count_size() is undefined, should be using upload_space_used() [2] instead. Also, beginning of upload.module patch adds the default maxsize setting which was previously taken out becuase it wasnt working properly. This patch [3] corrects that and is a seperate issue :) [2] http://drupaldocs.org/api/head/function/upload_space_used [3] http://drupal.org/node/30025 ------------------------------------------------------------------------ Fri, 02 Sep 2005 18:31:12 +0000 : Steven Bèr: I'm afraid you missed my point. It is pretty useless for other modules to be able to read the inline value, when upload.module always adds the image to the node body. Suppose I want to make a module which adds more advanced inline positioning, perhaps with a marker tag. I cannot avoid _upload_inline($node) being called, so I have to somehow remove that (themed!) image tag again. This makes the "other modules can use it" argument pretty silly IMO. ------------------------------------------------------------------------ Fri, 02 Sep 2005 18:48:32 +0000 : Dries It would make sense for Ber to provide one or two examples of how this could be used. ------------------------------------------------------------------------ Fri, 02 Sep 2005 21:02:39 +0000 : Bèr Kessels @Steven: no I did not miss your point. on nodeapi view this is appended to the $node->body, regardless. You are correct. But nothing witholds you from using that variable *before* the view is invoked. ou can then choose to unset the variable, or leave it. I first had yet another hook, that would be called on nodeapi view, by the upload, on a per-file base; but then I found that all this was very OTT, since it ca all be achieved with the existing nodeapi. I reverted to the nodeapi. @Dries; examples, sure. 1) Look for the result at http://fixme.remixreading.org/node/595, the small image at the right is rendered with this variable. 5and I must add that users that tested this behaviour were VERY positive!) In phptemplate, the node, we do: <?php foreach ($node->files as $file): ?> <?php if ($file->inline): ?> <div id="preview"> <img src="files/<?php print $file->filename ?>" alt="preview" title="preview of <?php print $node->title ?>" /> </div> <?php endif; ?> <?php endforeach; ?> Because we have ourtheme_upload_inline() to return NULL, this looks like you see online. Together with some CSS you get a very nice result. 2) For most weblogs, or newssites, you need not do anything, for the $node->body has the image already appended *if* the inline checkbox is checked. Esp since these types of sites need a unified look, they do not (or should not) need to alter the place and aligning of each image. 3) For the power users there are node-layout tools available such as tinyMCE, wikisyntax or even inline.module. Since that last one is the simplest I use that as example. <?php function inline_nodeapi(&$node, $op, $arg) { if(is_array($node->files) && $op == 'view') { //on view we replace the tokens with the img tags $node->teaser = _inline_substitute_tags($node, 'teaser'); $node->body = _inline_substitute_tags($node, 'body'); } elseif(is_array($node->files) && $op == 'validate') { $node = _inline_remove_tags($node); //remove the tags that are not flagged as inline } elseif(is_array($node->files) && $op == 'load') { _inline_remove_flags($node) //remove all $node->files[XX]->inline variables, so that upload does not render them. } elseif(is_array($node->files) && $op == 'from pre') { $node = _inline_add_tags($node); //add [inline:X] tokens to 'body', so editor can use them. $node = _inline_remove_tags($node); //remove [inline] tokens that call files that are (no longer) marked for inline use. } } //.... function _inline_add_tags(&$node, $field) { foreach ($node->files as $file) { if ($file->inline && !_inline_check_for_tag($file->filename)) { //look if the tag is not already in body, and if the file is marked for inline usage. $node->body = _inline_add_single_tag($file) . $node->body; //Actually add the token to the beginning of the body $node->teaser = _inline_add_single_tag($file) . $node->teaser; // same for teaser } } } ?> //note. preview is broken no Idea how this post looks ;) ------------------------------------------------------------------------ Sat, 03 Sep 2005 04:37:23 +0000 : TDobes Bèr: The idea of clearing $node->inline never occurred to me... but won't that only work some of the time? It depends on the order in which modules' _nodeapi hooks get called. I believe they get called alphebetically (there's an asort in module_list), so a module whose name starts with v, w, x, y, or z will not be able to prevent upload.module from displaying its inline image. While I do feel this functionality is useful, I think a better way of implementing it would be to allow other modules to add columns to the upload table. Then a module could invoke this upload hook to add a field for "display inline" and invoke _nodeapi to actually place the image. This would provide for future expansion of the upload module in addition to solving the immediate goal. ------------------------------------------------------------------------ Sat, 03 Sep 2005 06:37:17 +0000 : Bèr Kessels in short my battle plan was: * introduce inline stuff * introduce hook for altering the file table Why i did not go for #2 at once? because IMO you can do two things wit a file: show it, or download it. Anything else should be done in a file admin UI. And furthermore: yes modules are ran in alphabetic order, but, nodeapi(load) is ALWAYS ran before nodeapi(view). So you can always make sure on nodeapi(view) the variable is unset. ------------------------------------------------------------------------ Sat, 03 Sep 2005 12:22:00 +0000 : moshe weitzman I think that Ber's suggestion to unset the variable is a great, simple workaround. I don't see any more outstanding objections to the patch. Yes, it can be improved later, but it is useful already. ------------------------------------------------------------------------ Sat, 03 Sep 2005 13:08:05 +0000 : Bèr Kessels the patch is still in brokenish state. I am trying to free some time this weekend (sunday evening) to fix it and synch with HEAd again. not h revisions and JS upload broke this patch. Its a lot of work to reroll, due to that amount of changes (nearly a complete rewrite, I am afraid) ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:48:47 +0000 : Bèr Kessels please review. This a complete rewrite, * I ignored all the huge fucntions and just left tath for another ime to clean up (never). It does make the patch easier to read and smaller, though * I left the 'upload preview' bug for what it is: it belons in a differnt issue, and would clutter this inline feature patch a lot. For the rest see above. * Themes could be updated to do nice stuff to the inline patch. I did not do that, for that goes far beyond what this patch tries to do

1 0

[drupal-devel] [feature] add inline functionaliy to uploads
by Bèr Kessels 12 Sep '05

12 Sep '05

Issue status update for http://drupal.org/node/26288 Post a follow up: http://drupal.org/project/comments/add/26288 Project: Drupal Version: cvs Component: upload.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Bèr Kessels Updated by: Bèr Kessels -Status: patch (code needs work) +Status: patch (code needs review) please review. This a complete rewrite, * I ignored all the huge fucntions and just left tath for another ime to clean up (never). It does make the patch easier to read and smaller, though * I left the 'upload preview' bug for what it is: it belons in a differnt issue, and would clutter this inline feature patch a lot. For the rest see above. * Themes could be updated to do nice stuff to the inline patch. I did not do that, for that goes far beyond what this patch tries to do Bèr Kessels Previous comments: ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:03:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline.patch (19.05 KB) One of the most often asked features is proper inlnie handling of files. Look at the amount of solutions, the popularity of image_assist, and the amount of peolpe dowloading image.module! That alone should be enough proof that Drupal lacks proper inline image support. This patch adds that to core. In fact, it does little more then appending a link of img tag to the body or the teaser. Off course that is configurable per file. Next to the [] list checkbox, this patch adds an [] inline checkbox. Simplicity is the foundation of this patch. I want no stles for inline editing, no fancy html wrappers, no tokens, just $node->body or teaser appended with a small html string. Another small themable funtion is introduced, (hey, you cannot expect me to develop something without adding more power for themers, now, can you? ;) ), that allows people to theme the string that is appended to the body or the teaser. Oh, and also note hat the biggest part of this patch is some cleaning I had to do in order to be able to develop properly. I dont like Ifs inside cases in foreaches inside swiches. in other words: nodeapi now calls functions instead of executing code directly. ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:25 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot.png (26.68 KB) here is how the form now looks ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:58 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot3.png (30.53 KB) and this is an example of inlined images and a .doc file. ------------------------------------------------------------------------ Sun, 03 Jul 2005 20:44:00 +0000 : sepeck changing to patch per request from berkes ------------------------------------------------------------------------ Tue, 05 Jul 2005 07:46:13 +0000 : Kobus This gets a +1 from me in principle, however, the [inline:xx] tag with inline.module gives you greater freedom as to where the inline image must be displayed. If you can add this functionality (I haven't checked the code, I don't know if it is in there) it would be a great addition for Core. This same strategy can be used for inline blocks, I am sure. Regards, Kobus ------------------------------------------------------------------------ Tue, 05 Jul 2005 08:57:50 +0000 : Bèr Kessels there are a couple of reasons why i did not include the [inline] tags. * I aimed for extreme simplicity: a checkbox shows an image inline: its up to the theme where it appears (if one does not like it before/above the body and teaser.). Simplicity was the main goal. * We don't have any tokens in core. And we should not have them. * Tokens are a very bad substitute for a good interface. They give less power then plain HTLM. Are much worst documented then HTML, but in the mean time, they are still as hard to learn as HTML. (Yes, I know people _think_ they are easier, but there _is_ not difference between [ ] and , only that its a different ascii char. So, no. I don't allow any placement of the image. I leave that that for dedicated modules, or the themer to decide. ------------------------------------------------------------------------ Tue, 05 Jul 2005 09:34:06 +0000 : Kobus So you will provide an API to do this? For example, with perhaps minor modifications, the inline.module will be able to display these files? In that case I am happy. If not, then I can't give my support to this patch (as if that matters...). A themer can't do this task as inline images (and blocks for that matter) is too dynamic for theming; it can be placed anywhere in a node where the user pleases. This means for me that there should be a module for this, such as inline module that allows you to define [inline:xx] tags. If your module emits an array of uploaded files (such as upload.module), inline.module can be, with minor changes, adapted to show these files inline. To show you what I mean with the content is too dynamic for a themer to perform this task, look at this screenshot related to inline blocks (inline images can follow the same pattern): http://drupal.org/files/issues/regions---possibility-3.png ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:26:15 +0000 : Bèr Kessels Kobus, We are dealing with different issues here: You want a method to place images, files or so anywhere in the post. That is fine, but certainly NOT addressed in the patch! I made patch that *only* adds marked files to the body. its really nothing more then <?php $node->body = $filestring . $node->body ?> No APIS, no, dynamic tokens, no filters, nothing. However, what I meant with themers, is that there now is a theme_upload_inline available, so you can theme the abovementioned $filestring. On top of that $node->files[FILEID]->inline is TRUE if a file is flagged for inline. So in node.tpl.php, or wherever you want to theme a node, you can print nice images inline, when that flag is set. And about the comment that a themer cant do this: Simply not true. On most sites images are always placed in the same places. REally, even the sites see which use img_assist or inline, use them to place the images on the exact same places in every node. People /think/ they want the power to place images anywhere, but they hardly ever use that power. Just look at all the big news/publishing sites out there (BBC, CNN, BoigBoing, r even freshmeat) images are all placed acc. to the theme. They are not placed in random places by authors. So if you are one of the few that still want that power, there aer mots of power tools like inline module or img_assist. We should offer a good default, one that is simple. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:41:34 +0000 : Gerhard Killesreiter I fully agree that tags are evil. Kobus: You can always look for tags in $node->body in your theme's node function. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:50:52 +0000 : stefan nagtegaal Ber, the theme-function in your code looks like: <?php +/** + * Theme function for rendering of inline images + * @param $file a file object. + * @param $image a Boolean, indicating whether an img tag (TRUE) or an anchor tag (FALSE) should be used. + * @ingroup themable + */ +function theme_upload_inline($file, $image) { + if ($image) { + return '<img src="'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'" alt="'. check_plain($file->filename) .'" class="upload inline">'; + } + else { + return ''. check_plain($file->filename) .' [1]'; + } + +} ?> After looking at your code it's not clear to me when $image is TRUE or FALSE, can you elaborate on me please? [1] http://drupal.org/'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'\" class=\"upload inline ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:06:39 +0000 : Bèr Kessels The function calling the theme function should decide whether its an image. IMO that is far too hardcore code for a themer ;) <?php $image = ereg('^(image/)', $file->filemime); ?> inside _upload_inline() does the trick. I did find one issue, though, with svgs, which are image/svg so maybe we should limit this to really only inline jpg, png, and gif, by extension? But I am no fan of determining files by extension, and IMO getimgsize is too heavy; ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:13:52 +0000 : stefan nagtegaal Can't you make use of PHP's <?php image_type_to_mime_type(); ?> or <?php image_type_to_extension(); ?> ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:45:43 +0000 : Kobus Well, if not in the patch, I need inline functionality one way or another. inline.module works perfectly with the current upload.module to fulfill my needs, and if there is a way where someone can extend your proposed upload.module, that would get my +1, otherwise I will just remain neutral about this issue. inline.module makes use of $node->files and displays that image. If your patch still uses $node->files, then this is a non-issue. In other words, if I can, with inline.module, or with minor modifications to inline.module still show files inline, I am +1 for this. As for the themability question... I still disagree. You say "Simply not true. On most sites images are always placed in the same places." That is ONLY true because they have NO alternatives. Means the content is in the same places, because they have no other places to put them. I have made tens of static sites where the content does not resemble a fixed pattern or structure. For me, free-flow layout is my expression of my creativity. This is why I still even BOTHER with static sites; when Drupal don't do what I want it to do. If Drupal could do inline display of content properly, I'd never even build another static site. It is a great mistake people make (including myself), and that is they design the site before ANY content is available, and this happens a lot in static sites. But in dynamic sites, you have far less control over this, unless you have a VERY clean structure with limited information that can be added, and only a few people posting content. If you can have your content before you start the design, you will have a better fit. I can't see how you can anticipate every possible posting posted on your site if you have a diversity of users. Especially not if you're using a site such as an designer's showcase site where your creativity is shown by your web sites. Possible, but not easy. If I claim that I have "unique, fresh designs" I can certainly NOT give them a "box-like" site with "left and right side bars only". That's why I need inline content, this includes images AND boxes. Gerhard: Your concern about tags is answered as well above, if I am not mistaken. The patch need not provide the tags, but should provide for someone to develop a module that provide the filters or tags. To summarize: I don't need your patch to do the inline images. I just need your patch to be able to allow someone else to develop a module that can display inline files. Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 09:56:38 +0000 : Bèr Kessels okay, so for all clarity, let me sumarise: * this patch introduces a chackbox "inline". If checked, the file will show up inline. IT will be appended to the teaser and the body. * this patch does *not* allow one to place images of files in a userdefined place in the body. * this patch does *not* do any resizing nor any thumbnailing. * This patch is meant to be simple, clear and transparant. No tokens, no javascript, no nothing. ------------------------------------------------------------------------ Mon, 18 Jul 2005 11:21:54 +0000 : Kobus Then I can't see how this patch could replace the existing upload.module, which allows you to use inline.module to place images inline. I therefore withdraw my +1 and go -1 on this (not that it matters, I believe). I think you didn't properly read my previous message. I said I don't need your patch to do all this, but I can't live without the functionality currently provided by the upload.module and inline.module combination. I agree that your patch don't need to have that functionality, but if your patch takes away this functionality, in other words, I can't (by hook or by crook) manage what I need to do, my vote is -1. Regards, Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 19:58:34 +0000 : Bèr Kessels Kobus, And all the others looking fofr advanced inline systems: please do not -1 this. It will hep inline module a lot too. allthough not yet in this patch, but can you imagine a perission system for inline module, that comes for free? Or a core system, that makes inline module ten times smaller? it is this path! so, please, if it is not /exactly/ what you are looking for, at least look at the advantages for Joe Average, and even for the possibilties for your favorite inline-module. even imag_assist will gain an anomous usability impcact when usiong this module, for it can then finally use the upload module in full scale. Ber ------------------------------------------------------------------------ Sat, 23 Jul 2005 18:59:10 +0000 : Bèr Kessels Sohodjo jim: " I hope it matters. A big -1 on enforcing over-simplification at the expense of important and existing functionality. If automatic, uncontrollable placement of images becomes the standard, it will mean fewer not more image-rich Drupal sites as site owners/developers get complaints about "Why can't I control my images!" I _strongly_ encourage Ber to provide a "have it both ways" solution. Why not have an admin configuration setting for "allow inline tags" with default off. Change it to on and you get an additional upload checkbox for 'inline at tag' to accommodate the current functionality of the inline module while simultaneously allowing for default placement. " NOOO. people; please; look at the PATCH. Again: and hopefully last time: Simplicity. This patch does NOT replace ANYTHING inline module or img_assist wants to do. This patch hands better data to such modules. It hands a variable over to these modules, $node->file[FID]->inline, which tells these sorts of modules if people wnat that file to appear inline. AND it CAN (by default will) render a file in the most simple way inline. So really, if you want to -1 this patch, fine. But do not -1 it, because it does too little IYO. It still allows MUCH more than what you can do no, without that patch! And any solution, for core, that allows advanced handling of inline images will either require an enourmous amount of work, or it will simply no get in. So, unless you come up with a good core-worthy patch, this is still a big leap forward from what we have now. ------------------------------------------------------------------------ Sat, 23 Jul 2005 19:01:51 +0000 : Bèr Kessels :$ this is an attempt to fix the borken HML in the previous follow up. ------------------------------------------------------------------------ Sun, 24 Jul 2005 04:07:34 +0000 : TDobes Please don't combine unrelated changes together in the same patch. Moving stuff in/out of the _nodeapi hook has absolutely nothing to do with the patch and makes it much harder to read through the patch and see what it's actually doing. I'd appreciate it if you could separate the changes made here which are really relevant and move the other changes into another issue. Also: You change form_group_collapsible(t('File attachments') to form_group_collapsible(t('File Attachments'). A quick glance at some other core code seems to indicate that we've standardized on capitalizing only the first word in multi-word group titles. As for the functionality itself, I don't think this belongs in upload.module. It would be useful to have the functionality in core, but as a separate module which can be switched on and off. (upload_inline.module or something like that) If a site were set up using an inlining system from contrib and the functionality you're proposing could not be switched off, it would be extremely confusing to users that there are two different ways of inlining images. A minor comment: I'm not so sure whether the results of this patch, when inlining non-image attachments, will be the one users expect. A non-web-savvy person may expect his/her Word document (for instance) to appear inline with the node when they click "inline" -- just like their images do. I'd suggest that we limit inlining to images only. I'm not as adamant about this as the other issues, so I could be convinced otherwise if others disagree. ------------------------------------------------------------------------ Sun, 24 Jul 2005 07:58:19 +0000 : Bèr Kessels Thank you for your thoughts Tdobbes. As for the "moving things in and out of nodeapi" This was necessary, because otherwise the patch would have made the code even more unreadable. I agree with you on this point, but I simply had to clean it up a little, in order to be able to work in it. As it stands now, it is simply not possible to make this a separate module. We cannot hook into the files table. In a next stage, I might work out some hook for that table. But first things first. Can Dries or Steven please comment on his. I want to know If i should bother spending this enormous amount of time on commenting and debating this issue. If the chances are small this is accepted, Ill just drop it This ridiculous long thread reminds me again why I try to avoid making patches for core. ------------------------------------------------------------------------ Mon, 25 Jul 2005 05:49:37 +0000 : Steven Overall I like the concept, but I think it at least needs some default styling to make the image appear okay, like floating it. Right now it appears inline between whatever was there... very ugly. Sure you can say "leave it up to the themer" but if the goal is to provide a simple default, we can't expect people to dive into theming if they are going to use this feature instead of img_assist or whatever. Also, I wonder about the classes "upload" and "image". Is there a reason to use two classes? Isn't "image" a bit too generic? Oh and there is a missing XHTML / for the img tag. ------------------------------------------------------------------------ Mon, 25 Jul 2005 12:42:26 +0000 : Bèr Kessels I will fix the broken Xhttml. I chose "image" because it is exactly the same as the .image class used by image.module. But I will see to it, to add more functionality. I agree that it is not up to the themer for some good defaults, thus I will add a line of CSS to drupal.css (*shiver* ;) ) upload, IMO does not communicate well enough what the Image is doing. Not sure yet how I ill fix this, but I will see to it to come up with a better CSS/theme solution. Thanks! ------------------------------------------------------------------------ Tue, 26 Jul 2005 14:18:23 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_0.patch (12.24 KB) Suggestions by Steven are now included. I very simple float is added to the inline images in drupal.css. XHTML fixed. And I cleaned up some old crufty comments, as suggested by Dries. ------------------------------------------------------------------------ Tue, 26 Jul 2005 17:05:45 +0000 : fax8 What is proposed here is a good idea. But I think that this is done in not good way. I think that upload.module should be only a layer between file system and modules. So modify upload module to add a specific feature like your inline for me is not good. but I like the idea to add checkboxes for enable interaction modules between modules and upload.module. Maybe we could design an of api wich let modules tell upload.module that they can interact with it. I make an example: I'm a developer of video.module. Now to add a video file to video.module one need to put the file into drupal folders using ftp and then insert the path to the file during node creation. One of the features our users are asking a lot is the possibility to directly upload a video file to dupal. So I should write a lot of code, settings, security buggy code.. that for me isn't useful. What about if only calling an upload.module api I should enable a new checkbox on upload form called "use as video file" which let me use the file as a video file on my module??? What do you think about it???? ------------------------------------------------------------------------ Tue, 26 Jul 2005 20:00:03 +0000 : Bèr Kessels fax: upload /is/ such a layer. Just look at how image module does it. You need not write any additional 'buggy' code, you can use all teh upload functionality. The upload interface (the table and list), however, is tightly integrated, so that drupal offers an out-of-the-box file system. Nox, my patch simply adds an "out of the box" inline functionality. Small and simple. For any more advanced file features you should use or write a module that, in turn, uses the uplaod.module. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:22:40 +0000 : stefan nagtegaal After discussing with Ber, I think this patch has some great potential though the simplicity of it.. For the people who don't completely agree what this patch does: - It adds an extra checkbox for each attachment. If the 'Inline' checkbox is checked, images are displayed inline in your post and other attachments (like .pdf, .ppt, or whatever) were transformed into links. The potential of this patch is in the fact that this makes it much, much easier to have another contrib module which makes it possible to let you choose at the node submission how you would like your node to be displayed, incl. the selected inline attachments to be displayed (using _nodeapi()). So, a big +1 from me on this.. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:27:52 +0000 : Kobus > - It adds an extra checkbox for each attachment. If the 'Inline' > checkbox is checked, images are displayed inline in your post and other > attachments (like .pdf, .ppt, or whatever) were transformed into links. Does this mean it is done automatically or do I have control over this? I don't want the images to be inline if I can't control where I put it. Let me just clarify my position on this short-and-sweet-like: If it breaks the existing functionality that upload.module + inline.module provides without opening the door for a replacement, then I am -1, otherwise +1. :-) Kobus ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:51:50 +0000 : Bèr Kessels Kobus: Again: it does not break anything! It opens up enormous potential for modules such as inline/module. Because inline module now has access to a variable $node->files[fid]->inline. in other words: modules such as inline.module can now find out what images a user wants to see inline! ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:59:53 +0000 : Kobus Ber: Great, then I +1 it. From our previous talks about this, it seemed as if this functionality was being taken away. ------------------------------------------------------------------------ Wed, 27 Jul 2005 16:18:21 +0000 : Sohodojo Jim Agreed, +1. The most on-going and especially most recent discussion has clarified that we can have our simple cake and contrib mod cake as well. Does this mean that inline module will work as is, or will a post-patch update be required? --Sohodojo Jim-- ------------------------------------------------------------------------ Wed, 27 Jul 2005 17:33:47 +0000 : Bèr Kessels inline module will continue working. but in order for it to use this inline variable (and become even better) it needs to be patched. ------------------------------------------------------------------------ Thu, 28 Jul 2005 18:12:09 +0000 : moshe weitzman I read through the code and it looks good to me, though it is hard to know whats new given all the code thats moved around. this is definately needed functionality. If you were still up for enhancements, I'd like to see the 'inline' box get checked by default when uploading an image. when the attachment is not an image, the box should be disabled and unchecked. ------------------------------------------------------------------------ Thu, 28 Jul 2005 18:32:40 +0000 : Bèr Kessels disabling the box for non-images would break the link function (i.e add a link in the text. But, on second thought: that is fine, since it defeats the list checkbox. I will add that. As well as making it checked by default. ------------------------------------------------------------------------ Thu, 28 Jul 2005 19:54:14 +0000 : Junyor @Bèr: Why not simplify this patch to only add the hooks to a contrib module would need to add this functionality? You said that your patch would allow inline.module to work much more easily. But then there is a lot of extra stuff there that inline.module won't need. This functionality can't replace inline.module, so anything that it doesn't use just gets in the way. I'm afraid that users will think this is the best Drupal can provide without looking for a contrib module. I'd prefer we spent our time and effort adding the whole functionality of inline.module or making it easy to add and position inline content in nodes to core rather than adding some functionality to upload.module that doesn't cut it for most users. So, either strip this patch of everything but the functionality that inline.module (and similar modules) can use or allow me to position images in my text through this patch. Otherwise, -1. If this is the first step in a plan to add the functionality I want to core, I'd like to hear your full plan. ------------------------------------------------------------------------ Thu, 28 Jul 2005 21:19:37 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_1.patch (19.71 KB) to all: everyone has Big Plans[tm] and even worse: everyone has Bigger Plans for my patch, for inline module. Why not code it? Hooks? fine! module integration, even better. Sorry if I sound a bit grumpy here. But so far I have spent nearly three times as much hours debbating the patch, then I spent creating it. Hooks sound great. but are not the intention of this patch. I want to add a *default* way, that works. But that does not break other modules, nd in fact tries to help them. But I am not inventing some advanced image handling system here. Here is an updated patch with renewed updates.inc (got lost in the previous patch) Moshes suggestion about disabling non-images-checkboxes is also implemented. I like it a lot more now. I did not, however, implement Moshes idea about defaulting to "checked". for a few reasons: * It looks a bit silly if you have more then one image inline, And defaulting all uploads to inline will inline any uploaded image. * When uploading large images they will default to inline, thus pushing your sites layout to oblivian * None of the other options default to TRUE, so for consistancy I prefer to default it to not inline. ------------------------------------------------------------------------ Thu, 28 Jul 2005 21:24:26 +0000 : Bèr Kessels oh, bytheway: you can test adn play with this patch here: http://fixme.remixreading.org/?q=add_content note that it is a sandbox, so your account will get lost, and pages might not work :) ------------------------------------------------------------------------ Fri, 29 Jul 2005 03:47:35 +0000 : ejort +1 from me for this feature. It's frustrating that core doesn't include some basic image display functionality, and this will cover a large percentage of users needs without introducing complexity. People wanting arbitrary image placement still have inline.module etc., so it's nothing but an improvement on the current situation. ------------------------------------------------------------------------ Fri, 29 Jul 2005 06:38:46 +0000 : Steven Bèr: one thing is not clear to me... you are right that other modules can access the inline value. But upload.module will always add the image to the body. So, how can other modules use the inline checkbox/option meaningfully? ------------------------------------------------------------------------ Tue, 02 Aug 2005 18:33:30 +0000 : Bèr Kessels Steven: what I mean, is that they have access to a variable that tells them a user wants something inline. Nothing more. Its up to these modules to use that meaningfully. ------------------------------------------------------------------------ Thu, 01 Sep 2005 15:26:36 +0000 : Bèr Kessels The patch still applies. Dries, please let me know if I must bother trying to get this in before 4.7? ------------------------------------------------------------------------ Thu, 01 Sep 2005 15:30:09 +0000 : m3avrck Ber, check this latest commit: http://drupal.org/node/28483 ... I think your patch no longer applies since this JS based does do inline functionality. ------------------------------------------------------------------------ Thu, 01 Sep 2005 19:46:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_2.patch (20.02 KB) updated patch works with revisions and JS upload. ------------------------------------------------------------------------ Thu, 01 Sep 2005 20:32:11 +0000 : m3avrck got an extra 'favico' in the patch file there. ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:24 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_2_0.patch (19.22 KB) favicon removed. ------------------------------------------------------------------------ Thu, 01 Sep 2005 23:06:11 +0000 : killes(a)www.drop.org I think I'd like to have this feature in core. The patch is quite big, though. ;-/ Also I noticed that the pgsql part is bogus. ------------------------------------------------------------------------ Thu, 01 Sep 2005 23:27:42 +0000 : Bèr Kessels how would the pgsql look then? I am a complete pgSQL nitwit :) ------------------------------------------------------------------------ Fri, 02 Sep 2005 15:18:47 +0000 : telex4 Just a quick +1 from a user who is hoping that this gets into core for 4.7. We're using the inline functionality on our Remix Reading test site at the moment, and it's very handy for our theme where the user uploads any number of files and may want one or two images to be previewed in the node. ------------------------------------------------------------------------ Fri, 02 Sep 2005 17:26:49 +0000 : m3avrck Patch needs to be rerolled. Latest 2_0 version does indeed fix JS-based uploads commit, however it is missing all of the database changes and updates from the last _1 patch. Also, upload_count_size() is undefined, should be using upload_space_used() [2] instead. Also, beginning of upload.module patch adds the default maxsize setting which was previously taken out becuase it wasnt working properly. This patch [3] corrects that and is a seperate issue :) [2] http://drupaldocs.org/api/head/function/upload_space_used [3] http://drupal.org/node/30025 ------------------------------------------------------------------------ Fri, 02 Sep 2005 18:31:12 +0000 : Steven Bèr: I'm afraid you missed my point. It is pretty useless for other modules to be able to read the inline value, when upload.module always adds the image to the node body. Suppose I want to make a module which adds more advanced inline positioning, perhaps with a marker tag. I cannot avoid _upload_inline($node) being called, so I have to somehow remove that (themed!) image tag again. This makes the "other modules can use it" argument pretty silly IMO. ------------------------------------------------------------------------ Fri, 02 Sep 2005 18:48:32 +0000 : Dries It would make sense for Ber to provide one or two examples of how this could be used. ------------------------------------------------------------------------ Fri, 02 Sep 2005 21:02:39 +0000 : Bèr Kessels @Steven: no I did not miss your point. on nodeapi view this is appended to the $node->body, regardless. You are correct. But nothing witholds you from using that variable *before* the view is invoked. ou can then choose to unset the variable, or leave it. I first had yet another hook, that would be called on nodeapi view, by the upload, on a per-file base; but then I found that all this was very OTT, since it ca all be achieved with the existing nodeapi. I reverted to the nodeapi. @Dries; examples, sure. 1) Look for the result at http://fixme.remixreading.org/node/595, the small image at the right is rendered with this variable. 5and I must add that users that tested this behaviour were VERY positive!) In phptemplate, the node, we do: <?php foreach ($node->files as $file): ?> <?php if ($file->inline): ?> <div id="preview"> <img src="files/<?php print $file->filename ?>" alt="preview" title="preview of <?php print $node->title ?>" /> </div> <?php endif; ?> <?php endforeach; ?> Because we have ourtheme_upload_inline() to return NULL, this looks like you see online. Together with some CSS you get a very nice result. 2) For most weblogs, or newssites, you need not do anything, for the $node->body has the image already appended *if* the inline checkbox is checked. Esp since these types of sites need a unified look, they do not (or should not) need to alter the place and aligning of each image. 3) For the power users there are node-layout tools available such as tinyMCE, wikisyntax or even inline.module. Since that last one is the simplest I use that as example. <?php function inline_nodeapi(&$node, $op, $arg) { if(is_array($node->files) && $op == 'view') { //on view we replace the tokens with the img tags $node->teaser = _inline_substitute_tags($node, 'teaser'); $node->body = _inline_substitute_tags($node, 'body'); } elseif(is_array($node->files) && $op == 'validate') { $node = _inline_remove_tags($node); //remove the tags that are not flagged as inline } elseif(is_array($node->files) && $op == 'load') { _inline_remove_flags($node) //remove all $node->files[XX]->inline variables, so that upload does not render them. } elseif(is_array($node->files) && $op == 'from pre') { $node = _inline_add_tags($node); //add [inline:X] tokens to 'body', so editor can use them. $node = _inline_remove_tags($node); //remove [inline] tokens that call files that are (no longer) marked for inline use. } } //.... function _inline_add_tags(&$node, $field) { foreach ($node->files as $file) { if ($file->inline && !_inline_check_for_tag($file->filename)) { //look if the tag is not already in body, and if the file is marked for inline usage. $node->body = _inline_add_single_tag($file) . $node->body; //Actually add the token to the beginning of the body $node->teaser = _inline_add_single_tag($file) . $node->teaser; // same for teaser } } } ?> //note. preview is broken no Idea how this post looks ;) ------------------------------------------------------------------------ Sat, 03 Sep 2005 04:37:23 +0000 : TDobes Bèr: The idea of clearing $node->inline never occurred to me... but won't that only work some of the time? It depends on the order in which modules' _nodeapi hooks get called. I believe they get called alphebetically (there's an asort in module_list), so a module whose name starts with v, w, x, y, or z will not be able to prevent upload.module from displaying its inline image. While I do feel this functionality is useful, I think a better way of implementing it would be to allow other modules to add columns to the upload table. Then a module could invoke this upload hook to add a field for "display inline" and invoke _nodeapi to actually place the image. This would provide for future expansion of the upload module in addition to solving the immediate goal. ------------------------------------------------------------------------ Sat, 03 Sep 2005 06:37:17 +0000 : Bèr Kessels in short my battle plan was: * introduce inline stuff * introduce hook for altering the file table Why i did not go for #2 at once? because IMO you can do two things wit a file: show it, or download it. Anything else should be done in a file admin UI. And furthermore: yes modules are ran in alphabetic order, but, nodeapi(load) is ALWAYS ran before nodeapi(view). So you can always make sure on nodeapi(view) the variable is unset. ------------------------------------------------------------------------ Sat, 03 Sep 2005 12:22:00 +0000 : moshe weitzman I think that Ber's suggestion to unset the variable is a great, simple workaround. I don't see any more outstanding objections to the patch. Yes, it can be improved later, but it is useful already. ------------------------------------------------------------------------ Sat, 03 Sep 2005 13:08:05 +0000 : Bèr Kessels the patch is still in brokenish state. I am trying to free some time this weekend (sunday evening) to fix it and synch with HEAd again. not h revisions and JS upload broke this patch. Its a lot of work to reroll, due to that amount of changes (nearly a complete rewrite, I am afraid)

1 0

[drupal-devel] [bug] theme_image() doesn't actually output width and height of image
by m3avrck 12 Sep '05

12 Sep '05

Issue status update for http://drupal.org/node/30935 Post a follow up: http://drupal.org/project/comments/add/30935 Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck Status: patch (ready to be committed) Well the actual code in theme_image() returned this: return '<img src="'. check_url($path) .'" alt="'. check_plain($alt) .'" title="'. check_plain($title) .'" '. $image_attributes . $attributes .'/>'; There is *no* mention of the $width and $height variables that are assigned above, not used at all. I checked images in the themes directory and none had this information, unless I was missing something obvious, I see no way that is being generated... nothing in the above img src about it. m3avrck Previous comments: ------------------------------------------------------------------------ Sun, 11 Sep 2005 16:22:28 +0000 : m3avrck Function theme_image() doesn't actually return a width and height for an image like it claims to do. ------------------------------------------------------------------------ Sun, 11 Sep 2005 19:30:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_11.patch (2.74 KB) Ok patched attached, which fixes this issue. Also, included a patch for system.module which sets the screen shots to 'TRUE' so image dimensions will also be outputted there as well (which they should be!). ------------------------------------------------------------------------ Sun, 11 Sep 2005 19:33:41 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_12.patch (2.75 KB) Fixed a tab issue. ------------------------------------------------------------------------ Sun, 11 Sep 2005 19:35:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_13.patch (2.75 KB) Fixed a spacing issue. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:18:32 +0000 : Souvent22 Used the patch, and did a quick test. Worked well for me. +1. ------------------------------------------------------------------------ Mon, 12 Sep 2005 16:21:16 +0000 : Robrecht Jacques I don't see why this patch is needed, "theme_image" returns a img tag with the width and height set if $getsize = TRUE. Eg: $node->body = theme('image', file_create_path('druplicon.png'), 'no alt', 'no title', array(), TRUE) . theme('image', file_create_path('druplicon.png'), 'no alt', 'no title', array(), FALSE) will return: <img src="files/druplicon.png" alt="no alt" title="no title" width="88" height="100" /> <img src="files/druplicon.png" alt="no alt" title="no title" /> (if druplicon.png is copied to the files/ directory). I don't see what you are fixing... You are right about the use of theme('image') in system.module though. The "false" should be "true".

1 0

[drupal-devel] [bug] theme_image() doesn't actually output width and height of image
by Robrecht Jacques 12 Sep '05

12 Sep '05

Issue status update for http://drupal.org/node/30935 Post a follow up: http://drupal.org/project/comments/add/30935 Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: Robrecht Jacques Status: patch (ready to be committed) I don't see why this patch is needed, "theme_image" returns a img tag with the width and height set if $getsize = TRUE. Eg: $node->body = theme('image', file_create_path('druplicon.png'), 'no alt', 'no title', array(), TRUE) . theme('image', file_create_path('druplicon.png'), 'no alt', 'no title', array(), FALSE) will return: <img src="files/druplicon.png" alt="no alt" title="no title" width="88" height="100" /> <img src="files/druplicon.png" alt="no alt" title="no title" /> (if druplicon.png is copied to the files/ directory). I don't see what you are fixing... You are right about the use of theme('image') in system.module though. The "false" should be "true". Robrecht Jacques Previous comments: ------------------------------------------------------------------------ Sun, 11 Sep 2005 16:22:28 +0000 : m3avrck Function theme_image() doesn't actually return a width and height for an image like it claims to do. ------------------------------------------------------------------------ Sun, 11 Sep 2005 19:30:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_11.patch (2.74 KB) Ok patched attached, which fixes this issue. Also, included a patch for system.module which sets the screen shots to 'TRUE' so image dimensions will also be outputted there as well (which they should be!). ------------------------------------------------------------------------ Sun, 11 Sep 2005 19:33:41 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_12.patch (2.75 KB) Fixed a tab issue. ------------------------------------------------------------------------ Sun, 11 Sep 2005 19:35:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_13.patch (2.75 KB) Fixed a spacing issue. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:18:32 +0000 : Souvent22 Used the patch, and did a quick test. Worked well for me. +1.

1 0