February 2005 - Development - lists.drupal.org

[drupal-devel] [feature] Improve functionality of block generation for book module
by Steven 05 Feb '05

05 Feb '05

Project: Drupal Version: cvs Component: book.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: nysus Updated by: Steven Status: patch Doesn't this patch still suffer from the inability to show the menu block only on the relevant book(s)? I know we can now restrict blocks per node type, but that still doesn't help with multiple books. By the way, big code style violation: $menu_block_settings[$menu_block->path][weight] "weight" should be surrounded by quotes. Right now it is being treated as an undefined constant. This is not a good idea. Other than that there's a bunch of missing spaces... if the code looks cramped, it usually violates the coding style. + $menu_block_settings[$result->path] = array('status'=>$result->status, 'weight'=>$result->weight, 'region'=>$result->region, 'throttle'=>$result->throttle, 'visibility'=>$result->visibility, 'pages'=>$result->pages, 'types'=>$result->types); + db_query('DELETE FROM {menu} WHERE type=%d OR type=%d', MENU_MODULE_MENU | MENU_MODIFIED_BY_MODULE, MENU_MODULE_ITEM | MENU_MODIFIED_BY_MODULE); //restore menu_module_menu block settings There are several more examples. Please pay attention to this. It's incredibly annoying to have to fix it later, and it makes the code harder to read as you cannot make certain assumptions about how it is layed out. Steven Previous comments: ------------------------------------------------------------------------ December 9, 2004 - 07:12 : nysus Attachment: http://drupal.org/files/issues/book_19.patch (4.85 KB) Attached is a patch for the book module that does the following: 1) Allows book blocks to appear on any page at any time, not just when a node from the book is being viewed. 2) Allows multiple book blocks to appear on the same page. This functionality is achieved by the automatic creation of individual blocks for each book when the book is created. Simply enable the book's block to enjoy the benefits of 1 & 2 above. If the blocks are not enabled, the blocks will appear only when a node from that block is being viewed (the same way it works now). ------------------------------------------------------------------------ December 9, 2004 - 13:35 : andremolnar Attachment: http://drupal.org/files/issues/book_19_1.patch (4.84 KB) +1 This is great. A good many people have asked for something like this, and I think its a nice solution. But in the end this isn't up to me. One minor error in the patch... + $result = db_query('SELECT n.title, b.block, b.nid FROM {book} b INNER JOIN {node} n on n.nid = b.nid WHERE b.parent = 0'); b.block is not a valid field in this query. this updated patch removes reference to it. andre ------------------------------------------------------------------------ December 9, 2004 - 14:54 : nysus Glad you like it and thanks for fixing that up. It was left over from an older version of my patch. ------------------------------------------------------------------------ December 9, 2004 - 14:54 : andremolnar Steve, If I apply this patch, and I attempt to configure one of the newly created blocks. I noticed that for some reason the block.module is returning "true" at line 249 (of the block module)- and is creating a form for module-specific configuration. But all that shows up on the screen is the word "array". Can you trace this back to see why - and maybe update the patch. I can continue to test your changes - anything to help this patch make it into core. andre ------------------------------------------------------------------------ December 9, 2004 - 16:29 : nysus Hmmm...probably because I tested my patch on my version of Drupal, version 4.5.1. I'm having no problems. Are you using a cvs version of Drupal to test. If so, I'll set up cvs on my site and track this down. ------------------------------------------------------------------------ December 9, 2004 - 19:53 : drumm See http://drupal.org/node/12347 for information on how the block system has been updated. When I saw that the elseif ($op == 'view') was taken out I knew immedaitely that there was something weird about this patch. ------------------------------------------------------------------------ December 9, 2004 - 19:58 : nysus OK, thanks for the tip. Sorry for the confusion. Still kind of new to making open source contributions and it's easy for me to overlook some obvious stuff like this. I'll fix this up when I get a chance. ------------------------------------------------------------------------ December 9, 2004 - 20:06 : Dries Please do, because this being a new feature, it has no chance getting committed to the DRUPAL-4-5 branch. The DRUPAL-4-5 branch is in bugfix-mode. New features go into CVS HEAD. ------------------------------------------------------------------------ December 9, 2004 - 23:32 : Dries There is quite a bit of duplicated code in the patch. Maybe it can be simplified (using a function)? Either way, it is a little weird. I haven't tried the path, and I'm not sure I understood the description. It's somewhat vague. Is the book module exporting multiple blocks that are nearly identical, yet have different display behavior? If so, why not add a simple block configuration setting to the original book block? ------------------------------------------------------------------------ December 10, 2004 - 03:51 : nysus Dries, Yes, there is some code that can be factored out and some general cleaning up that can be done. It was a little tricky to write so I left it kind of ragged around the edges until I'm sure there are no bugs. It has worked very well on 4.5.1 but I obviously need to update it to work with cvs. I'll be on that soon. As far as what it does: 1) For every new book that a user creates, a new block is associated with it. So if you create "Book A", "Book B", "Book C", you will get three new blocks visible on the block administration page called "Book A", "Book B", "Book C". The original "Book Navigation" block is still there, too. The functionality of the "Book Navigation" block is not affected at all by this patch. 2) If Book A's block is enabled, the block containing its menu will appear not only when a node within Book A is viewed, but at all times (unless the user suppresses it on certain pages with the "path" feature). When any node is that is NOT in Book A is viewed, Book A's menu still appears but it is fully collapsed. When a node that DOES belong to Book A is viewed, Book A's Book menu expands accordingly. 3) The user can also enable Book B & C's block, and have their menus appear in a block at all times as well. 4) If none of the book's blocks are enabled by the user, the module will behave just as it did without the patch. That is, when the "Book Navigation" block is enabled, the only time any book menu will appear is when a book node is being viewed. 5) It's important to note (and this was the tricky part to write) that if both the "Book Navigation" block is enabled and "Book A" is enabled, they will play nice with each other and not do nasty things like create the same book menu twice. ------------------------------------------------------------------------ December 10, 2004 - 06:51 : nysus Attachment: http://drupal.org/files/issues/book_20.patch (5.37 KB) Andre, Attached is a new patch that will resolve the problem of the block-specific stuff showing on the block configuration form. Let me know if you spot any other bugs. If it looks good to you, I'll go to work making the code look leaner and prettier. ------------------------------------------------------------------------ December 10, 2004 - 08:26 : nysus Attachment: http://drupal.org/files/issues/book_21.patch (4 KB) Dries, Andre: Here is a new and improved streamlined version of the patch. Have a look if you get a chance. ------------------------------------------------------------------------ December 10, 2004 - 09:11 : nysus Attachment: http://drupal.org/files/issues/book_22.patch (4.15 KB) Found a bug in the last version that would cause the block to jump to a different location. I think this should do it. Everything appear to work well (famous last words). ------------------------------------------------------------------------ December 10, 2004 - 15:49 : andremolnar Steve: bugs appear to be gone, and I didn't run across any other errors. This is functioning exactly as described. everyone: I personally would encourage support for this functionality. Book is a powerful navigation building tool in a site, not only with its ability to move next and back through a hierarchy of pages - but also its ability to build the appropriate navigation block without further user intervention (unlike the admin features in the menu module or taxonomy). The most frequently cited complaint about the book module is its inflexibility when it comes to when and where the block shows up. I also frequently hear requests for the ability to show multiple book blocks at the same time. Up until now the best alternatives suggested required users to do a hack (e.g. build a custom block that calls such and such a hook). Most abandon their request at that point because its over their heads. With this patch all those requests are covered and more. Now all books can automatically have their own block and admins can easily decide when and where each of those individual blocks show up (left right, up down) and coupled with the new configuration features of the block module - its very easy for admins to decide on which individual pages a block will show up. I would be interested what other have to say about this feature. My only reservation (which is minor compared to the benefits of the functionality offered) is that there is no way to turn this functionality off. i.e. The default behaviour is to build individual blocks for each book. If there could be a way to toggle this feature on and off somewhere - it would be perfect. Still, AS IS - this is a major improvement and offers great flexibility to admins and site creators. andre ------------------------------------------------------------------------ December 10, 2004 - 16:04 : Anonymous Andre, Thanks for the feedback on the usefulness of this module. Glad I could pitch in and help. I agree about the inability to turn the feature on/off and I was thinking about that myself. I think it could easily be accomplished by creating a checkbox in the "book navigation" block individual configuration's settings. Call it "Enable individual book blocks." When enabled, the individual book blocks will appear on the block administration menu. One question: Where would the state of this checkbox get saved? Has Drupal moved away from serializing data in the data base? ------------------------------------------------------------------------ December 10, 2004 - 19:36 : Dries I'm afraid that 'Enable individual book blocks.' is not descriptive/clear at all. Are you suggesting a setting to toggle between 'show block on all pages' and 'show block on book pages only'? ------------------------------------------------------------------------ December 10, 2004 - 20:45 : nysus Dries, No. Andre and I suggest a setting within the "Book Navigation" cofiguration page, that would toggle whether or not individual books appear on the list of all blocks on the block administration page. Hence the name 'Enable individual book navigation blocks.' The help text for this checkbox might read something like: "By default, a book's navigation block is visible only when a page from that book is being viewed. Check this box if you want more control over where and when an book's navigation block is visible. You will then be able to control the book's navigation block location and visibility settings on the "admin/block" page." Hope this makes it pretty clear. ------------------------------------------------------------------------ December 10, 2004 - 21:08 : Dries I understand what you are trying to do, but not how you are trying to do it, or how the setting is supposed to work. I guess I'll have to try it when a new patch lands. ------------------------------------------------------------------------ December 11, 2004 - 10:52 : nysus Attachment: http://drupal.org/files/issues/book_23.patch (5.02 KB) Alright, fellas, I'm proud to unveil my crowning achievement in the open source development world (no big deal to most of you guys but pretty good for a hack like me). Thanks for all the input so far. It's been helpful. I've streamlined the heck out of it per Dries suggestion and I've created an option to turn this functionality on an off per Andre's suggestion. Does this look good to you guys? Anything else I have to fix or improve? Thanks. ------------------------------------------------------------------------ December 11, 2004 - 14:02 : Dries I tried the patch. It works great but to me, the 'Give books their own block' settings seems to be redundant. Why not export the current book navigation block, along with an additional block for each book? Looks a lot simpler to me. I think I spotted a bug: orphaned book pages (or possibly book pages that are unpublished) appear to be getting book navigation blocks. ------------------------------------------------------------------------ December 11, 2004 - 18:34 : nysus I'll see if I can fix the bug. Might be tricky. But I don't understand your recommendation to "export the current book navigation block, along with an additional block for each book". Can you expand on this thought? ------------------------------------------------------------------------ December 11, 2004 - 19:06 : nysus Dries, I am unable to duplicate the bug. I have three orphaned pages. I also tried unpublishing some pages. But as far as I can tell, the patch works as expected. ------------------------------------------------------------------------ December 11, 2004 - 20:00 : Dries If you can't reproduce the problem, chances are my node/book table is somewhat fubar. As for the configuration option. I suggest removing it and to always make these new blocks available on the /admin/block configuration screen. ------------------------------------------------------------------------ December 11, 2004 - 20:52 : moshe weitzman I am hoping that we maintain the option to keep the behavior where the appropriate book block only shows up when its book page viewed. this is a nice, tidy arrangement. ------------------------------------------------------------------------ December 11, 2004 - 21:03 : nysus Yes, if you don't enable any of the individual book blocks, the a book's block (i.e. navigation menu) will only appear when a page in a book is viewed. In other words, you have the option to have the book block act like this patch isn't even installed. ------------------------------------------------------------------------ December 11, 2004 - 21:54 : Dries I guess I'll have to try the patch again, because I don't understand why it works like this -- or at least, why it can't be made simpler. ------------------------------------------------------------------------ December 11, 2004 - 22:13 : nysus Can you be more specific? Why it works like what? ------------------------------------------------------------------------ December 11, 2004 - 22:24 : nysus Attachment: http://drupal.org/files/issues/book_24.patch (4.1 KB) This patch reverses a change made in the last patch which required a user to enable individual book block before they could enable any individual book blocks. ------------------------------------------------------------------------ December 11, 2004 - 22:41 : andremolnar As I mentioned earlier I am *fine* with either version of this patch as long as it makes it to core. But, as I said earlier, I clearly think the preference for admins would be to have the option to enable or disable this functionality. BTW - if this does make it in, I would be happy to create a Handbook page that describes the new features - something like "how to build robust site navigation using the book module". (on or after December 17th). andre ------------------------------------------------------------------------ December 12, 2004 - 13:11 : Anonymous I am not at all happy with these features. They are incosistant, confusing and should use exising methods and UIs. May main concern is the incosistancy: it is confusing, will require extra attention with each core code change, adds extra logic to the core, and is not re-useable. so here are my questions: 1) why do we not use the menu system and apis to build and adminster the trees? Saves code, does not add extra UIs, and gives users more power. 2) why do we need that extra showing logic? a book block should not get exeptional if clauses, it should use the existing path setting methods on block admin. extra logic is confusing for administrators (hey, i set the path so that the book-block should show up here, but it does not, why?) we should really not provide extra logic in the block hook, but should rather use default settings in block admin (the book could fill the bookblock sql cells with custom paths, for example) 3) we should avoid extra UIs. We already have far too much, and far too much different ones. Please rather improve the block admin, than add new separate interfaces. 4) why do book blocks need al these expeptions in the first place? If they are so exeptional, we could consider not using blocks, but something else, like in-line book layout (pages with the index etc) 5) why did you not choose for a general, standard, block gerneation API? that way modules, such as taxonomy, image gallery, weblinks, article, etc can reuse it and introduce block gernation. All that sayd, i like the idea of this functionality, but i fear for a great useability downfall if we start introducing all sorts of exeptions for all sorts of modules. Because now chances are very big that taxonomy, image gallery etc will need to introduce other UIs, other code, new methods and new documentation, if they too want some sort of better block handling. so a -1 from me. ------------------------------------------------------------------------ December 12, 2004 - 13:12 : Bèr Kessels ^^--- That was me (bèr kessels) forgot to log in..... ------------------------------------------------------------------------ December 12, 2004 - 13:34 : nysus I'm not going to pretend I can argue if my patch does or does not fit well into Drupal's larger architecture. My motivation for writing it is that I had an immediate need to create an easy way to make it easy for users to create menus. I'll let others decide whether or not the patch has merit from the big picture perspective. But if it doesn't, why not just use it for its immediate benefits and then throw it away when something better comes along? ------------------------------------------------------------------------ December 12, 2004 - 14:01 : Dries My summary is this: +1 on the functionality, -1 on the implementation. The code itself is good, but the usability/integration is not. ------------------------------------------------------------------------ December 12, 2004 - 14:13 : nysus When you say "usability" is that from the user's perspective or the code maintainers? I'm guessing it's the latter but I'm unsure. What about the idea of using the patch until a more permanent solution comes along? Yes, it's much better to live in a home with indoor plumbing but why not use the outhouse while you wait for a toilet to get installed? Or are there other considerations I'm overlooking that would make this a bad idea? ------------------------------------------------------------------------ December 12, 2004 - 14:20 : Goba nysus it is just generally against the Drupal philosophy to add improperly implemented functionality until something better comes along. There even used to be ocassions in Drupal releases, when some functionality was removed (not fixed) for a release, because its implementation was not adequate. ------------------------------------------------------------------------ December 12, 2004 - 14:55 : Dries Usability for the user. The extra setting on the block configuration page is both confusing and awkward. I don't understand why things must be configured/enabled that way (see my and Ber's previous comments on this issue). ------------------------------------------------------------------------ December 12, 2004 - 15:03 : nysus Well, just for the record, I reversed that functionality per your suggestion and uploaded the patch. The indiviudal book blocks now appear automatically. ------------------------------------------------------------------------ December 13, 2004 - 14:56 : Bèr Kessels Hi, I am sure you can make not only simpler, but better usefull for admins and users. All you need to do is use the menus for this. i.e. make a menu entry for each book page. For each book make a menu on level 0, without a parent. that way they become a seoprate menu, each with an own block. it saves code, makes things more consistent, and most important, uses drupal functionality where it should. ------------------------------------------------------------------------ December 13, 2004 - 15:50 : nysus Ber, Are you suggesting that for every single book page that a menu item be created? That's really not practical. That was my main motivation for writing this patch: to make it easy to put links, not necessarily related to one another, into a block. Any more pages than 10 and the sheer tedium of the job would prevent anyone from ever doing that. The menu.module is great, but adding new menu items is far from quick and painless. I just added about 10 to my menu for different taxonomies on my site and it wasn't fun. Plus, if you do as you suggest, there is also the problem of the book showing up twice. It will be generated by the menu and then it will be generated again by the book module which is programmed to design a block. You'd have to put some logic in the book.module _block hook to try to anticipate if a user has enabled a book in the menu. That wouldn't be pretty code. I'm all for putting automatic generation of book navigation blocks as part of the menu module. It does make more sense to have it there. But it forces me to ask the question: "Then why do we currently have code in the book module that generates a menu? Shouldn't that belong in the menu.module, too?" ------------------------------------------------------------------------ December 13, 2004 - 23:31 : killes(a)www.drop.org I think what Ber is trying to say is that you can write a contrib module that monitors the changes to the book table and creates menu items automatically. nodeapi is your friend. I would also prefer this solution. ------------------------------------------------------------------------ December 15, 2004 - 17:39 : Anonymous ""Then why do we currently have code in the book module that generates a menu? Shouldn't that belong in the menu.module, too?" " Because it's old code. It would be nice if book.module generated this block using its _menu hook, so that the admin would have a few options in terms of configuration. "Plus, if you do as you suggest, there is also the problem of the book showing up twice. It will be generated by the menu and then it will be generated again by the book module which is programmed to design a block. " No. The old code which manually builds a block in book.module would be removed. Book blocks would only be generated by the menu. This would also have the added benefit of allow the administrator to easily place a book in an appropriate spot in the menu tree, while still allowing the possibility of displaying it in a separate block. Because of menu caching, I don't expect a large performance hit for creating the menu items. "That was my main motivation for writing this patch: to make it easy to put links, not necessarily related to one another, into a block. " It sounds like instead of (mis)using book.module, your time would be better spent in a usability improvement to menu.module so it's easier to do this. ------------------------------------------------------------------------ December 16, 2004 - 07:55 : nysus Thanks for the feedback and input. I appreciate it. However, I would also appreciate if you took more care to avoid the condescending tone in your post: "It sounds like instead of (mis)using book.module, your time would be better spent in a usability improvement to menu.module so it's easier to do this. " It's really quite unnecessary and off-putting. Though it won't stop me from contributing to Drupal in the future, I'm sure others would be really turned off by such a patronizing comment and it could dissuade them. I'd like the Drupal community to be a welcoming and friendly place that will inspire people to contribute, not discourage them. Thanks. ------------------------------------------------------------------------ December 16, 2004 - 11:07 : Bèr Kessels nyesus, Please do not start /that/ discussion here. :) Drupal community is known fo being direct, maybe because of the big number of western-Europeans attending, maybe because of other reasons. No-one commented that you are wasting your time. But the commentor was telling you somthing likethis: "If you would follow the previous sugeestions, your added feature would be much better appreciated, and will probably work much better for you too". He/she was by no means telling you to stop your silly coding, or anything in that line. He/She only wanted to show you the obvious and better direction. We often deal with issues that add some feature, and a complete new UI, because the author does not like, or cannot use the existing UIs and features. This is nogt good, because if that same author would have spend his/her time on improving that existing functionality or UI (improving is not neccesarily the same as extending!!) that code and time would benefit all much better. Thats what the commentor tried to say. And so is it here: If you dislike the way the books handle the blocks, and if you do not want to use the menu, because you do not like its UI, then do not add another UI and more features, but rather merge these, and improve the parts (in the menu) you dislike. ------------------------------------------------------------------------ December 16, 2004 - 11:35 : Dries We can worry about the menu integration later. Let's focus on the new option's usability/interaction design first. ------------------------------------------------------------------------ December 16, 2004 - 15:51 : nysus I understand what the commenter was saying and like I said, I appreciate and understand it. I'm not upset and I'm not looking for an argument, I was just being direct as well. :) As part of the Drupal community (albeit a minor player), all I'm saying is that I would like to see folks not have a tin ear to the humanity in all of us. It will help make Drupal an even stronger community and attract even more talented developers. Human interaction is part of the development process. Whether we like it or not, we must cope and deal with it. ------------------------------------------------------------------------ December 18, 2004 - 10:54 : Dries I thought some more about this and am starting to believe that integration with the menu system should take priority. Here are common cool scenario's: I want to create a separate navigation block for the 'Drupal handbook'. I want to add a menu item called 'handbook' to the user block. That is, I want the book navigation to be part of the existing user navigation block. I want to add a menu item called 'handbook' to the top-level navigation menu. How would that work from a user's point of view? What do I have to click and where to configure this? ------------------------------------------------------------------------ December 21, 2004 - 19:47 : andremolnar I was actually thinking about the same thing last night (must have been something in the arctic air). / 2. I want to add a menu item called 'handbook' to the user block. That is, I want the book navigation to be part of the existing user navigation block. 3. I want to add a menu item called 'handbook' to the top-level navigation menu. / This is already possible (to a certain extent) with the current Book.module and Menu.module - A 'Books' menu item is created in the user navigation by having the Book.module enabled. Menu.module allows you to enable/disable this menu item. Menu.module also allows you to re-name this menu item. But, this only helps if you only intend to have 1 book (or else the name 'Handbook' is misleading if the user finds more than 1 book listed). - so this isn't good enough (or is it). /1. I want to create a separate navigation block for the 'Drupal handbook'./ This is what I was trying to figure out. Not just this, but a different way to do what Nysus was attempting. i.e. create a menu block for each and every book that is created. There is a way to write code that would (re)build a 'custom menu' on every add/edit/update to a book page - or book outline update. Custom menu's automatically have a block created for them. But, this I think would be a misleading use of the 'custom' menu - as the menu would not be custom if they are a part of core. So, I would think that two new constants could be added to the menu.inc file - MENU_BOOK_MENU - and MENU_BOOK_ITEM - each would behave as custom menu's, but would be reserved for books. These menu types should NOT show up in the Menu.module administration - because the administration of the book_menu items would be done by administering the book itself (adding an item, removing an item, moving an item up/down in the hierarchy, assigning parents etc.). However, the blocks that the book_menus would create would show up in the block administration (so users could enable/disable each block - and decide where on the site they show up). The existing book block logic would be removed. So the logic would be: If a creates a book in the book administration page - the Book.module automatically creates a new MENU_BOOK_MENU Any time a user adds to or updates or delets a book page - the entire book menu is deleted and recreated based on the hierarchy defined by the book itself. The blocks for each book would show up in block administration. Any thoughts - I know that this doesn't exactly address points 1 and 2 - but it could act as a first step. Is this approach a bad idea? It would add special conditions for the proposed book menu's - but books would be a special case. Even if people don't like this solution, maybe it will give someone a better idea. I'd love to hear them. andre ------------------------------------------------------------------------ December 21, 2004 - 23:55 : Boris Mann +1 to this andre I had promised to put down my thoughts on this matter, as it relates to 1) primary and secondary links and how they are managed and 2) auto-generation of primary/secondary navigation based on book outlines So, for 1), we currently have functional-but-not-very-usable plain textfields to manage primary and secondary links. I would like to see menu.module used to control all navigation links, whether it is the navigation block or primary/secondary links. What is needed: a) default system menus labelled "primary" and "secondary" which would store; this is where modules could insert navigation b) support for full URLs (e.g. http://myexternaldomain.com) instead of just path 2) if we got 1 working, and andre does his book menus, this could get taken care of automatically. Basically, for brochureware/business/etc. sites that have static content, you could have a root book as one of the primary navigation links, and then the secondary links are generated automatically. ------------------------------------------------------------------------ December 22, 2004 - 22:02 : Dries I agree with Andre that the book module's integration with the menu system needs to be worked on. I support any effort that makes it easier to structure pages and that makes it easy to link pages/nodes from within a menu. However, I'm opposed to putting book module specific names in the menu system (eg. MENU_BOOK_MENU and MENU_BOOK_ITEM). I can imagine a handful of modules that want to maintain a menu tree (or part thereof) so I'm in favor of generic names. I'd have to read up on the menu system code, but last time I checked there was a MENU_MODIFIABLE_BY_ADMIN flag. You could choose not to set this flag for the menu items generated by the book module. Maybe it's already possible to implement to implement Andre's suggestion without having to modify/extend the menu system. Are you exploring this path? ------------------------------------------------------------------------ December 24, 2004 - 11:01 : andremolnar I've had a few spare hours to work on this. I've started to cobble together a solution - but in doing so I discovered a bug in the menu module (for which I will submit a seperate issue - if one doesn't already exist). The principal I suggested works. I added some code to the book module that creates custom menu's (MENU_CUSTOM_MENU with MENU_CUSTOM_ITEMs) for each Book that exists in a site. This is just as a proof of concept - I chose this menu type to start with because they automatically have a block created for them in admin/block (which is ultimatly the functionality we want). I ran a test and the menus are created as expected - the blocks are also created. But when I tested the menu blocks by enabling them I ran into a problem. It seems as though the menu system does not expand/explode sub menu items if the node type is book. I'm not sure why this is, and I haven't traced the source code yet - I thought I would ask if anyone has intimate knowledge of the menu system if they can point me in the right direction. No patch attached because until that problem is fixed this proposed change won't fly :( andre ------------------------------------------------------------------------ December 24, 2004 - 12:11 : Dries Just a wild guess: maybe it doesn't work because the book module's URI scheme is not hierarchical? ------------------------------------------------------------------------ January 7, 2005 - 11:45 : andremolnar Attachment: http://drupal.org/files/issues/book_module_1.patch (5.1 KB) I finally took some time out to do some work on this. As mentioned in a post to dev the problem I was having with menus not expanding/contracting properly was some crud in my database. Once that was cleared up my changes worked fine. I am attaching a patch for comments and for the brave willing to give it a test drive. History: This thread and http://drupal.org/node/15198 and http://drupal.org/node/15153 node/15198 has a patch that is required for this patch to work. What this does: Pretty straight forward. Any time a user adds/updates/deletes book or book pages (including via outline) - a function is called to create a new menu for each book. Any existing book menus are wiped out and then the new book menus are inserted - and the system menu is rebuilt to reflect the changes. The menus created consist of type MENU_MODULE_MENU and MENU_MODULE_ITEM. These menus show up in the menu/admin page so that admins can be aware of them, but the menu types are not editable via menu/admin (all changes are handled by the book module). Since the menus are in the menu table, the menu_block() hook handles the creation of the blocks for each of them. The blocks can then be administered in the usual ways via the block/admin interface. KNOWN ISSUE: (suggested solutions welcome) Since the menu table is updated on every change to books - the blocks associated with the menus are also recreated with default settings (i.e. disabled, and with no path or throttle settings) requiring a user to take an extra step and re-configure the book blocks for viewing on their site. I think this is unacceptable. For the casual user of the book module that only has a single book that doesn't change often, this would not be a big deal. But, if anyone wanted to make use of book module to handle dynamic site navigation this would create more work than it saves. Looking forward: If the block generation issue can be solved in a tidy way, this patch could allow users to use book to handle all their site navigation generation needs. Also, this patch could allow for the removal of a large chunk of code in the book module dedicated to building its own block via the block system. I left it there for now because I suppose there may be those out there that want to have book.module work the way book.module always worked (only show the book block when viewing a book page). Even so, since each book would have its own block, an admin could specify when and where the block shows via admin/block. I would appreciate feedback. If nothing else I hope this gives someone some new ideas. I'll continue to work on the block regeneration issue. andre ------------------------------------------------------------------------ January 7, 2005 - 11:47 : andremolnar Attachment: http://drupal.org/files/issues/book_module_2.patch (4.98 KB) sorry - here's a patch with prefered line breaks. andre ------------------------------------------------------------------------ January 7, 2005 - 17:47 : nysus Andre, I've been meaning to give this a throrough look when I get a chance. Hopefully this weekend. ---Steve ------------------------------------------------------------------------ January 20, 2005 - 03:02 : andremolnar Attachment: http://drupal.org/files/issues/book_module_3.patch (6.66 KB) Here is an updated patch. Same comments as followup 51 - except that the known issue has been resolved. This changes book module so that any action taken on a book, including adding new books or book pages will create a menu in the menu system for that book - and thus create blocks for those menus that can be administered in the usual ways. This is my first attempt at a major patch to core - comments are welcome I will be happy to update documentation once revisions to the code have been taken care of. andre ------------------------------------------------------------------------ January 20, 2005 - 03:53 : andremolnar Attachment: http://drupal.org/files/issues/book_module_4.patch (6.17 KB) Sorry - would be nice if I removed some debugging code - ;-) also previous patch also would have introduced a need for a change to the menu table that isn't required yet. This patch is a correct version andre ------------------------------------------------------------------------ January 20, 2005 - 20:16 : Dries The menu is recreated every time a book page is updated. I believe this is unwanted behavior because it requires the book block to be reconfigured upon every update. ------------------------------------------------------------------------ January 20, 2005 - 20:32 : andremolnar The menu is indeed re-created with every book page edit - because if the book page title changes the menu needs to reflect this change. The book block re-configuration is not required by the user. The code stores this information and re-sets the block settings. I will see if I can test for 'title change' before forcing the re-build of the menu - It would save a bit of processing power. andre ------------------------------------------------------------------------ January 20, 2005 - 21:21 : andremolnar Took another at this - and it turns out that I already have a check to see if title or weight change (rather - they already existed in the module and I used them). andre ------------------------------------------------------------------------ January 24, 2005 - 03:22 : andremolnar Attachment: http://drupal.org/files/issues/book_module_6.patch (7.42 KB) Feedback received indicated that the original block generation code in this module should be removed - since this patch hands block generation off to the menu and block system. This patch removes that code. BUT - It should be noted, that in order to have book blocks only show up on pages of node type book - an additional patch found at http://drupal.org/node/16074 is required. andre ------------------------------------------------------------------------ January 24, 2005 - 19:36 : andremolnar Attachment: http://drupal.org/files/issues/book_module_7.patch (7.43 KB) Yet another patch: Brings this patch in line with - http://drupal.org/node/16074 (i.e. the column name change in {block} table) andre ------------------------------------------------------------------------ January 25, 2005 - 19:56 : andremolnar Attachment: http://drupal.org/files/issues/book_module_8.patch (7.53 KB) And another minor patch to cover coding style and variable names. Just to bring everyone up to speed: 1) Each book and its children pages have a menu created for them (on any add, edit, delete to a book or book page). 2) Those book menus automatically have a block created for them via menu_block hook. 3) Block settings are preserved any time there is a change to a book that requires the menu and its associated block to be re-built. 4) book_block hook is removed because it is redundant. 5) With the addition of the configuration option in blocks to only show blocks on certain node types (see: http://drupal.org/node/16074) administrators have full control over when and where a book block shows up (including maintaining the status quo of only having book blocks show up on book pages). 6) This patch does require http://drupal.org/node/15198 andre ------------------------------------------------------------------------ January 25, 2005 - 22:49 : Boris Mann +1!! Book-based automatic navigation for corporate/brochureware sites! Fantastic! Chris Messina, get in here and add a +1 to this. ------------------------------------------------------------------------ January 25, 2005 - 23:26 : Dries Boris, have you tested this patch extensively, or are you just giving this +1 based on what you've read? If you want to see this committed, take the time to review/test it. ------------------------------------------------------------------------ January 26, 2005 - 06:45 : andremolnar Yes - please - testers are welcome. Nysus (aka Steve) has already agreed to give this a test drive when he had a spare moment. I hope for some feedback in the near future. But if you have a moment Borris I would appreciate it. OR - If someone would just like to demo this: http://s92258948.onlinehome.us/greenbeach/ UN: drupaldev PW: drupaldev Don't mind the mess - it is my test environment and there is a lot of junk data and settings floating around - but this patch is running there along with the additional block configuration settings. andre ------------------------------------------------------------------------ January 31, 2005 - 20:12 : Boris Mann You got me, Dries... I have since spent time testing the module on andre's site, and it does everything I can foresee needing at this point, with no functional errors. This functionality alone will make creating standalone sites that are composed of mainly static pages very, very simple. ------------------------------------------------------------------------ February 3, 2005 - 09:58 : Dries The demo-site appears to be down. Bummer. ------------------------------------------------------------------------ February 3, 2005 - 23:59 : andremolnar Attachment: http://drupal.org/files/issues/book_module_9.patch (7.58 KB) Sorry - demo site was down while database was being cleaned and latest CVS modules were being installed (general site maintenence for testing). The site is up now with a fresh database and install of Drupal CVS. UN drupaldev PW drupaldev (has access to manage blocks, books, and menus to see this patch in action) Attached is an updated patch to make use of db_rewrite_sql (instead of node_rewrite_sql). Also fixes a minor bug that appeared if you were creating the first book in the site. andre ------------------------------------------------------------------------ February 4, 2005 - 00:13 : Anonymous This patch seems to modify the blocks and menu tables directly from within book.module. (in the new book_build_menu_module_menu function) This sets a terrible precedent for other modules. IMO, these tables should be modified by functions defined within the block module or menu module (or menu.inc). Modifying the tables randomly throughout many other points in the code makes changes to these tables difficult and makes troubleshooting problems much trickier. Let's not sacrafice code organization for the sake of making the patch as small as possible. ------------------------------------------------------------------------ February 4, 2005 - 07:25 : andremolnar With regard to the blocks - the block module is doing all the block building (with the call to _block_rehash) - the only thing that this code does is preserve block settings (the select sql) before the menu is updated - and then restores those block settings (with the update sql). "Modifying the tables randomly throughout... " While I appreciate the point you are making, I would hesitate to call this random. After all the menu items being added to the menu table 'belong' to book module. There is a little bit of a discussion about the best way to go about doing this sort of thing (i.e. allowing modules to build their own menus) over at http://drupal.org/node/15198 - which includes the patch required to make this patch work. Right now there is no existing hook or method of doing this. hook_menu does not have anything in place as of yet. I have suggested the introduction of a new hook (name yet to be determined) that would be in charge of these menu_module_menu type inserts. But, I haven't heard any feedback on whether that is a good idea - or if anyone has a better idea. Anyone else have thoughts on the approach taken here or suggestions for a different/better approach? I'm willing to write the code, I would just like a little bit of input. andre ------------------------------------------------------------------------ February 4, 2005 - 07:46 : Dries I added a test page and all book blocks dissapeared? I also got an SQL error (check your watchdog messages) upon pressing the 'Submit' button. -- View: http://drupal.org/node/14120 Edit: http://drupal.org/project/comments/add/14120

