September 2005 - Development - lists.drupal.org

[drupal-devel] [bug] Can't post comments when using PHP5
by michaelsb 04 Sep '05

04 Sep '05

Issue status update for http://drupal.org/node/30354 Post a follow up: http://drupal.org/project/comments/add/30354 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: michaelsb Updated by: michaelsb Status: patch (code needs review) Attachment: http://drupal.org/files/issues/comment_15.patch (800 bytes) I get the below warning and form error when trying to add comments to any node (story, page, etc.) on a PHP 5.04 installation (Apache2, Debian Sarge). PHP warning: "Warning: strtotime() [function.strtotime]: Called with an empty time parameter. in /home/msb015/projects/drupal/modules/comment.module on line 448." Form error: "You have to specify a valid date." PHP5 backtrace: #0 comment_validate(Array ([subject] => test,[comment] => test,[format] => 1,[cid] => ,[pid] => 0,[nid] => 5,[uid] => )) called at [/home/msb015/projects/drupal/modules/comment.module:396] #1 comment_reply(5) called at [/home/msb015/projects/drupal/includes/menu.inc:354] #2 call_user_func_array(comment_reply, Array ([0] => 5)) called at [/home/msb015/projects/drupal/includes/menu.inc:354] #3 menu_execute_active_handler() called at [/home/msb015/projects/drupal/index.php:15] As you can tell, there isn't a 'date' key in $edit (what's passed to comment_validate()). After some debugging with the help of killes, we managed to fix the problem with the attached patch. The patch passes 'now' to strtotime() if edit['date'] didn't exist. michaelsb

1 0

[drupal-devel] [bug] use upload_tmp_dir as default for FILE_DIRECTORY_TEMP
by crunchywelch 03 Sep '05

03 Sep '05

Issue status update for http://drupal.org/node/26249 Post a follow up: http://drupal.org/project/comments/add/26249 Project: Drupal Version: 4.6.2 Component: base system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: danielc Updated by: crunchywelch -Status: active +Status: patch (code needs review) change status crunchywelch Previous comments: ------------------------------------------------------------------------ Sat, 02 Jul 2005 20:03:13 +0000 : danielc In includes/file.inc, the FILE_DIRECTORY_TEMP constant is set as follows: define('FILE_DIRECTORY_TEMP', IS_WINDOWS ? 'c:\\windows\\temp' : '/tmp'); It's wiser to set it to php.ini's upload_tmp_dir value: define('FILE_DIRECTORY_TEMP', ini_get('upload_tmp_dir')); I noticed this issue because I'm running Windows 2000, which defaults to an install directory of c:\winnt, not c:\windows. When I go to the "Settings" administration page, (while running PHP 5.0.5-dev) I got the following error at the top of the page: warning: mkdir() [function.mkdir]: No such file or directory in ...\includes\file.inc on line 86. ------------------------------------------------------------------------ Thu, 07 Jul 2005 13:05:36 +0000 : AlberT75 I had the same problem when installed on a host with open_base_dir enabled (it should be on a decent hoster machine)... ------------------------------------------------------------------------ Sat, 03 Sep 2005 21:42:49 +0000 : crunchywelch Attachment: http://drupal.org/files/issues/file.inc.patch (635 bytes) Here is a patch for this, seems like a good idea to me, works on a new HEAD install on SUSe Linux.

1 0

[drupal-devel] [bug] Users with 'maintain books' or 'edit own book pages' permission cannot delete book pages
by clydefrog 03 Sep '05

03 Sep '05

