March 2005 - Development - lists.drupal.org

[drupal-devel] [feature] Improve functionality of block generation for book module
by killes 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/14120 Project: Drupal Version: cvs Component: book.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: nysus Updated by: killes(a)www.drop.org Status: patch Still applies. Andre: Please create patches from the Drupal root directory. killes(a)www.drop.org 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. ------------------------------------------------------------------------ February 5, 2005 - 08:31 : Steven 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. ------------------------------------------------------------------------ February 5, 2005 - 23:56 : andremolnar I am fixing up the code as we speak. I actually made a bigger faux pas by not escaping my INSERT sql string properly - (There is a quick fix, but I understand 'addslashes' is not good enough for drupal ;-). (Thank you to Dries for catching this right away on his test of the demo site). As for multiple books - you actually have more flexibility to enable as many book menus as you like - not only based on type but also by path. e.g. Could allow for: drupal.org/handbook (with some general book menu blocks enabled) drupal.org/handbook/developer (with developer book menu blocks enabled) drupal.org/handbook/developer/theme (with theme developer book menu blocks enabled) etc. (the sky is the limit). I will resubmit a patch with all of the coding style fixes requested - and I will be more careful with the coding style in the future. andre ------------------------------------------------------------------------ February 6, 2005 - 01:05 : andremolnar Attachment: http://drupal.org/files/issues/book_module_10.patch (7.7 KB) Updated Patch, Implemented code style improvements as per Steven. Fixed unescaped sql INSERT string (turns out that addslashes is more than enough in this situation, because the 'title' string has already been valided and properly inserted into the database before any of this code ever runs - i.e. this isn't coming directly from user input, but rather from the drupal database). Demo site is once again live and welcomes all testers (Thank you again to those that have already tested this out). andre

1 0

[drupal-devel] [bug] Menu cache is not locale aware
by killes 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/13503 Project: Drupal Version: cvs Component: menu system Category: bug reports Priority: normal Assigned to: chx Reported by: chx Updated by: killes(a)www.drop.org Status: patch I agree, this should get patched. Still applies. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ November 26, 2004 - 08:13 : chx Attachment: http://drupal.org/files/issues/menu_locale.patch (522 bytes) If you have a multi language site, and change languages, the menu is already cached, thus it can not display the translated version. This simple patch makes the menu cache locale aware. Do I need to file the same patch against HEAD? 'Cos it applies, too. ------------------------------------------------------------------------ November 26, 2004 - 10:13 : Dries Isn't it better to flush the menu cache when someone (or the admin) switches languages? Keeps the cache size down. ------------------------------------------------------------------------ November 26, 2004 - 12:20 : chx There could be several points where the language is changed. i18n for example displays a language selection block which lets you switch anytime. Thus I think this solution is the best. ------------------------------------------------------------------------ November 26, 2004 - 13:26 : Goba See also this issue [1], which is still (again) open. The locale is also changed, when the admin changes it as the default on the admin interface, and the menu cache needs to be flushed then, and the admin needs to be redirected. I don't know how these problems are solved currently, so I would only be able to submit a placebo patch to make that issue pop up again... [1] http://drupal.org/node/11312 ------------------------------------------------------------------------ November 26, 2004 - 14:28 : Bèr Kessels I solved it by adding some logic to the swithcer function in i18n: when a language is switched the cache is flushed. The negative side of this, is that if you have hundred various roles of users reading English and one switching to German, the complete cahce is flushed. But then again, who has so many roles, that this might become an issue? ------------------------------------------------------------------------ November 27, 2004 - 10:59 : Dries I just fixed bug 11312 [2]. I don't see what's wrong with invalidating the user's menu cache when he or she switches language. Just do: <?php cache_clear_all("menu:$user->uid"); ?> [2] http://drupal.org/node/11312 ------------------------------------------------------------------------ January 30, 2005 - 20:26 : Jose A Reyero This is needed also for i18n. Invalidating cache is not a solution here, as we would need to do it also for anonymous users. So I think this is a simple solution and will work fine for both cases, user/locale and i18n. ------------------------------------------------------------------------ January 31, 2005 - 16:15 : Bèr Kessels I found out what is wrong with this: Swithcing and cache are two completely differetn things: The language is a per-session setting. The cache is a per-role setting. So any user can switch langauge on any moment, resulting in a cahce that will get flushed on nearly every page-load. And since roles and sessions are tow different thingsm we should not flush a role based cache because a variable in the session changes. The only correct option would be to have per-locale cache. (as per Jose's patch) ------------------------------------------------------------------------ January 31, 2005 - 16:27 : Bèr Kessels Correction, It was not José that submitteed the patch for locale aware menu's but Károly Négyesi, I got confused because José was the last to comment on that patch. ------------------------------------------------------------------------ February 2, 2005 - 21:44 : moshe weitzman i can't make heads or tails of this patch request. it seems that some folks are confusing menu cache and page cache. and we don't even have a role cache. dries' question remains unanaswered. why can we not just rebuild a user's menu cache when needed? ------------------------------------------------------------------------ February 2, 2005 - 22:17 : Jose A Reyero /why can we not just rebuild a user's menu cache when needed?/ This way you cannot have menus in multiples languages for anonymous users, unles they're not cached at all. ------------------------------------------------------------------------ February 2, 2005 - 22:27 : moshe weitzman Aha! Makes sense now. +1 for this simple patch.