1 0

[drupal-devel] [bug] Signature "interrupts" typing comments due to existence in text field
by Steven 05 Feb '05

05 Feb '05

Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: minor Assigned to: Anonymous Reported by: irwin Updated by: Steven Status: patch By the way, the comments are currently fetched with a single query. You could easily extend it to get the signature with a JOIN instead of doing a query per comment. Steven Previous comments: ------------------------------------------------------------------------ February 4, 2005 - 05:47 : irwin In the Drupal comments/ signature system, there is a usability problem with the signature. The main problem is that, when adding a comment, the signature appears in the same line as the textarea that you are writing the content of your post. When you begin typing, your new text actually appears AFTER the signature (at least in Konqueror 3.2). This makes it incredibly annoying for users because you have to go to the beginning of the textarea before you can type your data. A corrolary to this problem is that, when quoting (using the quote module), the signature is included by default in the quote, which should not be the default behaviour. It appears that the signature is being added to the actual body text of the comment, as opposed to being retrieved from the user's profile when the page is rendered. -- >From an engineering/design perspective, one possible way to fix this is instead of adding the signature text to the comment, instead retrieve the signature dynamically from the user's profile when the page is rendered. If you do this, not only do you avoid the above problems, but you also improve the flexibility of signatures (for example, changing it once will cause the signature to change in every comment you make). You can also use this design change to give the users a choice of when to display their signature (for example, by using a checkbox on the node that says, "Display your signature with this post"). -- Thank you. ------------------------------------------------------------------------ February 4, 2005 - 06:49 : irwin Attachment: http://drupal.org/files/issues/signature_load_on_render-20050204.patch (1.62 KB) Included: patch. Changes explanation: "theme_comment_form": the code to autoinsert the signature into the textarea is removed. "theme_comment_view": code added to insert signature at the end of the $comment->comment data structure. I encase the comment in [p class="signature"] [/p] (with the correct brackets of course) so that a CSS theme can change the appearance of signatures. Caveat: the sig still appears in the [div class="body"] of the comment theme. Added function _comment_get_signature($uid), which gets a signature, given a user ID. Caveat: this requires an additional query to the "users" table with every user comment. Tested with: - User without signature - User with signature - Anonymous poster - Previewing - Positng - Editing a signature: expected result is that all signatures are changed to reflect the new signature. Please let me know of any problems. ------------------------------------------------------------------------ February 4, 2005 - 06:50 : irwin Reducing severity to "minor" because that's what it is. ------------------------------------------------------------------------ February 4, 2005 - 10:15 : Bart Jansens The extra queries aren't necessary, the queries that are used to retrieve the comments already join the users table, just add u.signature to the select clause there. I'm not sure if adding the signature to the comment body is the best approach, if you just keep it in a different field, themes can decide where to place it. Note that usually admins will want to be able to use a different filter for signatures. Eg, allow bbcode in the comment body, but not in the signature. I am using a similar patch on my own site (although it really needs to be updated), it never made it into core because it is difficult or almost impossible to create an upgrade path. I hope this can be discussed again, or at least made into an option for new sites, i'm surprised there haven't been more complaints about this ;) ------------------------------------------------------------------------ February 4, 2005 - 16:40 : irwin I realised as I was going to bed last night that the extra query was extraneous. :) As for adding the comment to the comment body, the reason I would want to do that is for the signature to "appear" to be a part of the... well, comment body. That will avoid potential issues with some themes that aren't designed to handle signatures at all - for instance, a theme might put a border around the "body" part of the signature, and if it doesn't explicitly handle the signature, things can look very ugly and strange. But I can see what you mean about Input Filtering. Do you have suggestions on how to improve that? I had one idea, and that was to add the signature as an argument to the theme, but that would require theme modifications fir signatures to be displayed. If this patch is of interest I'll keep working on it and posting updates. ------------------------------------------------------------------------ February 5, 2005 - 08:19 : Steven Have you considered that not everyone might want retroactive signatures? It depends on the preference of the site owner I guess. -- View: http://drupal.org/node/16726 Edit: http://drupal.org/project/comments/add/16726