Issue status update for http://drupal.org/node/21559 Post a follow up: http://drupal.org/project/comments/add/21559 Project: Drupal Version: cvs Component: book.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: clydefrog Updated by: clydefrog Status: patch (code needs review) Your site behaved in that way because the users with "maintain books" permission did not have permission to use the input format defined for those nodes. This is a general feature of Drupal, not specific to the book module. clydefrog Previous comments: ------------------------------------------------------------------------ Fri, 29 Apr 2005 04:32:19 +0000 : clydefrog Attachment: http://drupal.org/files/issues/delete_own_book_pages.patch (626 bytes) AFAICT, only 'administer nodes' permission allows deletion of book pages. This patch allows users with 'maintain books' or 'edit own book pages' permission to delete book pages. The logic doesn't check if the node has updates pending. Should it? ------------------------------------------------------------------------ Fri, 29 Apr 2005 14:10:22 +0000 : clydefrog It's a patch, duh. ------------------------------------------------------------------------ Fri, 29 Apr 2005 14:21:27 +0000 : rivena If maintain books is a permission that overlaps other permissions, it should say so. For example, /maintain books (create, edit, and delete all book pages)/. While I agree that someone who has been given maintainer status should be able to delete pages, I wonder if 'delete own pages' should be made into a seperate permission. I could see a situation where I would not want someone to be able to delete their own pages. Do these permissions affect things that are not book pages but have been put into books? Anisa. It's /Golden Week/! ------------------------------------------------------------------------ Sat, 30 Apr 2005 03:43:17 +0000 : clydefrog I think it's clear that "maintain books" implies the ability to edit and delete book pages. No other node module provides a "delete own " permission, so it would be inconsistent for book.module to provide it. Can you provide some examples to justify the inconsistency? These permissions do not affect nodes which aren't book pages but are in a book. Those nodes are controlled by their respective type-specific permissions, but only users with "maintain books" permission can add or remove those nodes from books. I believe this is the correct behavior. ------------------------------------------------------------------------ Sat, 30 Apr 2005 11:03:47 +0000 : rivena Hmm... But, you just said that until this patch, it *didn't* include the delete pages permission. Right? So, now you are changing it's behavior, but leaving the wording the same. How will people know? Or does it not matter, because you presume they didn't really know it didn't include delete in the first place? I don't know that I can defend my idea of having a seperate delete own pages permission against a standard of consistency. If all the other edit own permissions include the delete pages permission, then I have no quibbles. :) Except it would be more userfriendly if I knew what the permissions meant before I granted them to someone, without having to look at the code. But I do realize that is completely out of the scope of the patch. :) Where do all these little changes *go*, anyway? The changelog just says things like... refractored the search module. Anisa. ------------------------------------------------------------------------ Sat, 30 Apr 2005 16:04:14 +0000 : clydefrog I believe that "maintain books" permission implies the ability to delete book pages, so this patch fixes that bug. Other node modules include deletion in the "edit own" permission. For example, page.module: <?php if ($op == 'update' || $op == 'delete') { if (user_access('edit own pages') && ($user->uid == $node->uid)) { return TRUE; } } ?> I agree that it would be better if permissions were clearer about what they implied. There was a discussion started by killes [1] a while ago: "One problem with Drupal is the way it handles user permissions. The permissions are attached to the user object but nobody really knows what they do. To find out what a particular permission allows you to do, you often need to have a look at the code. " I don't know if it ever lead to any code. I think the changelog is meant to list the changes since the previous version without too much technical detail. If you want to see every last change, take a look at the cvs messages [2]. [1] http://lists.drupal.org/archives/drupal-devel/2005-04/msg00704.html [2] http://drupal.org/project/cvs/3060 ------------------------------------------------------------------------ Mon, 23 May 2005 17:33:18 +0000 : rivena I've been looking at the book module permissions recently, and did a bit of testing. I wanted to know what was the minimum permissions needed to create a new book. I assumed based on this discussion, and a similar discussion on the drupal support list that this was 'maintain books' and 'administer nodes'. I ran into a problem, then added 'access nodes' to this. So now my user has no other permissions other than 'maintain books', 'administer nodes' and 'access nodes'. You'd think this is more than sufficient to do something simple like create a new book, but in fact, with just these permissions, you do not have access to create a book page. You can edit other pages, and move them into a new book. Once you have create pages permission, you appear to finally have all the permissions needed, edit own pages is not needed. So, since, the point of this patch was that not having delete permission was odd, might I suggest adding create permission to maintain books? ;p Anisa. ------------------------------------------------------------------------ Mon, 08 Aug 2005 10:37:49 +0000 : Bèr Kessels I would say this is a "by design". Maybe we need to rename the two book permissions into: "create books" and "edit own book pages". None of the other nodes can be deleted by the roles with "edit own Foo" permissions, nor should the books. ------------------------------------------------------------------------ Mon, 08 Aug 2005 16:59:30 +0000 : clydefrog Thanks for your review, Ber. story.module [3] allows users with "edit own stories" permission to delete stories. <?php if ($op == 'update' || $op == 'delete') { if (user_access('edit own stories') && ($user->uid == $node->uid)) { return TRUE; } } ?> Therefore it is not inconsistent for the same behavior to apply to book nodes. If "maintain books" really means "create books", then the name should be changed and existing code in book.module needs to be updated. However, I think "maintain books" should allow more than just that. In my situation, I want to be able to give someone the permissions to create books and to create, edit, and delete all book pages. With my patch, "maintain books" and "create book pages" does that. [3] http://cvs.drupal.org/viewcvs/drupal/drupal/modules/story.module?rev=1.167&… ------------------------------------------------------------------------ Mon, 08 Aug 2005 19:16:35 +0000 : Bèr Kessels What i see from story, that is specific to story.module too. We should really settle for a consistent behaviour for *all* nodes, one that is "enforced" from node.module. These things should really not be decided per node-type and per-module. Thats a recipe for a mess. ------------------------------------------------------------------------ Mon, 08 Aug 2005 19:25:27 +0000 : Eric Scouten I agree that the current system is a mess. I disagree that there should be one consistent policy enforced by node.module. Sites may have quite legitimate reasons to have different policies for different node types. ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:35:59 +0000 : Bèr Kessels What i was aiming at is to let node module take care of the following: * edit $node_type * administer $node_type * delete $node_type (but would that not better be in 'administer';: users who can administer can delete) * create $node_type That way the modules need not cre about this. ut they /can/ take care of it, if they wish, and add more rules and permissions. ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:36:32 +0000 : Bèr Kessels oh, and -forgot- : * view $node_type ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:49:31 +0000 : Eric Scouten Thanks for the clarification, Bèr. I was afraid that you meant that there would be a single policy applied to *all* node types, not a policy *per* node type. Now I think we're in complete agreement on this subject. ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:51:13 +0000 : puregin I think it's important to keep a clean separation between content and structure (e.g., what's inside of the node, versus how the node is associated with other nodes) I think that there will likely be other explicit structures in Drupal over time, so these concerns affect more than just book module. Deleting a node from a book is potentially a problem, since this operation affects the structure of a book. We also need to think about other interactions with structure - for example, what happens if someone unpublished a node in a book? Should all children also be unpublished? Should the node contents be hidden, while children remain available in navigation? ------------------------------------------------------------------------ Mon, 08 Aug 2005 21:08:01 +0000 : Bèr Kessels puregrin. I think you are gettnig OT here, Thought it is a very valid (and problematic) issue, It really belongs in a new thread. ------------------------------------------------------------------------ Mon, 08 Aug 2005 22:57:34 +0000 : puregin What I was saying is that whether a user should be able to delete a node from a structure should really be enforced by the module which is responsible for the structure. For example, imagining a simple linear list structure - let's say for a "wish list" - composed of nodes owned by a bunch of different users. In this example, a user could delete his own node with no problem. For the book module, a users node may have children authored by other users. Letting an arbitrary user delete his own node would be, I think, a mistake, since the children would then be orphaned. I suppose one could imagine other modules which rely on a hierarchy, which might have a different policy (e.g., automatically attach children to the parent of the deleted node, if such a node exists). Perhaps eventually two 'tree-based' structure modules might inherit from a tree structure module, each enforcing separate policies regarding node operations. The point is, that one should not have to modify node module when a new structure is defined. Node module should not have to know about how composites of nodes work. ------------------------------------------------------------------------ Tue, 09 Aug 2005 00:45:07 +0000 : Boris Mann puregin: I was talking about this today, so I created a task [4] to remind us to build a generalized node relationship module/schema. [4] http://drupal.org/node/28480 ------------------------------------------------------------------------ Fri, 12 Aug 2005 00:42:58 +0000 : Crell /I think it's clear that "maintain books" implies the ability to edit and delete book pages./ I'm afraid I have to disagree with clydefrog on this one. The words "administer" and "maintain" and "manage" don't give any firm indication of what they mean, in and of themselves. They could vary depending on the context. Some nodes are deleteable in the fist place, some aren't. In that case, "maintain" means two different things, as would anything else you put in there unqualified. This isn't just a problem with the books module, but in general. It is not intuitively obvious what "administer nodes" or "administer X" means. That's a problem, especially when, as I said, it can vary from one node type to another. I see, generally, the following types of operations on nodes: - Create - Change Ownership - View Anyone's - View Own - Edit Anyone's - Edit Own - Delete Anyone's - Delete Own Which of those is covered by "administer" or "maintain"? And what if I want to allow someone to, say, edit their own nodes, but not delete them? Or allow a given class of administrator to edit anyone's nodes but not delete any but their own? I'd actually be much in favor of more fine-grained control over nodes access rights, both for the greater power it offers and for the clearer UI, system-wide. Whether that's best done via a central node API system or via a "recommended permission breakdown" I don't know, and will leave that to those who have spent more time with core than I have. :-) I do believe it's a good idea, however, both for greater flexibility and greater learnability. ------------------------------------------------------------------------ Fri, 26 Aug 2005 21:23:27 +0000 : noid Just to clarify things -- based on tests I did with the book module -- (the following hold true for regular users with no "administer nodes" permission) *1) the "edit own book pages" does not give the user the option to delete his own pages.* and *2) "maintain books" doesn't do anything. * This is the state of things right now, right? If this is so, may I suggest that the initial fix be simple in the meantime which should include: *1) "edit own book pages" means the user /can also delete his own pages./* *2) "maintain books" means the user /can edit (but not delete) pages created by other users./* ******************** I've also noticed that "create book pages" (or "edit own book pages" and "maintain books" for that matter) doesn't allow the user to create a top-level page -- there's no option for it in the pulldown. Can this be fixed so that * "create book pages" allow the user to create a top-level page (that is start his own book)?* ------------------------------------------------------------------------ Tue, 30 Aug 2005 14:14:42 +0000 : noid My bad. *"Maintain books" works after all.* Tested the permission again and found out that a user with the aforementioned access can edit some of the nodes after all, and these nodes were created after a certain date. So I looked into the modules I installed after that date, and, to cut to the chase, found out that the culprit was TinyMCE. Well, indirectly, that is. :) Turned out I was using a new customized input format -- per advice of TinyMCE install instructions -- and the older nodes that were created with the default filtered HTML format couldn't be edited. So all I have to do now is change the input format of these older nodes to the new input format I set for TinyMCE. :) At any rate, perhaps someone can figure out why the new format made the book module act that way. :)