1 0

[drupal-devel] Bug patch for forum.module
by Steve Dondley 13 Mar '05

13 Mar '05

I reported a bug and supplied a patch here at http://drupal.org/node/18787 I'm reposting it because it's gone over 24 hours without a response so I'm worried it might have been overlooked. Sorry for the extra traffic.

3 3

[drupal-devel] [feature] Proposed change to menu.inc that would allow modules to create custom menus
by killes 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/15198 Project: Drupal Version: cvs Component: menu system Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: killes(a)www.drop.org Status: patch Still applies. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ January 6, 2005 - 08:13 : andremolnar Attachment: http://drupal.org/files/issues/menu_inc_2.patch (2.65 KB) See http://drupal.org/node/15153 and http://drupal.org/node/14120 for discussion on this proposed change. Patch is included here for further discussion (with the changes suggested by Dries). A patch is forthcoming for topics discussed in http://drupal.org/node/14120 which would make use of this patch. Status to be changed to patch later if there are no objections following discussion. andre ------------------------------------------------------------------------ January 8, 2005 - 22:16 : andremolnar Patch making use of this is now added to http://drupal.org/node/14120. andre ------------------------------------------------------------------------ January 10, 2005 - 08:29 : andremolnar Just realising that the rules for setting status to patch would allow this to be patch since its technically ready to go. (and it might actually get noticed if its on the dev list and patch queue). andre ------------------------------------------------------------------------ January 11, 2005 - 07:47 : Dries I'm OK with this patch but won't commit it yet until the book module patch is ready. ------------------------------------------------------------------------ January 20, 2005 - 03:07 : andremolnar I've uploaded the latest version of the patch to book. http://drupal.org/node/14120 andre ------------------------------------------------------------------------ January 27, 2005 - 18:38 : JonBob I'd prefer a solution that allowed modules to declare such menu items using the menu hook, but haven't been able to come up with such a solution. I'm okay with this approach until such time as someone has a brilliant idea to make the menu hook more powerful. ------------------------------------------------------------------------ January 28, 2005 - 01:00 : andremolnar JonBob, I'd be happy to work on a solution with you. Right now what I had to do in the proposed changes to book was build functions to insert data into the {menu} table by hand since there was no other handler available (e.g. hook_menu). And proposing this MENU_MODULE_MENU type was the only way I could think of to clearly distinguish where these new menu items were coming from (i.e. a module doing an insert into the menu system outside of the normal chanels). Would the creation of a new hook_ be in order. Perhaps hook_menu_block? While it would be a confusing hook name - it is does describe the desired outcome. i.e. Output from hook_menu_block would be inserted into the menu system and a block would be created for that menu. But even then I think these new menu types (or something similar) would have to be employed in some way. andre ------------------------------------------------------------------------ February 7, 2005 - 08:08 : andremolnar Attachment: http://drupal.org/files/issues/menu_inc_3.patch (2.65 KB) Noticed patch was out of date - keeping it current.

1 0