1 0

[drupal-devel] [bug] Signature "interrupts" typing comments due to existence in text field
by Steven 05 Feb '05

05 Feb '05

Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: minor Assigned to: Anonymous Reported by: irwin Updated by: Steven Status: patch Have you considered that not everyone might want retroactive signatures? It depends on the preference of the site owner I guess. Steven Previous comments: ------------------------------------------------------------------------ February 4, 2005 - 05:47 : irwin In the Drupal comments/ signature system, there is a usability problem with the signature. The main problem is that, when adding a comment, the signature appears in the same line as the textarea that you are writing the content of your post. When you begin typing, your new text actually appears AFTER the signature (at least in Konqueror 3.2). This makes it incredibly annoying for users because you have to go to the beginning of the textarea before you can type your data. A corrolary to this problem is that, when quoting (using the quote module), the signature is included by default in the quote, which should not be the default behaviour. It appears that the signature is being added to the actual body text of the comment, as opposed to being retrieved from the user's profile when the page is rendered. -- >From an engineering/design perspective, one possible way to fix this is instead of adding the signature text to the comment, instead retrieve the signature dynamically from the user's profile when the page is rendered. If you do this, not only do you avoid the above problems, but you also improve the flexibility of signatures (for example, changing it once will cause the signature to change in every comment you make). You can also use this design change to give the users a choice of when to display their signature (for example, by using a checkbox on the node that says, "Display your signature with this post"). -- Thank you. ------------------------------------------------------------------------ February 4, 2005 - 06:49 : irwin Attachment: http://drupal.org/files/issues/signature_load_on_render-20050204.patch (1.62 KB) Included: patch. Changes explanation: "theme_comment_form": the code to autoinsert the signature into the textarea is removed. "theme_comment_view": code added to insert signature at the end of the $comment->comment data structure. I encase the comment in [p class="signature"] [/p] (with the correct brackets of course) so that a CSS theme can change the appearance of signatures. Caveat: the sig still appears in the [div class="body"] of the comment theme. Added function _comment_get_signature($uid), which gets a signature, given a user ID. Caveat: this requires an additional query to the "users" table with every user comment. Tested with: - User without signature - User with signature - Anonymous poster - Previewing - Positng - Editing a signature: expected result is that all signatures are changed to reflect the new signature. Please let me know of any problems. ------------------------------------------------------------------------ February 4, 2005 - 06:50 : irwin Reducing severity to "minor" because that's what it is. ------------------------------------------------------------------------ February 4, 2005 - 10:15 : Bart Jansens The extra queries aren't necessary, the queries that are used to retrieve the comments already join the users table, just add u.signature to the select clause there. I'm not sure if adding the signature to the comment body is the best approach, if you just keep it in a different field, themes can decide where to place it. Note that usually admins will want to be able to use a different filter for signatures. Eg, allow bbcode in the comment body, but not in the signature. I am using a similar patch on my own site (although it really needs to be updated), it never made it into core because it is difficult or almost impossible to create an upgrade path. I hope this can be discussed again, or at least made into an option for new sites, i'm surprised there haven't been more complaints about this ;) ------------------------------------------------------------------------ February 4, 2005 - 16:40 : irwin I realised as I was going to bed last night that the extra query was extraneous. :) As for adding the comment to the comment body, the reason I would want to do that is for the signature to "appear" to be a part of the... well, comment body. That will avoid potential issues with some themes that aren't designed to handle signatures at all - for instance, a theme might put a border around the "body" part of the signature, and if it doesn't explicitly handle the signature, things can look very ugly and strange. But I can see what you mean about Input Filtering. Do you have suggestions on how to improve that? I had one idea, and that was to add the signature as an argument to the theme, but that would require theme modifications fir signatures to be displayed. If this patch is of interest I'll keep working on it and posting updates. -- View: http://drupal.org/node/16726 Edit: http://drupal.org/project/comments/add/16726