1 0

[drupal-devel] [bug] After creating a book, empty book divs shouldn't be created
by crunchywelch 03 Sep '05

03 Sep '05

Issue status update for http://drupal.org/node/13099 Post a follow up: http://drupal.org/project/comments/add/13099 Project: Drupal Version: cvs Component: book.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: FactoryJoe(a)civicspacelabs.org Updated by: crunchywelch Status: patch (code needs review) Attachment: http://drupal.org/files/issues/book.module_8.patch (3.06 KB) Seems I tried to sneak in my timezone system.module patch as well ;). Here is a patch without the system.module diff. crunchywelch Previous comments: ------------------------------------------------------------------------ Thu, 18 Nov 2004 02:09:07 +0000 : FactoryJoe(a)civicspacelabs.org Attachment: http://drupal.org/files/issues/book_divs.png (9.83 KB) This code creates a styled section beneath a new book: <div class="book"><div class="nav"> <div class="links"><div class="prev"> </div><div class="next"> </div></div> <div class="titles"></div></div></div> </div> If there is no data to be displayed, empty divs should not be output. ------------------------------------------------------------------------ Thu, 16 Jun 2005 18:26:57 +0000 : Robin Monks The <div class="book"><div class="nav"> <div class="links"><div class="prev"> </div><div class="next"> </div></div> <div class="titles"></div></div></div></div> still exists. Bumping up to HEAD. Robin ------------------------------------------------------------------------ Sat, 03 Sep 2005 21:28:48 +0000 : crunchywelch Attachment: http://drupal.org/files/issues/book.module_7.patch (7.89 KB) Here is a patch that fixes this, it rearranges the theme_book_navigation function logic to only output the navigation book div if there is content to display.