[drupal-devel] [bug] User feedback if commenting on a closed node
by killes 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/10417 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: killes(a)www.drop.org Reported by: killes(a)www.drop.org Updated by: killes(a)www.drop.org Status: patch Attachment: http://drupal.org/files/issues/comment_2_0.diff (723 bytes) Updated to cvs. Better wording. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ August 28, 2004 - 20:37 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/comment_2.diff (701 bytes) If a user (for whatever reason) tries to post to a node which doesn't allow this, he currently doesn't get an error message. This patch fixes this. ------------------------------------------------------------------------ September 5, 2004 - 08:02 : killes(a)www.drop.org recategorizing to show up in the patch queue. ------------------------------------------------------------------------ September 14, 2004 - 03:42 : Steven I'm not 100% sure about this patch. We don't normally cater to inaccessible forms. I suppose the comment form is a candidate, when a moderator disables commenting on a (heated?) topic while people are still typing replies. But are there more probably situations like this out there? ------------------------------------------------------------------------ September 14, 2004 - 07:37 : killes(a)www.drop.org As you mentioned the comment form isn't neccessarily inaccessible, the sending of the comment might become later. We should present a meaningfull message to the user in that case. I am developing a module where you can only comment for a limited amount of time, so no message is actually confusing. ------------------------------------------------------------------------ September 14, 2004 - 22:03 : Dries I don't think this needs fixing. ------------------------------------------------------------------------ September 14, 2004 - 22:17 : killes(a)www.drop.org I disagree. The user doesn't get a 403 or anything, just an empty "Post comment" page.Tha's bad usability. ------------------------------------------------------------------------ October 14, 2004 - 23:54 : drumm +1 Although it is a rare case, this is a really simple fix that might as well go in.

1 0

[drupal-devel] [feature] Show link indicating that a node has attachments on the homepage
by killes 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/12211 Project: Drupal Version: cvs Component: upload.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: simon_g Updated by: killes(a)www.drop.org Status: patch Sounds usefull. And still applies. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ October 27, 2004 - 22:17 : simon_g Attachment: http://drupal.org/files/issues/upload.module.diff (1.53 KB) I've made a small change to the upload module to make it possible to show a link under the teaser text on the homepage similar to the one shown by the comments module to indicate that a node has attachments. I find this feature particularly useful if you create an article that is so small that all text does fit in the teaser on the homepage. In that case there will be no 'read more' link under the article, if you've added an attachment a user scanning the homepage will not notice it's existence. This feature is configurable by the admin - default value is off, but maybe it should be on. I hope someone who has CVS access will commit this patch - or notify me if it should be changed in some way to get it accepted. (Sidenote: This is my first Drupal modification and I don't do a lot of php programming. This is just a simple bit of cut-and-paste programming so please tell me if it can be improved) ------------------------------------------------------------------------ October 27, 2004 - 22:42 : Bèr Kessels two minor things: adding more configuration options is considered bad: unless absolutely nessesary you use a tab instead of two spaces before $links[] = drupal coding style only allows spaces. furthermore +1 for this functionality ------------------------------------------------------------------------ October 27, 2004 - 23:00 : simon_g thanks for the quick response! Do you (or anyone else) need a new diff with spaces instead of tabs? I just noticed the diff should have been against the CVS HEAD - I checked it and the line numbers are identical, so it won't be really needed for that. About the number of configuration options: The upload module currently has only one 'global' configuration option (the max size) so I think a second setting will not hurt a lot. I agree that too many options is bad, maybe it's just that I was hesitant about anyone else wanting this functionality. I'll create a patch without the configure options if needed, but how do I know what's the most favored version? (Or is one +1 enough for all?) [I've read a bit in the "Contributers guide" but I'm not really sure about the process. ------------------------------------------------------------------------ October 27, 2004 - 23:17 : Bèr Kessels I am not in charge, at all, of any patches that might go to core. I merely gave my ideas, so that if a core cvs maintainer passes by he will notice that the patch was reviewed already, and then might easier decide to commit it. Bèr ------------------------------------------------------------------------ October 29, 2004 - 00:47 : Steven Wouldn't it be better to have a little attachment/paperclip icon instead? The link is more so informative rather than for clicking. ------------------------------------------------------------------------ October 30, 2004 - 00:49 : simon_g I like the idea - but at what location should that icon be shown? At the same place the text will display now? And I think the link certainly is for clicking if the text is short and therefore there is no "read more" link. I know you can click the title (at least in the bluemarine theme you can) but that doesn't really feel intuitive to me. (Probably having something to do with moving your mouse up from where you were reading instead of down in the 'normal' reading direction) And I don't see a lot of images being used in the rest of Drupal - how would something like that interact with the templating part? Are there examples of modules using images in the user-interface part? Simon ------------------------------------------------------------------------ October 31, 2004 - 07:08 : Boris Mann This should be added as a regular link, ideally something like "download attachment (filename.xxx)". BUT, I agree that at the theme level you would want to easily style ANY of those links as an icon. But...we currently can't order or edit or replace-with-icons any of those node links. So add the link as plain text, then lets get to work on some theming improvements. ------------------------------------------------------------------------ November 1, 2004 - 16:30 : simon_g I wouldn't want to have links to all the attachments on the homepage, if you have more than one attachment that would clutter the homepage too much I'd think -- My patch is only meant to make the user aware of the existence of the attachtment(s) so they will know there is more to see if they click the article. There is already a nice table of attachments in the page of the article itself, if you really want all the info on the frontpage you could just as well include that table in the teaser text. ------------------------------------------------------------------------ December 3, 2004 - 04:25 : mathias Attachment: http://drupal.org/files/issues/upload_attachment_node_link.patch (1.35 KB) +1 This feature makes it possible to know what nodes have attachments when multiple nodes are shown on a page in 'teaser mode'. This is a great usability feature especially when one is making reference to an attached file in node->teaser that currently is only visible at the node/nid/view level. This new patch applies to CVS HEAD and removes the setting of making this feature optional. I don't think that's necessary given that it's a usability issue and not a feature that changes the way the module behaves and treats files. ------------------------------------------------------------------------ December 3, 2004 - 04:55 : mathias Attachment: http://drupal.org/files/issues/upload_attachment_node_link_0.patch (1.49 KB) Slight revision to my last patch. Only show the attachments link when an attached file has the 'list' option checked. It doesn't make sense to say that attachments exist if none are listed.