1 0

[drupal-devel] [task] Put revisions in their own table
by Steven 05 Feb '05

05 Feb '05

Project: Drupal Version: cvs Component: node system Category: tasks Priority: normal Assigned to: killes(a)www.drop.org Reported by: killes(a)www.drop.org Updated by: Steven Status: patch By the way, I just remembered that Drupal.org has some blogs lingering on in the database even though blog.module is not enabled. Perhaps this is causing troubles? Steven Previous comments: ------------------------------------------------------------------------ May 5, 2004 - 17:25 : killes(a)www.drop.org Currently all node revisions are stored in a serialized field in node.table and retrieved for _each_ page view although they are rarely needed. However, we have agreed that serializing data is bad and that we should try to keep the memory foot print pf Drupal small. Therefore I propose to create a separate revisions table which would be in principle identical to the node table, only that it could have several old copies of the same node. Extra data added by other modules could be added in a serialized field unless we find a better solution. ------------------------------------------------------------------------ May 5, 2004 - 18:06 : jhriggs I too think the serialized approach is less than desirable, but here's an alternative. This would likely take some considerable rework in core and contrib, but the following is how we handle similar types of situations in our databases at work. It is more elegant that a separate table, and avoids the (almost exact) duplication of a table. Instead of separate tables, keep all revisions of nodes in the node table as follows: * add field: active (0/1 or Y/N) * add field: revision * every revision of a node is stored in the node table; however, only one revision can be active at any given time * nid can no longer be unique -- primary/unique key becomes (nid, active) * any time a node is loaded, updated (without revision), etc., the active version is used. Thoughts? ------------------------------------------------------------------------ May 5, 2004 - 18:57 : killes(a)www.drop.org I am not opposed to your scheme, but I want to stress the following: * Duplicating a table's structure is not bad (IMHO) as long as the content is different. * having two tables will allow us to have a rather small node table. This is (maybe) a performance gain. ------------------------------------------------------------------------ May 5, 2004 - 19:37 : jhriggs I don't necessarily think that duplicating a table's structure is _bad_. It just seems to be wasteful and a pain to maintain. (Every change to the node table is made twice...easy to do, but also easy to miss perhaps.) As for performance, as long as nid and the active indicator are indexed, there shouldn't be any performance loss. Also, archiving an old version when making a new revision will be much simpler: just change the active indicator rather than copying an entire node to another table (and ensuring everything gets copied...again a potential maintainance issue). To be honest, I would just like to see the serialized data go away, regarless of what approach is taken. ------------------------------------------------------------------------ July 30, 2004 - 20:49 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_07-30-2004.p… (10.47 KB) I'm interested in using Drupal for a large scale wiki-type project. In order to do this, I need revisions to be in their own table. Attached is a patch to do just that. Most of the changes are pretty self explanatory. Spreading out node data across two tables meant that I had to add database functions to do locking/transactions. Without this, race conditions in which the database becomes corrupted are possible. ------------------------------------------------------------------------ July 30, 2004 - 20:54 : Nick Nassar Oh yeah... The patch is a diff against Drupal CVS ------------------------------------------------------------------------ July 31, 2004 - 01:00 : Anonymous Gerhard speaking. Nick, thanks a lot for your nice patch! It saves me a great deal of labour. I looked through it and immediately liked it. You not only put the old revisions into a new table but also the current one. Do you have an estimate how much more expensive the additional join is? Besides a few minor coding style issues I found a major one: Just a few hours before you uploaded your patch JonBob's node access patch hit core. That means your patch won't apply anymore as all the queries you change have been changed. Can I bug you to update your patch? ------------------------------------------------------------------------ July 31, 2004 - 02:11 : Anonymous Also I think that your upgrade path loses existing revisions. ------------------------------------------------------------------------ July 31, 2004 - 03:39 : drumm I think this is the proper way to do things. No columns are duplicated, there is no serialized data, and only the fields that are logically revised are stored. Nothing jumped out at me as a way to have my node modules be able to keep a table of revisions of additional fields. I'm guessing this could be done within the confines of _insert and _update. Assuming the upgrade path works and modules can extend it I give it a +1. ------------------------------------------------------------------------ July 31, 2004 - 15:40 : Nick Nassar It figures that just as I finish a big patch, another patch comes along and breaks it. Oh well, it should be a pretty easy to fix. I'll work on it. Fixing the upgrade path to keep revisions should be fairly painless. I found another issue that needs to be fixed before this patch gets merged. There format of a node needs to be stored for each revision. Otherwise, for modules that store a format for the nodes, such as page and book, if you write one revision in PHP and the next in HTML, the PHP revision will be displayed as HTML. This is part of a larger issue of how node modules should store revisions of additional fields. I think each module that wants to do this should create another table with (nid, revid) as the primary key. Just as when they want to add fields to a node they create another table with nid as the primary key. As far as performance goes, for sites that make heavy use of revisions, an extra join on primary keys is going to be a lot faster than grabbing all of the revisions from that database everytime. We would need to run benchmarks to determine is the overall difference in speed is for an average site is a gain or a loss. I'm guessing it's very minor either way. ------------------------------------------------------------------------ August 23, 2004 - 14:55 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_08-23-2004.p… (10.92 KB) Here's an updated patch against CVS that puts revisions in their own table, provides an upgrade path, and fixes the format related bugs in the last patch. Hopefully, this can make it into CVS as soon as the freeze is over. ------------------------------------------------------------------------ August 23, 2004 - 15:10 : moshe weitzman Interesting patch ... drumm's question is still outstanding. how do modules store revisions of their fields? Are they expected to manage this on their own? Thats not how it works today. As an aside, i am seeing profile_ fields in my node.revisions column. One could argue that those need not be saved. They pertain to the node author, not to the node itself. ------------------------------------------------------------------------ August 23, 2004 - 17:14 : Nick Nassar Having modules be responsible for storing revisions of their own fields is a side-effect of storing revision data in tables. There's really no way around it. However, revisions generally don't make sense for node types that don't have PHP/HTML content, such as polls. I think it's going to be a pretty rare scenario for a new node type to want another field to change per-revision, so it's a pretty good trade-off. Storing fields that shouldn't be part of revisions, such as the profile_ fields, is a side-effect of storing revisions as serialized objects. Applying this patch will free up that wasted space. :) ------------------------------------------------------------------------ August 23, 2004 - 18:20 : Anonymous There should be a hook that let's the module choose whether it supports history. This way a module author can prevent the user from doing something that may break his module or just cause undefined behavior. If the module doesn't support history then don't let the user/admin choose to add history to nodes of that type. Craig ------------------------------------------------------------------------ August 23, 2004 - 20:23 : Nick Nassar I agree, there should be an API change to make specifying support for revisions easier. In the interests of keeping patches small and keeping to one change per patch, I think the API change should be a separate issue. A sort of ad-hoc API to decide whether or not a module supports revisions by default already exists. Instead of having a hook, modules set the default value of the "Create new revision" field in the edit form. The admin can change this option in admin/node/configure/defaults. This patch doesn't change that. Revisions are broken for node types that have their own database structure, like polls, even when storing them as serialized objects. This patch doesn't change that, either. ------------------------------------------------------------------------ October 26, 2004 - 03:35 : moshe weitzman I'm guessing that someone is going to have to demonstrate that this patch performs as well as current drupal before it gets comitted. i think this patch is a few benchmarks from being comitted. ------------------------------------------------------------------------ October 27, 2004 - 02:04 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB) I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ October 27, 2004 - 02:05 : Nick Nassar I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ October 27, 2004 - 02:05 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB) I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ November 15, 2004 - 06:05 : elias1884 please overthink the revision system default workflow as well. don't look at the revision system as an isolated system but as a part of the whole workflow system! if you combine revisions with the moderation queue the most logic default workflow would be like that: auth user creates node (revision #0) admin approves the node (status = 1, moderation = 0) => node publicly available auth user finds typo and changes node (revision #1, status = 0, moderation = 1) ------- what happens at that point at the moment is, that the node is not accessible anymore at all until the new revision is approved by admin. of course the new revision should not go online until reviewed and approved, this is absolutely correct, but there is no reason to not take the old revision offline, since it was already approved and should therefore be online until the new revision is approved. it is not practical if a node disappears only because the author corrected a typo. ------- admin approves the node (status = 1, moderation = 0) eventhough I first thought a plain boolean active field would not be capable of providing that functionality if finally came to the conclusion, that it can. The only thing to do is to not set that bit, when a new revision is created, but when it is approved (in case moderation is activated under default workflow). Every revision should have its own moderation, status and active field and on approval they are set like this (status=1, moderation=0, active=Y). When you wanna rollback to an old revision, you can chose between all revisions that already have the moderation bit set back to 0 again and the published to 1. There should be an extra permission for rollback! another concern that I have about the default workflow is, that users can't see the content, they have just created, when moderation is enabled. Eventhough, there is a big fat "submission accepted" presented after submissions, unexperienced users tend to question the information those stupid tincans give them, if they can't find their content afterwards. Many users are really lazy bastards and they don't even read the status messages. The best feedback about whether his story was submitted successfully or not of course is, if he can find the story somewhere on the site, maybe with a status message on top of it, mentioning, that the content is currently not publicly available since it has not been approved yet. there should be a my content section under my account, like somebody is trying to do with the workspace module I guess. so my suggestion is to make (status=0, moderation=1) still available for the creator under a my content section somewhere! ------------------------------------------------------------------------ November 24, 2004 - 05:21 : Nick Nassar I agree. The current workflow for moderation queues and revisions needs to change, but this patch isn't the place for it. The patch is already too big, and it only does the backend stuff. Instead of adding more to this patch and making it take even longer to get into core, would you mind creating a new issue for your UI suggestions, so the those changes can be added as a separate patch? Thanks, Nick ------------------------------------------------------------------------ December 11, 2004 - 13:26 : Dries This patch is _much_ needed so I'd love to see someone revive it. In order for this patch to be accepted, the following needs to be done: Update this patch to CVS HEAD. Rename revid to vid. Rename node_rev to node_revisions. Rename node_rev.changed to node_revisions.timestamp. Rename $rnode to $revision. Fix the coding style to match Drupal's: proper spacing, single quotes where possible, proper variable names. Benchmark this patch with a large database with enough revisions. I'd be happy to benchmark this on my local copy of the drupal.org database. The book.log field should probably move to the node_revisions table. This can be done in a separate patch. Investigate whether transactions are well-supported. ------------------------------------------------------------------------ December 13, 2004 - 01:25 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004-r… (11.02 KB) I've worked a bit on the patch (coding style issues as mentioned by Dries). One thing I noticed is that the patch uses REPLACE. IIRC this needs to be chagned to "UPDATE, if fail INSERT" for pgsql compatibility. Nick, are you still interested in working on that patch? I'd like to know how it works on your site and work on getting it into core. ------------------------------------------------------------------------ December 13, 2004 - 13:33 : Dries Gerhard: your patch does not apply. ------------------------------------------------------------------------ December 13, 2004 - 18:10 : killes(a)www.drop.org Yes, I know, that was the same version as I mailed to you earlier. ------------------------------------------------------------------------ December 13, 2004 - 22:02 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions.patch (52.96 KB) Ok, upüdated the patch to cvs. ------------------------------------------------------------------------ December 14, 2004 - 09:58 : Dries Some more comments: db_begin_transaction() and db_end_transaction() do not belong in database.inc, but in database.mysql.inc and database.pgsql.inc respectively. The node module calls node_revisionsision_list() which is not defined. (Fxed that on my local copy.) Do db_begin_transaction() and db_end_transaction() deprecate Jeremy's table locking patch? The upgrade path assigns the wrong user ID to each revision. The upgrade path assigns the wrong date to each revision (that or a node's revision page shows the wrong usernames/dates). The coding style needs a bit of work, but we can worry about that later. ------------------------------------------------------------------------ December 14, 2004 - 18:34 : Nick Nassar If you need any help getting those things fixed, just let me know. ------------------------------------------------------------------------ December 14, 2004 - 18:50 : Nick Nassar How this relates to Jeremy's node locking patch: There was lots of discussion, and node locking was decided against because from an end user point of view you never want a node to be locked. He's now advocating for a much simpler patch that warns users if their changes will overwrite someone elses. That patch still has a race condition, which might be fixed using db_begin_transaction(). http://drupal.org/node/6025 ------------------------------------------------------------------------ December 14, 2004 - 23:26 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_0.patch (55.96 KB) Here is an updated patch that tries to address Dries concerns. ------------------------------------------------------------------------ December 15, 2004 - 09:32 : Dries Attachment: http://drupal.org/files/issues/revisions-bug.png (76.06 KB) It didn't fix the aforementioned bugs. See attached screenshot. ------------------------------------------------------------------------ January 6, 2005 - 21:15 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_1.patch (51.77 KB) Ok, here is a new version. Dries and myself worked hart at it, so please have a look. what is still missing - database upgrades for the core modules with an own table - contrib modules need an upgrade too. - do we need nid and vid in both the node and the node_revisions table? - the amount of sql queries means a good stress testing for large databases. ------------------------------------------------------------------------ January 19, 2005 - 22:43 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_2.patch (49.49 KB) Here is an updated patch. We discussed to keep the current title in node module and also in the revisiosn table. This is content duplication but will save many joins as many queries only need the title of a node. Discussion is welcome. ------------------------------------------------------------------------ January 20, 2005 - 00:33 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_3.patch (29.93 KB) I've implemented the aforementioned solution. This makes the patch much smaller. The patch now also removes taxonomy_node_has_term() which wasn't used anywhere. I'd really apprciate if some people could test drive the patch. It will be another huge improvement for 4.6. ------------------------------------------------------------------------ January 20, 2005 - 01:05 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_4.patch (30 KB) Another revision. Steven didn't like my literal $node->vid in queries. ------------------------------------------------------------------------ January 20, 2005 - 02:10 : killes(a)www.drop.org - database upgrades for the core modules with an own table - contrib modules need an upgrade too. - do we need nid and vid in both the node and the node_revisions table? - the amount of sql queries means a good stress testing for large databases. These issues are still open, btw. Especially the first one needs to be tackled. ------------------------------------------------------------------------ January 25, 2005 - 21:11 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_5.patch (51.13 KB) Here is a patch that has the database tables updated for forum, book, and page module. ------------------------------------------------------------------------ January 29, 2005 - 23:55 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_6.patch (49.18 KB) Yet another update to keep it working with head. The patch now also removes the table definitons for the page table. ------------------------------------------------------------------------ January 29, 2005 - 23:57 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_7.patch (55.69 KB) Sorry, that was the old version, this is the right one. ------------------------------------------------------------------------ January 31, 2005 - 20:55 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_8.patch (55.71 KB) Updated once more. ------------------------------------------------------------------------ January 31, 2005 - 21:52 : Dries Anyone to help review/test this? ------------------------------------------------------------------------ January 31, 2005 - 22:22 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_9.patch (49.29 KB) Updated again, the update functions occurred twice. Thanks Bart. ------------------------------------------------------------------------ February 2, 2005 - 01:27 : killes(a)www.drop.org Don't know if the db I am using is corrupted or what. I still do have some didficulties. The latest patch is attached. ------------------------------------------------------------------------ February 2, 2005 - 01:27 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_10.patch (49.67 KB) I am probably slowly going mad ... ------------------------------------------------------------------------ February 2, 2005 - 02:54 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_11.patch (48.95 KB) The update issue still needs investigating. This patch is updated for cvs. ------------------------------------------------------------------------ February 2, 2005 - 21:20 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_12.patch (49.83 KB) Ok, here is a new version. I've solved my troubles with book.module. There are still some issues with forum module. Possibly due to inconsistent database. ------------------------------------------------------------------------ February 2, 2005 - 22:31 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_13.patch (49.83 KB) Turns out the drupal.org database had indeed some quirks. Please run this query in your oldest db and tell me the result: select nid,type from node where type like '%/%'; If you get a non-zero result we might need to add another security update. The patch could use still more testing, though. ------------------------------------------------------------------------ February 3, 2005 - 02:16 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_14.patch (49 KB) Ok, we are getting somewhere. At a first glance the update is working. There is a problem remaining: the revisions tab will be shown whether the node has revisions or not. Not sure we can/need to fix this. People with a drupal.org account can log in at http://killes.drupaldevs.org/revision/ and poke around. Your permissions will be the same as on drupal.org. Feel free to vreak everything but don't forget to file complaints here. (Note: this is only a pruned version of the drupal.org database with all project nodes and nodes with nids > 7000 dropped). ------------------------------------------------------------------------ February 3, 2005 - 05:19 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_15.patch (52.39 KB) There was some error in node_save and also the patches to the database.inc files got lost... ------------------------------------------------------------------------ February 3, 2005 - 08:07 : robertDouglass Submitting book pages doesn't work on your test site. It puts the entire content of the preview inside the body textarea. I wrote a sentence in the body and the log, and pressing preview put several lines of HTML containing both sentences in the body textarea on the preview page, plus the book page wouldn't submit. -R ------------------------------------------------------------------------ February 3, 2005 - 08:50 : Junyor 0 results here. I started using Drupal with version 4.4, though. ------------------------------------------------------------------------ February 4, 2005 - 00:56 : killes(a)www.drop.org @Junyor: Thanks, that's a good sign. Maybe somebody else has an older db to try. @robertDouglass: The first effect you describe is due to drupaldevs running on PHP 5. I am unsure why the second thing does not work. In node_save() the node object has a nid although there is none in the form. Very strange. I've enabled display of db queries on the testsite. ------------------------------------------------------------------------ February 4, 2005 - 20:17 : dmjossel No results here on the query: select nid,type from node where type like '%/%'; On a database that was put in place prior to Drupal 4 and is now running on 4.5.2. ------------------------------------------------------------------------ February 4, 2005 - 21:44 : killes(a)www.drop.org @dmjossel: thanks. @all. The strange problem I reported was apparently php 5 related. After applying Steven's php 5 patch it went away. One error is remaining: If I create a new forum topic it will be shown as part of the book on preview. Hmm, that was due to a db that got corrupted during testing so that is fixed too. Please poke around at the test site and look if you find more errors. -- View: http://drupal.org/node/7582 Edit: http://drupal.org/project/comments/add/7582