1 0

[drupal-devel] [bug] After creating a book, empty book divs shouldn't be created
by crunchywelch 03 Sep '05

03 Sep '05

Issue status update for http://drupal.org/node/13099 Post a follow up: http://drupal.org/project/comments/add/13099 Project: Drupal Version: cvs Component: book.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: FactoryJoe(a)civicspacelabs.org Updated by: crunchywelch -Status: active +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/book.module_7.patch (7.89 KB) Here is a patch that fixes this, it rearranges the theme_book_navigation function logic to only output the navigation book div if there is content to display. crunchywelch Previous comments: ------------------------------------------------------------------------ Thu, 18 Nov 2004 02:09:07 +0000 : FactoryJoe(a)civicspacelabs.org Attachment: http://drupal.org/files/issues/book_divs.png (9.83 KB) This code creates a styled section beneath a new book: <div class="book"><div class="nav"> <div class="links"><div class="prev"> </div><div class="next"> </div></div> <div class="titles"></div></div></div> </div> If there is no data to be displayed, empty divs should not be output. ------------------------------------------------------------------------ Thu, 16 Jun 2005 18:26:57 +0000 : Robin Monks The <div class="book"><div class="nav"> <div class="links"><div class="prev"> </div><div class="next"> </div></div> <div class="titles"></div></div></div></div> still exists. Bumping up to HEAD. Robin