1 0

[drupal-devel] [feature] introducing theme_term_link()
by killes 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/12154 Project: Drupal Version: 4.5.0 Component: taxonomy.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Bèr Kessels Updated by: killes(a)www.drop.org Status: patch What did the usability meeting say about this? killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ October 26, 2004 - 21:40 : Bèr Kessels Attachment: http://drupal.org/files/issues/drupal_84 (1.28 KB) There currently is *no* way to theme a term link. Thats not good [tm]. Because I think this is not just a simple featurem, but required for good themeing (note that even "silly-er" functions have extremely complex theme functions) I categorise it as a Bug and not a task or a feature! This patch allows themes to override the term-links by themeing a very simple function. ------------------------------------------------------------------------ October 26, 2004 - 23:02 : moshe weitzman why would someone want to theme a term link? how is this different from links to users or links to nodes. ------------------------------------------------------------------------ October 26, 2004 - 23:25 : Bèr Kessels I want it for one: <?php function remixreading_term_link($term) { return taxonomy_image_display($term->tid)." ".l($term->name, 'taxonomy/term/'. $term->tid); } ?> and other might want fancy opportunities here: <?php function mytheme_term_link($term) { if(in_array($term->vid, $array_of_fancy_vocs)) { return '<div id=voc-'.$term->vid.'">'.l($term->name, 'taxonomy/term/'. $term->tid).'</div>; } else { return l($term->name, 'taxonomy/term/'. $term->tid); } } ?> so Yes!, i believe its areal useable issue here! ------------------------------------------------------------------------ October 27, 2004 - 01:12 : drumm Setting status to patch. +1 for what is here and another request: The image and forum modules should be able to make the links go to the image and forum modules. Although this could easily fit into http://drupal.org/node/10425. ------------------------------------------------------------------------ October 27, 2004 - 06:50 : Eric Scouten "The image and forum modules should be able to make the links go to the image and forum modules. " I strongly agree here. Related suggestion: I'd like it if taxonomy_menu could "hijack" the term listings so that clicking on a term caused the menus to update properly. (IOW, the term link should be to taxonomy_menu/(vid)/(tid), not to taxonomy/(tid). This may or may not fit with the patch provided here. (I didn't read the patch.) ------------------------------------------------------------------------ October 27, 2004 - 13:28 : moshe weitzman i guess folks want image and forum links to be rewritten because thos modules output their nodes using a display that differs from node/x. that is image/x looks different from node/x. the right way to fix that is to do what we did for book.module. unify the look of image/x and node/x such that we no longer have an imagex. see how book.module accomplishes this. i am still -1 for this patch since i see many other types of links that could be themed in this way (users, forum nodes, recipe nodes, polls, etc.). on the other hand, it is a pretty small change. ------------------------------------------------------------------------ October 27, 2004 - 13:30 : moshe weitzman i guess folks want image and forum links to be rewritten because thos modules output their nodes using a display that differs from node/x. that is image/x looks different from node/x. the right way to fix that is to do what we did for book.module. unify the look of image/x and node/x such that we no longer have an imagex. see how book.module accomplishes this. i am still -1 for this patch since i see many other types of links that could be themed in this way (users, forum nodes, recipe nodes, polls, etc.). on the other hand, it is a pretty small change. ------------------------------------------------------------------------ October 27, 2004 - 18:39 : Eric Scouten No, false assumption. There is no image/x URL for displaying a single image. The issue is not how to display a *single* node (we all agree that node/x is correct), but how to display a *list* of nodes. For certain kinds of nodes, there are far better ways to do it than offered by taxonomy_render_nodes, so it makes sense to allow an override of that list display. (Note: This is straying from the original discussion. If I didn't have to leave ASAP, I'd start a new thread.) ------------------------------------------------------------------------ November 15, 2004 - 11:47 : Bèr Kessels bringing this back to the original discussion: pass the taxonomy links trough a theme function, so that people can, for example, add icons to the links. The patch sill applies (hint,hint!) ------------------------------------------------------------------------ November 15, 2004 - 16:24 : moshe weitzman Perhaps we do this themeing not for every taxo link but rather for lists of links. In other words, do like theme_node_list() and theme_user_list(). more consistent with existing practice.