1 0

[drupal-devel] cvs log messages
by no-reply＠zind.net 05 Feb '05

05 Feb '05

File: /modules/workspace/README.txt 1.4 Date: February 5, 2005 - 02:15 User: jvandyk add note about MySQL 3.x compatibility, change admin path to administer -> access control ---------------------------------------------------------------------- File: /modules/workspace/workspace.module 1.4.2.6 DRUPAL-4-5 Date: February 5, 2005 - 00:53 User: jvandyk fix #16792 by matteo (selectbox now works with translations) ---------------------------------------------------------------------- File: /modules/workspace/workspace.module 1.12 Date: February 5, 2005 - 00:50 User: jvandyk fix #16792 by matteo (selectbox now works with translations) ---------------------------------------------------------------------- File: /modules/fontsize/fontsize.module 1.4 Date: February 5, 2005 - 00:34 User: drumm Using the init hook does not play well with caching pages, moving drupal_set_html_head() call to hook_menu(). ---------------------------------------------------------------------- File: /modules/fontsize/fontsize.module 1.2.2.1 DRUPAL-4-5 Date: February 5, 2005 - 00:33 User: drumm Using the init hook does not play well with caching pages, moving drupal_set_html_head() call to hook_menu(). ---------------------------------------------------------------------- File: /sandbox/javanaut/nodeattachment/nodeattachment.module 1.8 Date: February 5, 2005 - 00:32 User: javanaut Fixed some copy/paste erroneous code. Fixed some nid vs. array of nids logic that was broken. Started using this module with editasnew.module. Though no specific code was written for that, it did reveal the above broken logic. ---------------------------------------------------------------------- File: /modules/stock/TODO 1.1 Date: February 5, 2005 - 00:27 User: kbahey Initial TODO list ---------------------------------------------------------------------- File: /modules/stock/stock.module 1.5 Date: February 5, 2005 - 00:24 User: kbahey Added a color code that changes from green for a stock that has increased, or red when it decreases. Ideally, this would be an id/class attribute in XHTML that can be themed using CSS to anything (e.g. up arrow, down arrow, color, ...etc), but at least we have something for now. ---------------------------------------------------------------------- File: /includes/database.inc 1.38 Date: February 5, 2005 - 00:12 User: unconed - #16111: bad quotes in db_rewrite_sql ---------------------------------------------------------------------- File: /modules/editasnew/editasnew.module 1.1.2.2 DRUPAL-4-5 Date: February 4, 2005 - 23:58 User: javanaut Fixed security flaw that would allow users to view content in template nodes that they otherwise are not permitted to do. ---------------------------------------------------------------------- File: /modules/editasnew/editasnew.module 1.3 Date: February 4, 2005 - 23:56 User: javanaut Fixed security flaw that would allow users to view content in template nodes that they otherwise are not permitted to do. ---------------------------------------------------------------------- File: /modules/workspace/po/it.po 1.2 /modules/workspace/po/workspace.pot 1.2 Date: February 4, 2005 - 23:17 User: matteo Translation template ---------------------------------------------------------------------- File: /modules/workspace/po/it.po 1.1 /modules/workspace/po/workspace.pot 1.1 Date: February 4, 2005 - 23:08 User: matteo Translation template ---------------------------------------------------------------------- File: /modules/editasnew/po/editasnew.pot 1.1 /modules/editasnew/po/it.po 1.1 Date: February 4, 2005 - 22:53 User: matteo Translation template ---------------------------------------------------------------------- File: /sandbox/javanaut/nodeattachment/nodeattachment.module 1.7 Date: February 4, 2005 - 22:45 User: javanaut Various updates: 1. Changed parent ordinality from "requires parent" boolean to a multi-selection box including: none, one, one or more, any 2. Changed "attach new node", "attach existing node" links so that they only appear if user permissions and parental ordinality accommodate the actions. 3. Hopefully resolved the node_access('create'..) problems I was having by addressing an issue with the user_access(..) function (see http://drupal.org/node/16705) 4. Reworked a bunch of URL juggling so that it's a bit cleaner. Where possible, I've replaced query string arguments with url arguments. This includes menu paths under node/add that I've taken over to some degree. Still haven't figured out how to limit the user/add node list such that when users create nodes there, they only see the types that don't require parents (while still allowing them to be created using the parented approach). 5. Fixed bug in nodeapi delete section where not all relationships were cleaned up when a node was removed. ---------------------------------------------------------------------- File: /sandbox/javanaut/nodeattachment/nodeattachment.mysql 1.2 Date: February 4, 2005 - 22:33 User: javanaut Added index keys in the table definition. ---------------------------------------------------------------------- File: /modules/inline/CHANGELOG.txt 1.6 /modules/inline/inline.module 1.7 Date: February 4, 2005 - 22:09 User: matteo ---------------------------------------------------------------------- File: /modules/editasnew/editasnew.module 1.2 /modules/editasnew/README.txt 1.2 Date: February 4, 2005 - 21:24 User: javanaut Initial import. I just wrote this, tested it briefly and uploaded it here. This project adds a "edit as new" tab to the node/$nid/view page that lets users create new nodes based on the content from an existing one. ---------------------------------------------------------------------- File: /modules/editasnew/editasnew.module 1.1.2.1 DRUPAL-4-5 /modules/editasnew/README.txt 1.1.2.1 DRUPAL-4-5 Date: February 4, 2005 - 21:18 User: javanaut Initial import. I just wrote this, tested it briefly and uploaded it here. This project adds a "edit as new" tab to the node/$nid/view page that lets users create new nodes based on the content from an existing one. ---------------------------------------------------------------------- File: /modules/node.module 1.460 Date: February 4, 2005 - 20:48 User: unconed - Fixing broken drupal_goto() after node submission on PHP5. ---------------------------------------------------------------------- File: /includes/common.inc 1.424 /modules/node.module 1.459 Date: February 4, 2005 - 20:14 User: unconed - PHP5 fix due object references: node previewing should not put node output into form. ---------------------------------------------------------------------- File: /modules/stock/stock.module 1.4 Date: February 4, 2005 - 04:50 User: kbahey Optimized querying Yahoo Finance, by opening the connection only once, and getting all the quotes, then closing. Previously, it was open/get/close for every symbol provided, which is inefficient. Also added error trapping on the fopen() to the Yahoo Finance URL, so we give a descriptive error. Adding the @fopen did not seem to suppress the errors though. ----------------------------------------------------------------------