1 0

[drupal-devel] [bug] Can't delete subjects with '&' in title
by Cvbge 03 Sep '05

03 Sep '05

Issue status update for http://drupal.org/node/27140 Post a follow up: http://drupal.org/project/comments/add/27140 Project: Drupal Version: cvs Component: contact.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: Cvbge -Status: active +Status: patch (code needs work) Hi, 0. You should keep all changes in one file, i.e. pake one patch, not patch for every file 1. You should include changes for database/database.{mysql,pgsql} 2. IIRC auto_increment was depreciated, you should use db_next_id() (but I see auto_increment in cvs...) 3. You use "WHERE cid = '%s'", but cid is integer so this should probably be %d (although this probably would not break anything..) Cvbge Previous comments: ------------------------------------------------------------------------ Wed, 20 Jul 2005 14:57:03 +0000 : m3avrck Using the site-wide contact forms, any subjects with an '&' cannot be deleted. Clicking delete for these subjects brings up the expected action flow, however, confirming the delete does nothing. Subject still exists. ------------------------------------------------------------------------ Mon, 25 Jul 2005 13:45:38 +0000 : m3avrck It appears this issue might be linked with this one: http://drupal.org/node/23685 A problem with mod_rewrite and not the actual contact module. ------------------------------------------------------------------------ Wed, 31 Aug 2005 16:51:56 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact.module_6.patch (4.7 KB) Redid how contacts work. It now references contacts by ID instead of their category as the unique identifier. It also allows for contacts to have weights, so that one may control where the contact will appear in the listing. ------------------------------------------------------------------------ Wed, 31 Aug 2005 16:53:01 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/updates.inc (27.23 KB) This is the update.inc for the database cahnges that goes along with this patch. ------------------------------------------------------------------------ Wed, 31 Aug 2005 18:31:37 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact.module_7.patch (4.5 KB) Updated patch. ------------------------------------------------------------------------ Wed, 31 Aug 2005 18:31:55 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/updates.inc_4.patch (1.29 KB) Updated database patch.

1 0

[drupal-devel] [bug] Session system - fix for NULL uid
by Cvbge 03 Sep '05

03 Sep '05

Issue status update for http://drupal.org/node/29143 Post a follow up: http://drupal.org/project/comments/add/29143 Project: Drupal Version: 4.5.5 Component: other Category: bug reports Priority: critical Assigned to: Anonymous Reported by: Cvbge Updated by: Cvbge Status: patch (code needs review) Attachment: http://drupal.org/files/issues/session-uid.inc (955 bytes) On fresh install session table is empty and session.inc tries to insert a row. But it ommits uid field which is NOT NULL and inserting fails. Attached patch makes it to have the same behaviour as 4.6 Cvbge

3 5

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

03 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 (ready to be committed) 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) 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.

1 0

[drupal-devel] [feature] add inline functionaliy to uploads
by moshe weitzman 03 Sep '05

03 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: moshe weitzman -Status: patch (code needs review) +Status: patch (ready to be committed) 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. moshe weitzman 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.

1 0

[drupal-devel] [bug] Remove code containing user access controls from profile theme functions
by robertDouglass 03 Sep '05

03 Sep '05

Issue status update for http://drupal.org/node/27949 Post a follow up: http://drupal.org/project/comments/add/27949 Project: Drupal Version: cvs Component: profile.module Category: bug reports Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: robertDouglass Status: patch (code needs review) Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme.txt (2.36 KB) The two theme functions in profile.module both violate good theming practice by running user control logic in the middle of them. Worse yet, this isn't immediately visible since it happens in yet another function. Thus themers overriding these functions to style profile pages[1] inadvertently break access control, thus leading to the misperception that overriding theme functions is inherently dangerous[2]. [1] http://drupal.org/node/16011 [2] http://drupal.org/node/16821 robertDouglass

6 11