1 0

[drupal-devel] [bug] [pgsql] location field in locales_source is the wrong field type.,
by killes 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/11689 Project: Drupal -Version: 4.5.0 +Version: cvs -Component: postgresql database +Component: locale.module Category: bug reports Priority: normal -Assigned to: adrian +Assigned to: killes(a)www.drop.org Reported by: adrian Updated by: killes(a)www.drop.org Status: patch Attachment: http://drupal.org/files/issues/locale-inc.patch (739 bytes) How about this? killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ October 18, 2004 - 17:13 : adrian Attachment: http://drupal.org/files/issues/pgsql_locales_fix.diff (2.09 KB) here's the patch ------------------------------------------------------------------------ October 18, 2004 - 17:39 : killes(a)www.drop.org The patch should not be neccessary becuase the location field is pruned to contain 128 chars. But - at least for pgsql- it doesn#t hurt either. ------------------------------------------------------------------------ October 18, 2004 - 17:57 : adrian here is an updated patch, which fixes the problem with too many unique restraints on locales_target. ------------------------------------------------------------------------ October 18, 2004 - 17:59 : adrian Attachment: http://drupal.org/files/issues/pgsql_taxo_bug_0.diff (1.96 KB) err. here is the patch. ------------------------------------------------------------------------ October 18, 2004 - 19:17 : adrian Attachment: http://drupal.org/files/issues/pgsql_locales_fix_0.diff (2.64 KB) here is a corrected patch, it contained some cruft fromt he taxonomy patch i made earlier ------------------------------------------------------------------------ October 18, 2004 - 20:53 : Goba Killes, the 128 char check is improperly implemented (the length is checked before a new substring of unknown length is added), so that check needs to be fixed in the first place. ------------------------------------------------------------------------ October 18, 2004 - 20:55 : Goba Killes, the 128 char check is improperly implemented (the length is checked before a new substring of unknown length is added), so that check needs to be fixed in the first place.

1 0