1 0

[drupal-devel] [feature] Support for category images in core
by Steven 05 Feb '05

05 Feb '05

Project: Drupal Version: cvs Component: taxonomy.module Category: feature requests Priority: normal Assigned to: Jeremy(a)kerneltrap.org Reported by: Jeremy(a)kerneltrap.org Updated by: Steven Status: patch With the new image API, this function can probably be implemented very cleanly and with less code. Perhaps it should be revisited? I think many people will like it, but I also agree that it should not be in the way. Steven Previous comments: ------------------------------------------------------------------------ June 1, 2004 - 01:46 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/taxonomy_image.patch (18.45 KB) The attached patch adds a module with a simple interface for associating images with taxonomy terms. The patch applies against current cvs. In addition to adding a new module and updating the database, it also updates the xtemplate theme to display category images (if enabled). If merged into core, all themes should then be updated to support category images. Some changes may need to be made for this patch to be merged into core. Suggestions are welcome. (For example, perhaps the functionality should actually be merged into the taxonomy module itself?) Features of this module include: - integrated into existing taxonomy (category) menu structure - allows one-to-one and many-to-one term-to-image relationships - administrator can configure to auto-scale images - images can be disabled by individual users (with proper permissions) - utilizes core file api - small, focused feature-set. Requires: - cvs version of Drupal - taxonomy module For a live demo, go to http://kerneltrap.org/ Screenshots to follow. ------------------------------------------------------------------------ June 1, 2004 - 01:58 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/overview.png (22.49 KB) Here's an overview look at the administrative interface. In this screenshot, one image has already been uploaded. ------------------------------------------------------------------------ June 1, 2004 - 01:59 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/upload.png (14.71 KB) Here's the upload page, after having clicked 'upload image'. ------------------------------------------------------------------------ June 1, 2004 - 02:00 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/uploaded.png (31.65 KB) And now we're back at the overview administration page, after having uploaded a new image. ------------------------------------------------------------------------ June 1, 2004 - 02:01 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/edit.png (24.08 KB) Here's the form you see when clicking 'edit image'. If you upload a new image when an old image was already uploaded, the old image will first be automatically deleted, then the new image will be saved. ------------------------------------------------------------------------ June 1, 2004 - 02:03 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/settings.png (26.28 KB) Here's a look at the module's settings page. It allows the administrator to auto-resize category images when displayed, and to support many-to-one term-to-image relationships (recursive image display) to support taxonomy hierarchies. ------------------------------------------------------------------------ June 1, 2004 - 02:04 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/frontpage.png (34.15 KB) And finally, here's a look at the front page of a site running with the patched xtemplate, displaying category images... ------------------------------------------------------------------------ June 1, 2004 - 02:21 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/multiple_terms.png (34.23 KB) I was asked how it handles multiple categories assigned to one node. This depends on the theme. The attached screenshot shows how xtemplate handles it with my patch applied, displaying both images together. ------------------------------------------------------------------------ June 1, 2004 - 03:02 : Anonymous Excellent! I've been wishing Drupal had this feature for a long time; this would be so much easier than hardcoding images into my theme. How does this interact with the avatar functionality? (Meaning: What would it look like if someone enabled both avatar display and taxonomy image display for a node?) ------------------------------------------------------------------------ June 1, 2004 - 06:07 : Dries I'm in favor of integrating this into taxonomy.module. Maybe we could start by reviewing the GUI? ------------------------------------------------------------------------ June 1, 2004 - 14:03 : Jeremy(a)kerneltrap.org The main argument for not merging this functionality into the taxonomy module is that it's very possible that the majority of Drupal users do not need this functionality. Do you think it should retain its own menu item called 'images' under categories? Or should it actually be merged into the existing add/edit term page? If kept as is, then what recommendations for improving the user interface? If merged into the existing taxonomy add/edit page, where would you get an image overview? (It can't easily be added to the existing taxonomy overview page, which is already quite "busy") ------------------------------------------------------------------------ June 1, 2004 - 14:07 : Jeremy(a)kerneltrap.org "How does this interact with the avatar functionality? (Meaning: What would it look like if someone enabled both avatar display and taxonomy image display for a node?)" This is up to the theme implementation. I've never used avatars - I don't even know how - so I didn't give this any though with my sample patch. A quick look at the code, however, suggests that they would all be displayed together on the right side of the screen. I'm not a gui guy. I would hope that theme designers would be the ones to actually add taxonomy_image support to their themes once this functionality is merged. ------------------------------------------------------------------------ June 1, 2004 - 16:57 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/taxonomy_image_0.patch (20.76 KB) I've updated the patch mainly per feedback on the drupal-devel mailing list: - added support for private images - stores images in configurable files/ subdirectory - refers to images as pictures, to standardize with avatar support This brings the patch current with all outstanding recommendations. (BTW: If interested in trying the latest code with Drupal 4.4.x, download the cvs version of the module in contrib) ------------------------------------------------------------------------ August 18, 2004 - 01:42 : Jeremy(a)kerneltrap.org FWIW: This module [1] has been updated to work with the latest CVS code (Drupal 4.5). I'm not going to update this patch though, as I assume effort is being made to address this issue in another way. Should an updated patch be desired, let me know. [1] http://drupal.org/node/view/8121 ------------------------------------------------------------------------ August 21, 2004 - 02:43 : nedjo The proposed patch is an improvement on functionality previously roughed in in the node_image module. I'd like to look at the possibility of rolling node_image functionality into this patch. Potential modifications might be: Images could be associated with individual nodes as well as terms (and, perhaps, other objects, e.g., comments, vocabularies, and potentially user avatars). Option for two image versions for each object: (a) for listings (generally, thumbnail versions) and (b) for full version (e.g., for node objects, display of one or other determined by value of $main variable). So, for instance, thumbnails would be displayed with listings of nodes in a taxonomy, while the full display of an individual node would display the full (potentially, larger) image. Would extending this to include nodes make sense? ------------------------------------------------------------------------ August 21, 2004 - 03:35 : Jeremy(a)kerneltrap.org As is, the module does exactly what I need it to do, so I'm very hesitant to add functionality. However, the first idea on your list does interest me. I like the idea of being able to associate an image to a specific node, as opposed to only a taxonomy term. I would be interested in seeing a patch that accomplishes this without making the user interface exceedingly difficult. (ie, where would the administrative interface go for listing/editing/deleting these images go?) ------------------------------------------------------------------------ August 23, 2004 - 19:17 : nedjo I strongly support inclusion of this functionality in core. Yet I think it would be worth the time to genericize image association so that it can be used with multiple object types (at a minimum, terms, vocabularies, and nodes) and multiple image categories (at a minimum, thumbnail and full). Hard-coding this to taxonomies would have the disadvantage of requiring different, partially duplicated systems for associating images with other object types. Given the sound design of taxonomy_image, this additional functionality could be added with a minimum of additional code. I'm working on proposed revisions. Meantime, a few comments: /where would the administrative interface go for listing/editing/deleting these images go/. This would be done, as in node_image, directly in the node editing form via a _nodeapi() hook. Would require two additional fields in module table: CREATE TABLE associated_image ( iid int(10) unsigned NOT NULL, type varchar(255) NULL, class varchar(255) NULL, path varchar(255) NULL, PRIMARY KEY (iid) ); To include node functionality, the main function would become: <?php /** * Call this function from your theme or other php code to display the * image associated with the given object id. An html <img> tag will be returned * if an image is found. The format of the link can be modified with the * tags parameter. * * @param int $id id of term, node, or other object. * @param string $type type of object image associated with, e.g., 'term', 'node' * @param string $class class of image, e.g., 'thumb' (for thumbnail version), 'full' * @param string $tags optional tags to add into the <img src=''> link * * @return string An html <img src> link. */ function associated_image_display($id, $type = 'term', $class = 'full', $tags = NULL) { global $user; if (user_access('access associated images') && !$user->associated_image_disable_images) { // do lookup, return full display path if ($image = db_fetch_object(db_query('SELECT path FROM {associated_image} WHERE type = %s AND class = %s AND iid = %d', $type, $class, $id))) { $image->url = file_create_url($image->path); } else if (variable_get('associated_image_recursive', 0)) { // walk up the taxonomy hierarchy and look for an image if($type == 'node') { $tids = taxonomy_node_get_terms($node->nid); $tid = $tids[0]->tid; } if ($image = db_fetch_object(db_query('SELECT path FROM {associated_image} WHERE type = %s AND class = %s AND iid = %d', 'term', $class, $tid))) { $image->url = file_create_url($image->path); } else { while ($parent = db_fetch_object(db_query('SELECT t.tid FROM {term_hierarchy} h, {term_data} t WHERE h.parent = t.tid AND h.tid = %d ORDER BY weight, name', $tid))) { $tid = $parent->tid; if ($image = db_fetch_object(db_query('SELECT path FROM {associated_image} WHERE type = %s AND class = %s AND iid = %d', 'term', $class, $tid))) { // we found a parent with a configured image, use it $image->url = file_create_url($image->path); break; } } } } if ($image->url) { list($image->width, $image->height) = getimagesize($image->path); // handle image resizing switch (variable_get('associated_image_resize_' . $class, 0)) { case 3: // exact if ($width = variable_get('associated_image_width_' . $class, 0)) $image->width = $width; if ($height = variable_get('associated_image_height_' . $class, 0)) $image->height = $height; break; case 2: // not less than if (($width = variable_get('associated_image_width_' . $class, 0)) && ($width > $image->width)) { $width_scale = $image->width / $width; } if (($height = variable_get('associated_image_height_' . $class, 0)) && ($height > $image->height)) { $height_scale = $image->height / $height; } if ($height_scale || $width_scale) { if ($width_scale && $height_scale) $scale = min($width_scale, $height_scale); else $scale = $width_scale ? $width_scale : $height_scale; $image->height = $image->height / $scale; $image->width = $image->width / $scale; } break; case 1: // not greater than if (($width = variable_get('associated_image_width_' . $class, 0)) && ($width < $image->width)) { $width_scale = $image->width / $width; } if (($height = variable_get('associated_image_height_' . $class, 0)) && ($height < $image->height)) { $height_scale = $image->height / $height; } if ($height_scale || $width_scale) { $scale = max($width_scale, $height_scale); $image->height = $image->height / $scale; $image->width = $image->width / $scale; } break; } return "<img src='$image->url' width='$image->width' height='$image->height' $tags />"; } } return ''; } ?> -- View: http://drupal.org/node/8188 Edit: http://drupal.org/project/comments/add/8188

