July 2005 - Development - lists.drupal.org

[drupal-devel] [feature] add inline functionaliy to uploads
by stefan nagtegaal 27 Jul '05

27 Jul '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: stefan nagtegaal Status: patch 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.. stefan nagtegaal 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.

1 0

[drupal-devel] user galleries: submit a screenie of your site
by Bèr Kessels 27 Jul '05

27 Jul '05

A random idea: * Every use can submit images, screenshots of his or her site, into the gallery. Off course them may add some text or a link to the site * they go in the mod. queue * a drupal.org administrator/moderator can then unqueue them This way we can build a nice gallery of example drupal sites. We can "replace" the ugly /sites until we get a better solution for that. And we can kill the myth that drupal is ugly and hard to theme. (there are pure beauties out there) we can evenchose to rotate them randomly on the place where we now show the three collaged screenies, no the frontpage. Yesterday I came by the site of Inkscape (http://www.inkscape.org) And thought they had a fabulous idea implemented. On the frontpage they randomly rotate a screenshot from the screenshot gallery. That is hat brought me to this idea. Regards, Bèr -- [ Bèr Kessels | Drupal services www.webschuur.com ]

2 1

[drupal-devel] [feature] Node module should provide node object to theme search hook
by Eric Scouten 27 Jul '05

27 Jul '05

Project: Drupal Version: 4.5.0 Component: page.module Category: feature requests Priority: normal Assigned to: Eric Scouten Reported by: Eric Scouten Updated by: Eric Scouten Status: patch Attachment: http://drupal.org/files/issues/node_search_hook.patch (719 bytes) As currently defined node.module's search hook doesn't provide enough information to allow a rich rendering of search results. For instance, in a photographic web site, I'd like the search results to show a thumbnail of each matched image. I can't do this without access to the node object. The attached one-line patch provides access to the node object in the search results. Eric Scouten -- View: http://drupal.org/node/16056 Edit: http://drupal.org/project/comments/add/16056

3 6

[drupal-devel] [feature] Nice DB error screens
by drumm 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/27231 Post a follow up: http://drupal.org/project/comments/add/27231 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: drumm Reported by: drumm Updated by: drumm Status: patch Attachment: http://drupal.org/files/issues/errorhandling_1.patch (8.75 KB) Updated patch using theme_placeholder() where appropriate. drumm Previous comments: ------------------------------------------------------------------------ Thu, 21 Jul 2005 22:43:16 +0000 : drumm Attachment: http://drupal.org/files/issues/screenshot2.png (96.1 KB) Screenshot. ------------------------------------------------------------------------ Thu, 21 Jul 2005 23:21:31 +0000 : drumm Attachment: http://drupal.org/files/issues/screenshot_10.png (88.44 KB) ------------------------------------------------------------------------ Thu, 21 Jul 2005 23:25:57 +0000 : drumm Attachment: http://drupal.org/files/issues/druplicon.png (3.98 KB) New Druplicon which will need to replace misc/druplicon.png. The current file in that location is a giant flat druplicon which isn't used anywhere. ------------------------------------------------------------------------ Thu, 21 Jul 2005 23:26:26 +0000 : drumm Attachment: http://drupal.org/files/issues/maintenance.css (329 bytes) Some CSS ------------------------------------------------------------------------ Thu, 21 Jul 2005 23:30:34 +0000 : drumm Attachment: http://drupal.org/files/issues/errorhandling.patch (8.47 KB) This patch makes the database errors which occur when the database connection information is wrong or the database isn't up much nicer and more explanatory. To do this it adds a themeable function, theme_maintenance_page(), and a way to get Drupal running without a theme but the ability to execute themeable functions in the usual way. This means we can use the same code to give upgrade.php a nicer look in the future. ------------------------------------------------------------------------ Fri, 22 Jul 2005 06:33:57 +0000 : kbahey I was about to do something similar on a less ambitious scale (mainly the connect function), because what we get now is ugly and looks unprofessional. +1 from me for this. Great job! ------------------------------------------------------------------------ Fri, 22 Jul 2005 06:52:30 +0000 : gordon +1, This looks great. Get this in and then we can do something with the upgrade.php system and it will look just great. ------------------------------------------------------------------------ Fri, 22 Jul 2005 19:36:39 +0000 : drumm Attachment: http://drupal.org/files/issues/errorhandling_0.patch (8.44 KB) New patch with a couple suggestions from chx. * $foo === NULL is faster than is_null($foo) * Removed 'bootstrap' from the function name since it is not really part of the bootstrap system. When testing/applying updates #2 and #3 of this issue need to be handled too. ------------------------------------------------------------------------ Fri, 22 Jul 2005 20:47:18 +0000 : stefan nagtegaal I indeed like the patch! you have some perfectly helptexts which asure that people *must* be foulish when they do not know how to setup drupal the correct way. On that, a very, very fine patch.. Though, code-wise I have one nit-pick, which is propably not even a real issue. Only, I think it looks better: + print theme('maintenance_page', 'We were able to connect to the MySQL database server (which means your username and password is okay) but not able to select the database. +The MySQL error was: '. mysql_error() .'. +Currently, the database is <code>'. substr($url['path'], 1) .'. The username is '. $url['user'] .' and the database server is '. $url['host'] .'. + + * Are you sure you have the correct database name? + * Are you sure the database exists? + * Are you sure the username has permission to access the database? + +For more help, see the Installation and upgrading handbook [1]. If you are unsure what these terms mean you should probably contact your hosting provider. '); Wouldn't it be nicer (code-wise) todo: $message[] = 'We were able to connect to the MySQL database server (which means your username and password is okay) but not able to select the database.'; $message[] = strtr('The MySQL error was: %error.', array('%error' => theme('placeholder', mysql_error()))); $message[] = etc, etc; print theme('maintenance_page', $message); Also would I encourage you to make a function theme_message($type, $message), like: function theme_message($type, $message) { switch($type) { case 'code': $output .= '<code>'. $message .'<code>'; break; case 'placeholder': $output .= ''. $message .''; break; case 'important': $output .= ''. $message .''; break; default: $output .= ''. $message .''; break; } } Which makes theme_placeholder() redundant and could be removed. Codesnippets could be handled in a more consistent way then we currently do (having a theme_placeholder() only is rediculous imo afterall). Also this function could be used to remove the hardcoded HTML from the helptexts.. The code is untested and needs some love here-and-there. But, when it's done, I think we will create (codewise) more consistency than we currently have! I look forward to see your idea's and hear your opinion about this.. [1] http://drupal.org/node/258 ------------------------------------------------------------------------ Fri, 22 Jul 2005 23:27:53 +0000 : drumm The theme_message() which stefan nagtegaal looks like it should be four separate themeable functions, one of which already exists. I think any such modifications are outside the scope of this patch. I am nearly finished preparing a patch to update.php which uses this patch, but I can not post it until this patch is resolved. ------------------------------------------------------------------------ Sat, 23 Jul 2005 09:35:03 +0000 : stefan nagtegaal Okay, I think I have to agree with Neill in here.. If this patch hits the trunk, I'll make a new issue and rolls a patch for introducing new functions which should improve code-wise consistency.. Let me introduce you to: function theme_placeholder($message) { return "$message"; } function theme_important($message) { return "$message"; } function theme_code($message) { return "<code>$message"; } function theme_text($message) { return "$message "; } Patches are soon to come in another issue, after this has hit the trunk..

1 0

[drupal-devel] [feature] Change check_output to check_markup
by mathias 26 Jul '05

26 Jul '05

Issue status update for http://drupal.org/node/27551 Post a follow up: http://drupal.org/project/comments/add/27551 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: mathias Reported by: mathias Updated by: mathias Status: patch Attachment: http://drupal.org/files/issues/check_markup.patch (4.95 KB) We have two functions for cleaning user submitted data: check_plain() for data with no markup and check_output() for HTML. For the sake of consistency, let's rename check_output to check_markup. Contrib modules affected: admnotify/admnotify.module atom/atom.module bookreview/bookreview.module discography/discography.module feature/feature.module filestore2/filestore2.module filtercache/README.txt filtercache/filtercache-drupal44.patch filtercache/filtercache.module flexinode/field_textarea.inc groups/groups.module image/image.module img_assist/CHANGELOG.txt listhandler/listhandler.module mail_archive/mail_archive.module menu_otf/CHANGELOG.txt naggregator/naggregator.module node_aggregator/feed.module node_aggregator/item.module nodewords/nodewords.module notify/notify.inc og/og.module privatemsg/privatemsg.module project/comment.inc project/mail.inc project/project.module quotes/quotes.module recipe/recipe.module shoutbox/shoutbox.module sidebar/sidebar.module sidecontent/sidecontent.module taxonomy_html/taxonomy_html.module tec/tec.module trackback/trackback.module trip_search/trip_search.module wallpaper/wallpaper.module webform/webform.module moviereview/moviereview.module question/question.module quiz/multichoice.module mathias

2 1

[drupal-devel] [task] node preview teaser message should use drupal set message
by Bèr Kessels 26 Jul '05

26 Jul '05

Issue status update for http://drupal.org/node/27535 Post a follow up: http://drupal.org/project/comments/add/27535 Project: Drupal Version: cvs Component: node system Category: tasks Priority: normal Assigned to: Bèr Kessels Reported by: Bèr Kessels Updated by: Bèr Kessels Status: patch Attachment: http://drupal.org/files/issues/node_uses_message.patch (1.25 KB) This small patch moves the 'teaser help' to a drupal_set_message. This is more consistent, looks much better and is much beter themeable. After all this is wexactly what drpal_set_messag is meant for. Bèr Kessels

3 2

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

26 Jul '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 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. 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????

1 0

[drupal-devel] Multiple sortable tables on one page?
by Evan Heidtmann 26 Jul '05

26 Jul '05

The pager API provides a nice way to have multiple page-able tables on one page: the $element argument to theme('pager') and pager_query(). But as far as I can tell, there is no way to do the same with the tablesort API. Am I mistaken? If not, should I write a patch? Thanks, Evan P.S. If this is the wrong place for this question, please forgive me and tell me where the correct place is.

2 2

[drupal-devel] Tight release cycles
by Karoly Negyesi 26 Jul '05

26 Jul '05

Hi! Let's start making a calendar for next year's releases now and stick to it. My proposal: 2005 dec 26 code freeze (or Jan 2, 2006) 2006 feb 6 Drupal 4.8 2006 may 1 code freeze 2006 june 5 Drupal 4.9 2006 aug 28 code freeze 2006 oct 2 Drupal 5.0 2007 jan 1 code freeze 2007 feb 5 Drupal 5.1 ... We always have five weeks for bug fixing and three months of features. Let's give active support for actual release and security fixes for one version behind and two versions back is simply not supported. Regards NK

11 18

[drupal-devel] [bug] Tablesort and $_REQUEST array handling
by mathias 26 Jul '05

26 Jul '05

Issue status update for http://drupal.org/node/5371 Post a follow up: http://drupal.org/project/comments/add/5371 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: mathias Reported by: mathias Updated by: mathias Status: patch Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_5.patch (3.58 KB) Allow a string, NULL or array to be passed into array2uri(), making life easier on developers per Steven's feedback on IRC. mathias Previous comments: ------------------------------------------------------------------------ Fri, 23 Jan 2004 16:07:37 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays.patch (652 bytes) Tablesort tries its best to rebuild the URI from which it was called. The problem is when $_REQUEST holds arrays such as $edit. Tablesort will process this as: example.com?edit=Array This can be problematic since many module conditions rely on whether or not the $edit array is set. My patch disallows tablesort to append "array $_REQUEST variables" to it's new query string, since it serves no purpose. ------------------------------------------------------------------------ Sat, 31 Jan 2004 11:00:20 +0000 : Dries I'd like to hear Moshe's take on this as I'm not sure this is the proper fix. I'd think that the tablesort code knows exactly what fields it should add to the URL. ------------------------------------------------------------------------ Sat, 31 Jan 2004 13:24:36 +0000 : moshe weitzman I don't think this is the proper fix either. For example, - browse to the Drupal.org issue search page [1]. - press Search button - On the results page, look at the links in the table header and the pager. The query state gets passed along by tablesort. This patch would break this behavior. Changing Status to 'Active', since this patch shouldn't be applied without modification. [1] http://drupal.org/project/drupal/issues/search ------------------------------------------------------------------------ Sat, 31 Jan 2004 17:01:56 +0000 : mathias I'm pretty sure the project system is hacking the request object just like I am before sending it to tablesort: <?php if ($_SERVER['REQUEST_METHOD'] == 'POST' && is_array($_POST['edit'])) { foreach ($_POST['edit'] as $key => $value) { if (!empty($value) && in_array($key, $fields)) { $query->$key = $value; $_POST[$key] = is_array($value) ? implode(',', $value) : $value; } } unset($_POST['edit'], $_POST['op']); } ?> Basically, we're both moving the edit array into separate name / value pairs and then destroying it before handing it off to tablesort. I don't understand what the benefit is of passing an array into a GET query string since the key/value pairs aren't retained. We could set an option in tablesort to ask it to decompose the array into it's key/value pairs for us and pass them back in the query string. For example an edit array such as: <?php $edit['a'] => 1 $edit['b'] => 2 would have a query string of: &edit_a=1&edit_b=2 instead of the currently meaningless: &edit=Array ?> ------------------------------------------------------------------------ Sat, 31 Jan 2004 17:12:57 +0000 : Goba You mean: <?php $edit['a'] => 1 $edit['b'] => 2 would have a query string of: &edit[a]=1&edit[b]=2 instead of the currently meaningless: &edit=Array ?> This reuses the wonderful array handling provided by PHP (as long as nested arrays are handled properly). ------------------------------------------------------------------------ Sun, 01 Feb 2004 15:02:25 +0000 : Kjartan Indeed project module hacks the request data. I don't really like this as a general fix as it is a hack, but for now it is the best way. I dont think the original patch will break this as it just ignors arrays(). Project module loops through the array and converts them to normal array string elements; $_POST['edit']['status'] = value becomes $_POST['status'] = value, then I think I unset the edit array to clean up the urls. ------------------------------------------------------------------------ Thu, 05 Feb 2004 13:42:16 +0000 : moshe weitzman After feedback from Matias and Kjartan, I now consider this patch to be desireable ------------------------------------------------------------------------ Thu, 12 Aug 2004 15:10:30 +0000 : killes(a)www.drop.org Patch doesn't apply anymore. ------------------------------------------------------------------------ Fri, 08 Oct 2004 04:14:30 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_0.patch (2.55 KB) This patch resolves the hacks project module and the ecommerce package use to rebuild the $_REQUEST arrays when implementing paged resultsets and/or tablesorting. I know some other developers have ran into this bug as well usually after form submissions. Tablesort will now fully handle multi-dimensional $_REQUEST arrays and convert them to a valid querystring and preserve user-submitted data. ------------------------------------------------------------------------ Fri, 08 Oct 2004 04:34:12 +0000 : moshe weitzman very nicely done ... bonus points for converting pager to use the new array2uri() function ... tablesort's poor handling of arrays frustrated me in a recent client project. this will simplify almost all cases where a form post leads to a sortable "results" table, such as in project.module. ------------------------------------------------------------------------ Fri, 15 Oct 2004 14:01:42 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_1.patch (2.99 KB) Moved the new array2uri function to live with its array2* counterparts in common.inc and cleaned up the function documentation a wee bit. ------------------------------------------------------------------------ Tue, 21 Dec 2004 16:43:33 +0000 : moshe weitzman this is a nice code consolidation IMO ------------------------------------------------------------------------ Wed, 22 Dec 2004 10:50:33 +0000 : Dries This patch adds more code than it removes. That aside, it would be nice if the function's PHPdoc comments were extended. Looking at the patch, it is easy to see how/when this function is being used, but reading the PHPdoc comments it is not. I'd add a simple example and some context information. Also, it is unclear what $current_key is used for without inspecting the function's code. For readability, maybe rename $array to $attributes? If that makes sense, maybe rename 'array2uri' to 'attributes2uri'? Which makes me wonder: how does this stack with the existing drupal_attributes() function? Do we really need the recursive array handling? ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:25:06 +0000 : killes(a)www.drop.org Doesn't apply anymore. ------------------------------------------------------------------------ Tue, 12 Jul 2005 22:49:07 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_2.patch (3.19 KB) Updating this patch since I've been bitten by this issue again. The new array2uri() function really shines for tablesorting and/or paging search results after a $_POST request. The only coding change involved for developers is to use $_REQUEST['edit'] and $_REQUEST['op'] instead of $_POST['edit'] and $_POST['op'] within their function callback where this functionality is desired. As an added benefit these pages could also be bookmarked. Dries, I've updated the documentation to hopefully makes things a little easier to understand. I don't think renaming the $array variable to $attributes or calling the function attributes2uri instead of array2uri would make the function and its purpose more transparent. It's not used for HTML attributes like the drupal_attributes function, but rather for form submitted data (e.g., the $edit array) that needs to be tablesorted/paged. Project module and the ecommerce package still currently hack the $_POST vars to circumvent this problem. Not sure if other modules are also doing this. ------------------------------------------------------------------------ Fri, 15 Jul 2005 16:25:46 +0000 : Steven If we're going to have an array2uri() function, we might as well add urlencode() calls in there... this would centralize a lot of existing urlencode() calls, I think. ------------------------------------------------------------------------ Mon, 18 Jul 2005 13:51:56 +0000 : Dries Not going to commit this yet. 1. Let's wait for Mathias' feedback on Steven's comment. 2. I suggest using query or query_string, not querystring. 3. It is not clear why this can't be part of drupal_attributes($array)? If there is an important difference, it would be nice to make that clear in the PHPdoc comments. ------------------------------------------------------------------------ Fri, 22 Jul 2005 03:18:19 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_3.patch (3.23 KB) Agreed with Steven that this is a logical place to urlencode values. The patch has been updated accordingly. There was some IRC discussion as to whether this function should be called drupal_uri, drupal2uri or just plain ol' array2uri. I think since this code doesn't make any Drupal specific function calls, array2uri is the best name. Which brings up a whole new issue for a whole new thread: Perhaps drupal_attributes should be renamed array2attributes? ------------------------------------------------------------------------ Mon, 25 Jul 2005 08:50:00 +0000 : Steven Dries: drupal_attributes is used for outputting attributes of an HTML tag. This patch is about putting data into the query (GET) part of an URL. They are completely different. mathias: couldn't we use this function in drupal_get_destination() as well ? ------------------------------------------------------------------------ Mon, 25 Jul 2005 08:56:05 +0000 : Steven By the way, what exactly is the point of this check: + if (!strstr($query_string, $key_as_uri)) { + $query_string .= '&'. $key_as_uri. '='. urlencode($value); + } It seems to be there to avoid duplicates, but there is no similar check below. If it is required, it needs a comment to explain it. ------------------------------------------------------------------------ Tue, 26 Jul 2005 17:02:50 +0000 : mathias Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_4.patch (3.39 KB) drupal_get_destination() now uses array2uri(). I discovered that the use of strstr(), was to compensate for the function calling itself at times when it didn't need to. This bug is fixed and strstr() is removed. Good catch!

1 0