[drupal-devel] [feature] move check_query() into the database interface files
by chx 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/13180 Project: Drupal Version: cvs Component: database system Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch I can only repeat what Goba said: . mysql_real_escape_string uses the connections charset settings to properly escape, so it is the best (but is not available in old PHP versions). chx Previous comments: ------------------------------------------------------------------------ November 19, 2004 - 20:35 : chx Attachment: http://drupal.org/files/issues/check_output_patch.txt (1.54 KB) The check_query documentation says 'Prepare user input for use in a database query, preventing SQL injection attacks.' But the code itself is a simple addslashes. This is simply not correct -- for mysql, yes, but the world is not just mysql. There are _escape_string functions for that: mysql_escape_string, pg_escape_string, sqlite_escape_string... Read the manual for pg_escape_string: "Use of this function is recommended instead of addslashes()." Or read sqlite_escape_string: "addslashes() should NOT be used to quote your strings for SQLite queries; it will lead to strange results when retrieving your data." Ooooops. So I propose moving check_query() declaration from bootstrap.inc to database.*.inc. I've already written a letter to the devel list about this, but drumm said "code is better than talk" so here is a patch. At this moment, this does absolutely nothing, only opens the possibility to use the aforementioned escape_string functions. But database.sqlite.inc simply will not work without this change. ------------------------------------------------------------------------ November 19, 2004 - 20:42 : drumm Looks straightfoward enough. Is there any reason to not put in pg_escape_string() now? ------------------------------------------------------------------------ November 19, 2004 - 20:47 : chx Actually, yes. Read on the mentioned manual page: "Note: This function requires PostgreSQL 7.2 or later." I have no idea how the check it. There is a pg_version() but I will leave this postgres business to someone who has a working postgres install to code and check this thing. ------------------------------------------------------------------------ November 19, 2004 - 21:58 : Chris Johnson PostgreSQL 7.2 was released February 4, 2002. Major production releases 73. and 7.4, along with many point releases, have been issued since then. Current production is 7.4.6 and 8.0 is in the 4th revision of beta. It seems pretty safe to assume that any Drupal installation using PostgreSQL would have version 7.2 or later. Document if a user encounters errors with PostgreSQL that they need to check the version is 7.2 or later. ------------------------------------------------------------------------ November 20, 2004 - 14:42 : Dries +1 I suggest creating a db_escape_string() function, and removing check_query() all together. ------------------------------------------------------------------------ November 20, 2004 - 14:55 : Goba +1 on Dries ------------------------------------------------------------------------ November 20, 2004 - 18:32 : chx Attachment: http://drupal.org/files/issues/check_output_patch_0.txt (1.6 KB) db_escape_string sounds find to me, it's really a better name, thanks Dries. But removing check_query? I'd suggest deprecating it and removing it later. I am afraid It would break a lot of custom modules. Here is a slightly modified patch with pg_escape_string included and documented. ------------------------------------------------------------------------ November 20, 2004 - 18:43 : killes(a)www.drop.org We were never afraid of breakign things for the good of the project. Please don't change this policy. ------------------------------------------------------------------------ November 20, 2004 - 19:26 : chx Attachment: http://drupal.org/files/issues/db_escape_string.txt (20.87 KB) Let's do it. ------------------------------------------------------------------------ November 20, 2004 - 19:29 : chx Attachment: http://drupal.org/files/issues/db_escape_string_0.txt (20.88 KB) Something funny happened to the PostgreSQL db_escape_string comment and I have missed it, sorry. Reposting the patch. ------------------------------------------------------------------------ November 21, 2004 - 09:29 : Dries I committed the patch. Thanks chx. Keep it coming. I've also updated the upgrade notes at http://drupal.org/node/12347. I'll update some contributed modules unless someone objects. ------------------------------------------------------------------------ November 21, 2004 - 12:27 : chx Attachment: http://drupal.org/files/issues/mysql_escape_patch.txt (572 bytes) Keep it coming? Well, let's see the MySQL case... It's far from being trivial, and I expect some controversy over it. ------------------------------------------------------------------------ March 10, 2005 - 00:36 : chx Attachment: http://drupal.org/files/issues/mysql_escape_patch_0.txt (573 bytes) OK, here is mysql version a bit corrected, this should get into 4.6 -- addslashes is not a correct way any more. ------------------------------------------------------------------------ March 10, 2005 - 19:29 : Dries Can we read up on that somewhere? ------------------------------------------------------------------------ March 10, 2005 - 19:37 : Goba Dries, sure: http://php.net/mysql_escape_string [1] and http://php.net/mysql_real_escape_string [2]. The latter also uses the connections charset settings to properly escape, so it is the best (but is not available in old PHP versions). The addslashes page user notes also have more information [3]. [1] http://php.net/mysql_escape_string [2] http://php.net/mysql_real_escape_string [3] http://hu.php.net/addslashes#20013 ------------------------------------------------------------------------ March 12, 2005 - 09:50 : Dries addslashes() has been working for ages (and is simpler code-wise). I guess the mysql_ functions are slightly more secure or not even that? That aside, I'm not really a big fan of version_compare()s. Also note that the patch violates Drupal's coding standards (again) but that's easy enough to fix.

1 0

[drupal-devel] [bug] Deleting a node/container causing page "nesting"
by nysus 13 Mar '05

13 Mar '05

Issue status update for http://drupal.org/node/18787 Project: Drupal Version: cvs Component: forum.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: nysus Updated by: nysus Status: patch Attachment: http://drupal.org/files/issues/forum_patch2.patch (265 bytes) Simple fix. Here's the patch. nysus Previous comments: ------------------------------------------------------------------------ March 12, 2005 - 10:48 : nysus Attachment: http://drupal.org/files/issues/screenshot_0.jpg (97.52 KB) When trying to delete a forum or container, multiple instances of a Drupal page appear nested inside of each other when the confirmation of deletion page is displayed. See screenshot.

1 0