1 0

[drupal-devel] [bug] Watchdog does not display log messages for deleted users
by Steven 05 Feb '05

05 Feb '05

Project: Drupal Version: cvs Component: watchdog.module Category: bug reports Priority: normal Assigned to: Morbus Iff Reported by: felixz Updated by: Steven Status: patch I don't think this patch applies anymore. Perhaps we could instead keep at least a minimal record of deleted users (i.e. name) to avoid this problem instead? I agree with Dries that "deleted user" is not very friendly. But I hate sites that don't delete you properly. Still, it would seem in the best interest to keep the partial user record around considering other modules. E.g. tracking a deleted user might be useful. Steven Previous comments: ------------------------------------------------------------------------ April 27, 2004 - 20:32 : felixz watchdog_view function will do inner join with user table when selecting messages for viewing. I think its not right way to do it. If user was deleted error messages for that user will be hidden from administrators view. I think it should do a right joing and if uid is NULL then display (deleted) in the user collum for the message. Why do i think this is important here is my explanation. I am developing module. I deleted Anonymous User by accident then I could not see any error logs that are generated by cron jobs. ------------------------------------------------------------------------ July 1, 2004 - 02:56 : Morbus Iff Attachment: http://drupal.org/files/issues/watchdog_deleted.patch (2.54 KB) I've attached a patch that handles this, both in _overview and _view - events assigned to users that were deleted will now show the translatable "(deleted user)" under the User column. ------------------------------------------------------------------------ July 6, 2004 - 18:48 : Dries Normally, users are not able to delete the anonymous user. ------------------------------------------------------------------------ July 6, 2004 - 23:33 : Morbus Iff Dries - changing this back. The patch in question has nothing to do with the Anonymous User (the original submitter's mention of it is a red herring, and should be ignored - this has nothing to do with anonymity). The original submitter describes a situation thusly: a) Assume user #56 logs in. b) Assume user #56 does something watchdoggable (login, comment, etc.). c) Assume user #56 is deleted. Now, those watchdoggable items are no longer shown. In essence, the title: watchdog does not display log messages for deleted users. ------------------------------------------------------------------------ July 16, 2004 - 22:28 : Morbus Iff Dries - any thoughts on this? ------------------------------------------------------------------------ August 18, 2004 - 17:21 : Morbus Iff I'm turning this into critical, in hopes of getting it into 4.5.0. Say User #54 is being a prick and leaving zillions of comment spams. You delete User #54 cos he sucks. A week later, you notice another User has started leaving comment spams, and you have an inkling it's the same guy - the times he leaves the comments coincide with "when the brat comes home from school". Unfortunately, however, since User #54 ("where are you!?!") is deleted, you won't see any of their related log entries (even though they're still in the database), and thus, you have no way of confirming it's the same person. ------------------------------------------------------------------------ August 18, 2004 - 20:39 : Dries This is not a critical feature: you are the first person to request it. I'm not happy with the way this patch presents information. Adding 'deleted user' makes things look cluttered. I'd simply display the name of the deleted user like we do elsewhere (consistency). ------------------------------------------------------------------------ August 25, 2004 - 15:18 : Morbus Iff If the user is deleted, the only thing we have to go on is the saved uid from the watchdog table, right? How would we get the user's username if said user doesn't exist (for a proper uid lookup?). In fact, the absence of the record in the users table is the only reason why this Issue was created - the JOIN was failing because of it. -- View: http://drupal.org/node/7428 Edit: http://drupal.org/project/comments/add/7428

1 0

[drupal-devel] [feature] Where should RSS2 and RDF feeds be linked?
by Steven 05 Feb '05

05 Feb '05

Project: Drupal Version: 4.4.2 Component: other Category: feature requests Priority: normal Assigned to: Nick Nassar Reported by: Nick Nassar Updated by: Steven Status: patch How does this patch fit in with the aggregator/feed changes for extensible feeds, Steven Previous comments: ------------------------------------------------------------------------ August 19, 2004 - 23:37 : Nick Nassar One major stumbling block we've come across in moving DownhillBattle.org from WordPress to Drupal is that WordPress supports RDF, RSS.92, RSS2, and ATOM feeds. We have lots of people using each of the different types of feeds, and we don't want to disrupt them. Drupal core only supports RSS.92, with a contributed module for ATOM support. Support for RSS2 and RDF feeds would make it much easier for many people to switch to Drupal. ------------------------------------------------------------------------ August 20, 2004 - 20:03 : Nick Nassar The RSS.92 feed is located at node/feed. The custom module for ATOM feeds is locates them at atom/feed. If we follow the convention set by the atom feed module, the new feeds should go in rss2/feed and rdf/feed. The other logical option seems to be linking the new feeds at node/feed/rss2 and node/feed/rdf. Any preferences either way? ------------------------------------------------------------------------ August 25, 2004 - 17:58 : Nick Nassar Attachment: http://drupal.org/files/issues/feeds.tar.gz (1.91 KB) Here are really basic RSS2 and RDF modules that link from /rss2 and /rdf ------------------------------------------------------------------------ August 25, 2004 - 19:36 : erikhopp is this patch applied? fyi: if you add a patch to an open issue, you should mark it status:patch, then when it is approved, the approver sets it to status:fixed. sorry if it actually is applied, but i don't see any indication that it is on the devel list... i hope the downhillbattle.org stuff is going well! erik ------------------------------------------------------------------------ August 26, 2004 - 06:27 : Anonymous Are these module suitable for CVS version? ------------------------------------------------------------------------ August 27, 2004 - 01:06 : Anonymous I found these module can't pass the Feed Validator, problems are on the time tag, have futher pattch more? thansk. I really think it should be include in 4..5 core. ------------------------------------------------------------------------ September 1, 2004 - 23:13 : Nick Nassar Changing status to active ------------------------------------------------------------------------ September 2, 2004 - 01:13 : Nick Nassar Attachment: http://drupal.org/files/issues/feeds.tar_0.gz (1.95 KB) The feeds now pass the Feed Validator. ------------------------------------------------------------------------ September 2, 2004 - 02:29 : drumm I have to say -1. Sorry I haven't been able to be more involved in this issue, but I can tell you what I don't like about the current implmentation: -makes 2 places to maintain code (node views and feeds) into four -makes 2 levels of feeds, core node functionality and addon modules -the feeds still aren't extensible (although that could be another issue) -this only adds feeds for the front page when there are many more feeds -its not for drupal cvs Having said that, this code would be perfect in the contributinos CVS repository in sandbox/yourusername/... It should be published, but I think we can do better before Drupal 4.6 (most realistic place to target this functionality for Drupal core), so it doesn't need to be a full module. I will have a chance to describe my ideas in some detail on the drupal-devel mailing list at some point, maybe sooner if I get poked enough. -- View: http://drupal.org/node/10203 Edit: http://drupal.org/project/comments/add/10203

1 0

[drupal-devel] [task] Make posting permission more granular.
by Steven 05 Feb '05

05 Feb '05

Project: Drupal Version: cvs Component: comment.module Category: tasks Priority: normal Assigned to: killes(a)www.drop.org Reported by: killes(a)www.drop.org Updated by: Steven Status: patch Is this patch still needed? Doesn't it tie comment.module unnecessarily into node.module? Steven Previous comments: ------------------------------------------------------------------------ August 29, 2004 - 07:55 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node-access-comment.patch (5 KB) There have been requests to private forums. I think the most straightforward way to allow them is by extending the node_access hook. The attached patch tries this. It assumes that my node object caching patch got committed. Afterwards loading a node twice won't be a performance issue anymore. The patch is untested, I'd like to get feedback on the implementation. ------------------------------------------------------------------------ August 31, 2004 - 17:53 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node-access-comment_0.patch (5.02 KB) The patch is now tested. I think this needs a better explanations: The patch isn't only for forums (which still need some kind of taxonomy permissions) but applies to all nodes. You can set individual nodes to be commentable through the _access hook. This would be helpfull for a forum where only a selected role should be allowed to post, but others should read the announcements. ------------------------------------------------------------------------ September 4, 2004 - 09:57 : killes(a)www.drop.org The patch has a small problem: We need either to move this check <?php if ($op == 'comment' && $node->comment == 2 && user_access('post comments')) { return TRUE; } ?> before the db query (and thus taking away a bit of the power from that patch) or add a grant_comment column to the node_access table: alter table node_access add grant_comment tinyint(1) unsigned NOT NULL default '0'; ------------------------------------------------------------------------ September 5, 2004 - 11:42 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node-access-comment_0_0.patch (4.78 KB) Keeping up with CVS. I'd like to receive some feedback on the resolution of the problme mentioned above. ------------------------------------------------------------------------ September 13, 2004 - 03:25 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/module_0.patch (7.1 KB) Keeping up with cvs. Node modules should implement _access('comment'). ------------------------------------------------------------------------ September 14, 2004 - 07:38 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/commentable.patch (7.89 KB) Had forgotten blog.module -- View: http://drupal.org/node/10426 Edit: http://drupal.org/project/comments/add/10426

1 0

[drupal-devel] [feature] generalizing node_rewrite_sql
by chx 05 Feb '05

05 Feb '05

Project: Drupal Version: cvs Component: database system Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/db_rewrite_sql.patch (7.66 KB) As I was thinking on the various usages of node_rewrite_sql , it came to my mind that with very little changes it can be generalized so that any queries against any table -- not just node -- can be rewritten. I have moved node_rewrite_sql to database.inc, renamed it db_rewrite_sql, introduced another necessary parameter, which does not affect the current (light) usage of node_rewrite_sql. I think the usage is even clearer now, 'cos I think nid_alias caused some confusion. Now it is called $primary_table , while the new parameter is called $primary_key. What this would be good for? One may want to have permission and languages for taxonomy terms and vocabularies. chx -- View: http://drupal.org/node/16111 Edit: http://drupal.org/project/comments/add/16111

7 19