improve book module: use nodeapi and menu API

subscribe

here is a slight schema change... bookid => book_id in indexes

subscribing

first try @ an upgrade path

update.php stuff got in...

need node_types_rebuild and menu_rebuild after node type save

tested. Seems to work flawlessly.

This baby is ready to fly. RTBC

This patch is poorly reviewed -- doesn't buy you guys much reviewer points. :/

1. Total lack of code comments in book_form_alter() and book_outline() -- just to name two functions.

2. Coding style issues. For example, in _book_add_form_elemts() we are using tabs.

3. Inconsistent capitalization and punctuation in PHPdoc.

4. I'd prefer to get rid of some of the book terminology. People new to Drupal want to structure pages, not create books. If this is too much work, we can leave that for another patch.

5. I don't quite understand what the $child_type is for?

6. What I don't like about this patch is that the book.module now accesses all of the menu system's internals. It shows that the menu system's API are far from perfect.

7. I'd like to get rid of things like 'Move to another book'. This shouldn't be an explicit operation, IMO. It should be a matter of selecting a new parent in the drop down menu. This patch suffers from feature creep. It introduces the notion of separate books. Please don't. That should go in a separate patch. (We want to remove the notion of a book, not make it more book-y.)

8. I was confused by the name 'book_link'. It doesn't seem to contain what I expected it to contain. What exactly is it for?

9. Shouldn't we be able to remove the mass book administration page? That should probably be part of the menu module now?

Hey, this is looking great. I'll do a more extensive review of your patch soon.

1. It would be great if it would be possible to not only have nodes in a book. One might want to have a view or a panel be part of an outline as well. It would be great if e.g. Views could just somehow add a form defined by book.module to its UI that allows a view to be inserted into a book.

2. Concerning seperate books: This is not required as long as we can easily get the top-level links. IMO, there is no difference between a top-level link and its children and seperate books, but the latter is probably a little more confusing. Concerning the UI, something like two drop-downs would be great, the first listing all top-level links, and the second one the children of the first (active select):

Place in outline [DROPDOWN: before | after | as child of]  [DROPDOWN: top level items, "books"] [DROPDOWN: children | of | selected | book]

Regarding:

7. I'd like to get rid of things like 'Move to another book'.

I find that a great feature of this patch. I've heard both 7 and 12 used as upper limits on the number of items to display in a list before that list should be broken down into smaller pieces. In my use of the book module, the current parent dropdown frequently has hundreds or even thousands of items in it. By splitting the "move to a new book" functionality into a new area the dropdown is much smaller which is a great improvement in usability in my opinion. Just today Ezra and Laura were complaining about the hundreds of items in a book on a knowledgbase site and talking about using form_alter and other custom code to achieve what this patch provides: simplification of the parent dropdown.

One alternative would be to change the UI to use the lumen menu system which allows quick drill down into deeply nested lists.

pwolondin: I think we're trying to do to much in this patch.

Things I'd like to see removed:

1. Move to different book. This should go into a different patch.

2. Child page type. I really do not understand what this is for. The UI is confusion and doesn't give me a clue as why this is useful. Care to elaborate?

What I'd like us to see focus on instead is:

1. Proper integration with the menu system.

Can't you do a generic "add child" that lets me chose from different node types? I don't see why we shouldn't be able to mix different child types easily.

'add child' is poor language compared to 'add product' or 'add policy' (i.e. custom types). furthermore, an 'add child' link would send you to an intermediate page where you choose from available content types and then you would start authoring. bleh. my .02.

I think we need to leave all of these new features out or we risk missing the code freeze. I already mentioned what features I'd like to see removed from this patch.

I think it Dries says cut back on features we should cut back on features. This patch is becoming somewhat huge.

As for the AJAX callback, it is unnecessary IMHO. The way the list of OPTIONS is set up for the select element, it will be totally possible to use jquery to extract a useful data structure and implement an interactive parent-picker. I don't have working JS for this (yet), but this loop lets me extract the options in order and establish their parentage:

$.each($('#edit-book-link-plid-wrapper option'), function(index, option) {
  console.log($(option).html());
});;

If we go back to the old-style UI of having all the options in one select, that would be easiest to enhance with JS I believe.

No need to be frustrated. I disagree with some of the extensions you made, not with the overall direction of this patch. We both want to convert the book module into a powerful outliner that allows you to created structured pages. We share the same goals. :)

You're making it more book-like, ultimately, I want to make it less book-like. Making it less book-like is key if we want this to be _the_ outliner feature. We have to stop using the wrong terminology, and as such, the notion of there being multiple books need to go. We also need to rethink the workflow.

I'd like to do a straight-forward conversion and then focus on how we can add more functionality, in such a way that it is easy and intuitive to use. For example, the proposed 'child_type' feature is currently difficult to understand and use, and therefore a step backward for many users.

I have not had time to track this patch and have not looked at the implementation, so take my comment with the appropriate salt lick. :-) However, I have to agree conceptually with pwolanin. Whether an "outliner" is being used as a book or not, multiple separate "hierarchy things" is an important feature. Drupal.org may only need one hierarchy that it can use for all handbooks, but there are absolutely use cases where you need them separate. For instance, put OGs into a hierarchy and have a separate collection of multi-page handbooks on the same site that are independent of them (which I would use this for). Forcing them to all be one structure is just as limiting as saying that the Navigation Menu (mid 1) is all a site really needs.

i think the only difference between pwolain and dries on this is the terminology, both in the UI and in the code. i (hope) we're all in agreement at separate top-level outlines/containers/books/whatever is a good move. see http://drupal.org/node/81226 for some prior effort/thought on that topic.

also, d.o better not just setup a single "handbooks" outline, or the UI benefits aren't going to be much. we should have separate outlines for each "book" in the handbooks now. then, the "move to a new outline" will make life *much* easier for editing, expanding, and maintaining the d.o handbooks.

if i didn't have too many of my own battles to wage right now, i'd tear into this issue more closely and help get it into D6. but, it seems to be in good hands, so i'll focus my efforts elsewhere...

whoops, actually http://drupal.org/node/65319 was the issue i was talking about, but that ended up getting re-started as http://drupal.org/node/81226. ;) so, both are worth reading for the interested historian...

I have always wanted separate outlines for each book (preferably still be able to move content between books). I have additional hopes for separate permissions for each book as well and can produce several use cases if needed.

/also hasn't had a proper look at the patch so big chunks of salt/

I have some full length books on my site, organised in chapters, quite a lot of them. To me that's book working as essentially a paged article with extra menu and navigation links. But we also have a multi-level quasi-handbook organised with book.module. I've also been following discussions about using the outliner + views to replace the forum index if you were able to add views (and panels) to outlines - which may be an option later on from either end (panels2). Seperate outlines would be a god send to manage that sort of thing.
Also with child types, it'd help a lot to seperate 1920's political texts from HOWTOs, and avoid using "book parent" filters all over the place.

@pwolanin: cool, thanks for the new patch that follows one possible solution to the $node namespace stuff over at http://drupal.org/node/148420. However, I fear it's premature, since we haven't converged on a solution yet (module-based, schema-based, hybrid, etc).

Also, I believe Dries wants this to not retain the name "book" at all. From what I gather from his comments here and in other places, he wants this to be the "outline" module, not the "book" module -- in name, in UI, in the code, and in the $node object. ;) Setting this to needs work since I think a complete renaming is going to be required before this lands. Dries, please correct me if I'm wrong...

I spent another hour with this patch.

When I create a page (like an about page), I expect that I can add it to the navigation menu. I want the book module to integrate with the page node type so I can start building structural pages right from the start. To make that work, the book module needs to integrate with the "create page" form. Use case: create an about page, with a 'staff pages' below that. The about page is of type 'page', the pages of type 'employee' (CCK node type). Thus, the book module works like a very simple view. Either way, I want us to optimize this module for this use case. There should be no special configuration steps required -- the learning curve has to be flat.

Specifically:

1. I think we should enable this module by default. Please add it to the default install profile.
2. The outline-tab must become a fieldset on the node edit page so that I can outline the page upon creation time. Creating a child page should be done in one step, not two steps. Remember we're trying to lower the barrier.
3. Specifically, I'd look at how this can be integrated with the menu module. The menu module already asks you to provide a "parent" and a "weight". The book module ask you the exact same information. Integrating the book and menu module will lower the barrier even more.
4. The "child page type" feature needs to be removed. One should be able to add _any_ node as a child.

Suggestions from a discussion w/pwolanin in IRC:

1) Drop the child page setting. It seems obscure. A better default is to assume the node-type of the parent is the default for "add child page" links.

2) Implement hook_perm so that each "book" (that is, each root) has two permissions: add child pages, and edit outlines. This will determine whether a user gets the add child page link, and whether or not they can move nodes around, into or out of the outline from some other interface.

3) In terms of putting an outline fieldset on the node/add and node/edit interface, there's an issue of what to do for sites that have huge numbers of book pages. The arbitrary yardstick is drupal's ~10,000 pages. One school of thought is to have a 2-part interaction for moving nodes between books (similar to upload.module's handling of file-uploads), another is that we have to solve the 10,000 node problem sooner or later anyway.

More on permissions:

If we assume that child-pages are the same (by default) as the root node, and we implement a per-book permission to add them, there could be a conflict where a user role has the "add child page" permission for a book, but not the necessary access to create pages of the default child type.

I think this is a possible configuration issue that we may want to alert admins of, but I still thing having explicit per-book permissions for adding child pager and editing outlines is the way to go.

Also, I think it makes sense to assume that any role with "administer nodes" can add child pages and edit outlines for any book.

I wouldn't worry about dieting for now. That's post-freeze performance tuning that we can do in early July. For now just worry about making it work and getting it committed. We can deal with code/file factoring later.

Here is the long-awaited ajax!

with the js file too

for real this time

fixing the base url

WOW!! This patch has come an incredible way in the past little while! My only regret is I don't have a means of auto-generating 1200 bazillion pages so we can see how this would work in a handbook situation. :)

+1 for book outline options on the node form itself. The collapsed fieldset there is great, and the JS stuff for moving pages between books and such seems to work really well. I'll need to do a more thorough review of this soon (hopefully this weekend).

It sounds a bit to me though that to reconcile the conceptual differences between Peter and Dries's vision, we might need to handle renaming this module and making various UI changes in that direction at the same time. Although parts of what has been talked about leave me wondering if the "outline" feature isn't really best implemented by menu.module, and migrating legacy books to be menu paths. Maybe for D7. ;)

for those of us coming in very late to help test... exactly what needs to be patched in what order?
I just used a fresh install of 6 dev and the most current patch and got errors about missing fields in book table ...
I added 3 fields and still had no luck understanding how to make this come together.

@webchick: re: menu links: book.module is pretty much a way to link pages to menu items, and pwolanin made a script somewhere to test it

@pwolanin: please come into IRC :P

I had another look at this patch it is becoming rather large. I _still_ think that we need to ditch the 'Change book' functionality and just display a list of all the possible parents.

Also, this patch is becoming _huge_. I agree with the fundamentals of this patch (removing the book node type) but not with some of the details of this patch. The things we agree on can't be committed because of the things we don't agree on (yet). It's a pity. Either way, I'll try to spend some more time with this patch the next couple of weeks. :)

I applied this patch to a fresh install of HEAD today, and proceeded to beat on it with a stick for some time. I did not look at the code.

In general, everything worked, and I successfully created and organized a series of pages into a hierarchical "About Us" section for a fictitious company, including sub-pages for various types of contact information. I created several node new node types and included them in a book, and then removed them from the node hierarchy. Everything worked as expected; nay, worked much better than expected. I created books within books, even, and had no issues other than a small undefined constant notice in the 'printer friendly' view with regard to LANGUAGE_RTL -- I mentioned this to pwolanin in #drupal.

This is a significant increase in functionality and dramatically improves the book module. That said, this expands book module to a point where it almost seems as if the book paradigm is lacking itself; I mean to say, the functionality here is leaving the old book metaphor behind quickly. I found myself wondering if the entire module should be renamed into something more generic.

Warning--feature creep that should not hold up this patch follows:
I also wonder about how, or even if, the UI could be further integrated with the functionality exposed by menu module on the node add/edit screens. In some very real ways, book module, menu module and path module all provide some outward facing bits that work together to do some very nifty site structuring. I wonder if there is a way to combine the interface of all three into an enabled-by-default, holistic approach to creating hierarchy/menu links/paths.

the following query has more values than there are columns in the new book table, so the database update fails:

db_query("INSERT INTO {book} (mlid, nid, book_id) VALUES (%d, %d, %d, '%s')", $book['mlid'], $book['nid'], $book['book_id']);

attached patch corrects.

with that adjustment, both the new database schema and the database update (with or without existing book nodes) work in postgres.

there seem to be some UI problems with the patch IMO:

• the 'Book outline options' collapsible fieldset is in a very funky place in the node page layout -- between the title and the body. it should most likely be down with the other fieldsets below the body.
• it seems poor design to have both that fieldset and an 'Outline' tab, which only has the same stuff as the fieldset. we should pick one or the other.

whoop. that bad query i listed above actually happens twice. attached patch fixes the one i missed...

The patch as it currently stands doesn't do a great deal, AFAICT, apart from changing the book module to use menu items for storing the parent of each node. As far as I'm concerned, the patch falls way short of the proposal in http://drupal.org/node/128731, for a new "core hierarchical page structuring module".

I hate to be whining, and I hate to demand stuff that may be considered feature overkill for this patch, but I think that the following big changes are needed in this patch before it can go into core:

1. Get rid of the dependency on nodes. Completely! Make this the dream outliner module that we so desperately need in core. This patch is halfway there - with it, the book module now defines page hierarchy through menu items. Take the patch the rest of the way. Allow ANY menu callback (i.e. any page - be it a 'system-generated page', a node, a taxonomy term, a view, or whatever else) to be outlined using the book module.
2. For all non-node pages, add an 'outline' tab, allowing those pages to easily have their parent and weight (and title?) redefined (perhaps exclude it from admin pages - might not be appropriate allowing those to be re-organised so easily).
3. For node pages, add the 'outline' feature as a fieldset in the node edit form. Make the feature toggleable per node type (like comments, attachments, etc). I vote keeping the 'add child page' link (for selected node types) - but instead of linking directly to the 'node add' form, make it first link to a page where the user chooses the node type of the child page, then goes to the appropriate form for creating that child node.
4. Really, we don't need the {book} table at all anymore. Get rid of it. Just use the menu system for all the storage needs. Book module becomes purely a front-end to improve usability. 'Menu trees' become the new 'top-level books', and 'menu items' become the new 'book pages' - the entities are basically equivalent, so there's no need for the book module to continue having its own system for organising things.

And the following proposals are probably feature bloat for this one patch - but I'd definitely like to see them go into core at a later stage:

1. Create a new generic library of hierarchical tree functions (in a new tree.inc file), and make the menu system depend on this library, instead of using its own code to do parents / children / prev / next / etc stuff.
2. Remove all hierarchical structuring abilities from taxonomy.module, and instead use the new book / outline module and the menu system to organise taxonomy terms, same as for nodes (hey, they're pages, part of the menu tree, just like nodes, right? they don't need their own hierarchy system!).
3. Change comment.module to utilise the generic tree library as well. Then, whereas we now have FOUR separate hierarchy systems in core (comment, book, menu, taxonomy), we will instead have only TWO systems (comment and menu), both using the same ONE underlying function library!

Would love to help make all this a reality, but since I'm travelling at the moment, my feedback will have to do for now. Hope it helps. :P

fwiw, I really really like the "outline" tab, and never got on well with the extra fieldset. Trying to put different content types in books, and having the form in two different places when moving them around has been really confusing sometimes. And as indicated by Jaza, would provide a consistent ui if taxonomy terms, views, panels etc. are ever book/outline-enabled.

@pwolanin: the bottom line is that it's poor UI to have both the fieldset and the tab display at the same time. if you want to keep both, then you should find a way to only display one at a time depending on the circumstance.

spent some time looking over the code in this patch:

1. i've seen some references to: MENU_MAX_DEPTH -- which i believe is currently 6 layers deep. does this mean that books will be limited in depth to 6 layers? that might be fine, but i thought it was worth bringing up.
2. function book_form_node_delete_confirm_alter(&$form, $form_state) { -- this whole function seems a bit weird to me -- isn't there a more elegant way to accomplish this? i would at least think we should use a drupal_set_message() there instead of a form item.
3. book_node_types() -- seems like this function could be accomplished more efficiently with a single query than going through the book_get_books() dance.
4. i may have to plead ignorance here, but aren't there any FAPI 3 helper functions available to help w/ the AJAX callback? it seems weird that you have to set the cache by hand.

this is one huge patch -- i only made it about 1/3 of the way through the code before i ran out of gas! hope the above helps...

The result of book_get_books() is cached in a static variable and used later in the form building, so I think calling that is (on average) cheaper than doing a separate query.

yes, i did see the static var. i was more meaning that when you _do_ build that var, it seems like you could do some kind of 'SELECT DISTINCT type FROM..' query to get the types, instead of calling that other, more involved function. not a big deal, i just thought it might be a little quicker for the initial building of the static var.

Ref. #94:
It might be that the practicality of having both outweighs the "bad UI" concern in this case.

Anyway, if there is consensus for absolutely having it in only one place, I'd say a +1 for keeping the outline tab. Makes it logical and easy to spot for new users too.

But more importantly:
- would keeping this functionality in the outline tab actually make it possible to restructure book outlines without affecting the update timestamp of the pages? It is quite important to have the option of doing a restructure without having the affected pages pop up on top of the activity or recent listings.

I agree with most of Jazas points in #96, particularly regarding going all the way and making this totally independent/generic.

I tested on Peter's site and I say it's ready. We can always tweak it more -- but it's very, veyr good now.

+1 on the RTBC. I gave this patch a medium-detailed review, and didn't spot anything wrong. I clicked through the test site extensively, tried many operations as different users, etc. All works better than I expected. Of course, we could probably tweak things here and there, but this is a vast improvement over the existing book functionality, so let's get this in.

Also, I think making the block title a clickable-link to the root of the book is a great solution, even though we don't normally make block titles links. The alternatives are ugly and gross, and this is nice and elegant, IMHO.

Finally, I agree that this should be renamed to something like "outline.module". However, that's a completely separate issue. 5.x's book.module is just as poorly named, so there's no reason to hold up this patch any longer over this. All of you who complain about the patch being too large should take notice: doing the rename *and* the port to menu *and* the nodeapi() changes, etc, etc, would make this even larger and less likely to be carefully reviewed. So, let's not lose site of reality, here. These changes are great, book is (still) a poor name for this module, and if we reward all of peter's hard work by committing this, we can handle the rename as a follow-up issue.

Thanks,
-Derek

p.s. I also respect Jaza's input on this, but I think that we have to take incremental steps here. This patch/issue already has trouble getting serious input since it's doing a lot. It can't really be made much smaller, but we can certainly prevent it from mushrooming even larger by not throwing every wish-list item and its best friend in here. ;) Again, if this lands, it makes it easier to do the complete outliner solution that Jaza was asking for, not harder. The patch for that would be relatively small compared to the initial foundation of menu-based-books laid here. Worst case, we do completely generic outlining in D7. At least we made book much better in the meantime, and the d.o handbooks won't be such a huge pain to manage.

I had a look 'round the demo site last night. The UI is much better, everything works OK as far as I can tell. Any gripes I might have are inherited from the 5.x book module, not introduced with this patch, so agree with other posters here that it should go in now, with any extra tweaks/renaming etc. in a different patch.

This is looking nice!

I recommended that the "Book this page belongs in" be moved to the top of the fieldset rather than the bottom. I like it a lot better there.

1. The patch outputs things like "Submit Story" -- should be "Submit story".

2. I don't think this patch makes it easier to organize pages in trees. As I mentioned a dozen times, I'd like to get rid of the 'book' notion. I want to assign pages to a parent, not to a book and its parent.

3. I'm not sure I like all the Javascript going on. All of a sudden there appeared to be a button? I didn't manage to get the button back, but I know I was confused for a second.

I think I prefer to wait for a patch that does a better job integrating with the menu system and that manages to eliminate the concept of books. This patch could be simplified to make it easier for people to work with pages and hierarchies.

Like I said, I tried it out, and it's a big improvement in UI (not to mention the back end stuff which I get the idea of but can't review how that's coded).

There's other stuff that could be done to change 'book' into an outline.module - but that's not going to get into D6, this can, and it's a prerequisite to further abstraction.

Dries, the patch is a big step forward. Other steps can be made later. If you want to have something that organizes nodes into a hierarchy this works, and much better than we had earlier. Menu is a diffrent thing.

@Dries - as has been stated earlier, this patch simply uses the node api and the menu api how they should be used. It does not change any concepts or re-name anything. That is for another patch entirely. This patch simply brings book module up to date to use what Drupal provides it, not hack up its own solutions.

I still support this patch (and the new title of the issue). Our motto should be "code is gold". The patch represents probably hundreds of hours of work bringing book.module up to date with our existing core APIs and giving it some much-needed usability improvements. Is it perfect? No. Will we ever improve upon it? Of course. Are we going to go 2 major releases of core without improving book.module at all (dogfood we eat on d.o itself)? I certainly hope not. Should book be renamed and undergo yet more changes in the future? Definitely.

As with the Deletion API, let us not waste this immense effort because someday something else might supercede it. In the name of "it's not perfect yet," we must not paralyze ourselves and prevent making progress at all. The drop is always supposed to be moving. Let's not leave book behind.

Comments on the Javascript only. I haven't tested.

This is nicely coded and looks to accomplish in a few simple lines what other attempts I've seen took much longer to do.

Some points for refinement.

1. The behavior should have a separate attach function rather than be added directly to the jQuery ready object. This facilitates reattachment.

2. Drupal.bookFillSelect should not be part of that function but, for consistency with what we do elsewhere, should be defined independently.

3. The behavior is generally useful and so ideally wouldn't be hard-coded to book.module. The JS is written well enough to be relatively easily genericized to e.g. updateSelect. Probably we should do this through the Forms API, setting element properties and a #process callback.

4. The issue of attaching behaviors to a collapsed fieldset is not one I was aware of and may require more thought and a fuller solution. What would happen if someone form_altered the book fieldset to set ['#collapsed'] = TRUE? It looks like that would break the behavior. We have the anomalous situation where Forms API can only say ['#collapsed'] = FALSE, while the JS hard-codes ['#collapsed'] = TRUE. A possible solution is to add the JS in a #process callback, where we can also set #collapsed to FALSE and send a setting indicating the original state of the fieldset (collapsed or not), which the JS then reinstates. Ideally we'd find a way to solve this for all behaviors at once, since presumably other behaviors will also fail to attach to elements in collapsed fieldsets.

Points 1 and 2 are quick and easy fixes.

Points 3 and 4 obviously would require more work and arguably could wait for followup patches. Point 3 might benefit from the forms AHAH patch currently in the RTBC queue.

Points 1 and 2 are too minor to hold up the patch.

Points 3 and 4 are longer term issues.

I'll just speak to this quickly purely from an end-user, handbook maintainer perspective. This isn't the "holy outliner module" that we ultimately want and it is certainly not even a perfect book module, but it is an improvement for book module as it currently is. I would REALLY like to use this in D6 and not wait another year for book/outliner/whatever to come along. This patch simply brings book up to current with the rest of Drupal. Things that I like about this from the end-user view:

1) Using nodeapi so I can just outline my content, whatever it may be. This makes it feel like it belongs in a CCK Drupal world rather than the old school hard-coded world and I'm not confused by the book content type when I have my own ideas of what my books should be made up of.
2) Limiting the selection to one book at a time. Especially on sites like d.o this is a HUGE improvement on UI, maybe not perfect but the current UI is ridiculous and overwhelming.

So, seriously that is all I'm looking at here, not having a chance to really review the code. Is that enough to put it in core? That's up to Dries but I do feel that it is worthwhile for the end-user experience.

The attached patch fixes the two first issues nedjo mentioned. The other two issues really do not belong here, they should be follow-up patches.

add1sun cross-posted with nedjo, and reading her post, I don't think she actually meant to set back to 'needs work'

Remove the "book page" node type, and turn the book module into a module that is specialized at creating hierarchical content trees. The module should automatically extract a menu from the node hierarchy, set the correct breadcrumbs, help you define a hierarchical URL structure with clean URLs, etc. This module will be the de facto standard to structure pages and to create hierarchical navigation schemes in Drupal.

http://buytaert.net/suggestions-for-drupal-core let's see, we have book type removed, it's creating node trees as ever just now via menu system, automatic extraction no (what's that anyways -- especially now a book is a menu), breadcrumbs are set, hierarchical URLs are not set but that'd be an easy followup patch. So, there the book patch was high on the list but now that you have something well tested, supported and implemented it's a no go :( ? As I always say, surely can be something better, but this is good enough in itself.

important: http://drupal.org/node/146425#comment-253019 that's the documentation team lead 's comment.

This patch updates book.js to use the just-commited behaviors patch. Tested, no functionality changes, and confirmed to be done right by nedjo.

Stop talking about how multiple books would be useful. I know it would be useful. We are NOT debating whether we should implement this feature. We are debating HOW it should be implemented. Please, get your act together if you choose to respond. Thanks.

As I said repeatedly, there are many good things in the patch. However, it does some things that I disagree with.

Achieves 100% integration with the menu system.

At the API level, it might but not at the UI level. For this patch to be committable it needs to integrate properly at the UI level too. In other words, the module needs to integrate with the menu module (i.e. it should probably have a dependency on the menu module and offload a number of its tasks to the menu module). When I create a book, and when I add it to the menu using the menu module, the book's child pages are not added to the menu. Only the root is added. In other words, this patch provides integration with the menu system, but it works independantly from non-book pages. That seems flawed to me, and makes it rather confusing. It means that it fails to be "the tool" to structure pages.

Given the many important cleanups this patch adds and the fact that it brings the book module up to current with the rest of Drupal, I would be able to live with this flaw assuming that we'd fix it in Drupal 7. However, what makes it non-committable is the fact that this patch introduces new functionality that is implemented in the wrong way, and that carries us away from proper menu module integration. The support for multiple books is useful, but it stretches out the flaw. Committing this to Drupal 6 would probably mean that we'd need to rollback large chunks of this patch once we start working on Drupal 7, and that we'll be presented with a challenging migration path that depends on menu.module changes. That's going to going to cause a lot of trouble. Committing this patch now, is going to lock us in in the wrong technical implementaiton and I want to avoid that at all costs.

So I'm afraid that this patch suffers from feature creep. It does a number of good things that I'd love to commit, but it also takes the book module in a direction that I don't agree with. The best way forward is to split this patch in smaller chunks, and to commit one chunk at a time. Step #1 should be to remove the book node type. Step #2 should be to integrate properly with the menu APIs. Step #3 should be to integrate with the menu module. Step #4 should be to improve permission support in the menu module/system so we can implement multiple books.

The current patch provides step #1 and step #2, but fails at step #3 and step #4. I'd love to commit #1 and #2 though. I could live with committing #1, #2 and #3, but only if #4 is left out of this patch. We can rework #3 in Drupal 7, but it's going to be troublesome if we need to rework both #3 and #4 in Drupal 7. The best way to get there is to remove the support for multiple books from this patch (#1, #2 and #3). Then for Drupal 7, we can figure out how to support multiple books with support from the menu module.

Hope that helps!

The only problem with Dries committing without #4 is that d.o uses multiple books, right?

At the API level, it might but not at the UI level. For this patch to be committable it needs to integrate properly at the UI level too. In other words, the module needs to integrate with the menu module (i.e. it should probably have a dependency on the menu module and offload a number of its tasks to the menu module). When I create a book, and when I add it to the menu using the menu module, the book's child pages are not added to the menu. Only the root is added. In other words, this patch provides integration with the menu system, but it works independantly from non-book pages. That seems flawed to me, and makes it rather confusing. It means that it fails to be "the tool" to structure pages.

Dries,

I can't comment on some of the other issues, as they primarily relate to the documentation team and their needs on drupal.org. (They've been angling for book.module improvements for a while, and I talked to sepeck a while ago about helping, but got sucked into FAPI stuff and wasn't able to dedicate much time...)

The trail you seem to be pointing down is the one that was blazed by Category module a couple of years ago. The expressed problem was that there should be one method for 'building hierarchies' that's used everywhere. At the time, the idea was that taxonomy, book, and menu should be merged into a single mega-module. Making book and menu use the same UI and mechanisms isn't quite that extreme, but edges in the same direction. I think it's compelling at an abstract level, but is a mistake in the real world of Drupal usage. I've argued before that the answer is to standardize on underlying APIs and storage mechanisms while allowing high-level modules to present situationally appropriate UIs. Why?

1. Pure hierarchial site structure management, and managing online documentation/online books, etc, are different conceptual tasks with different UI needs.
2. The refactoring in this patch, along with the refactoring in menu.module and the menu.inc APIs, move book.module's underlying storage mechanism to the menu APIs.
3. The underlying API/storage refactoring makes it easier, not more difficult, to expose a streamlined UI in Drupal 7 that lets users manage the entire site hierarchy without the concept of a 'top level book'.

I've been on the fence with this patch for a while, but after spending a lot of time reading over your concerns (which I think are valid), studying the code and the API changes, and picking Peter's brain about the way it works, I think that it it accomplishes a subset of what you're hoping for without standing in the way of subsequent improvements that take us all the way to your vision for site outlining.

Because drupal.org depends so heavily on book.module, I think this approach is the best one. Obviously, it's your call, but I think that some of the history of this patch may be clouding the fact that as it stands now it is real progress along the path that you've laid out. Hope this analysis is at least helpful.

I've just taken it through its paces and the code itself is -- from a technical standpoint -- RTBC in my opinion.

I want to make clear that I think this is Dries' judgement call, and I don't want to play "It's ready"/"No, it's not"/"Yes it is" ping pong with the issue status. If you find the discussion above compelling and feel that it provides useful refactoring and enhancements for the doc team, without standing in the way of future outliner development, this patch is ready.

I'll have another look at this tomorrow morning. From reading the comments, it looks like we're just running around in circles though.

I'd like to find a better solution to the issue of handling collapse in the Javascript. What we have now is a hack.

I'd like to understand better why the behavior doesn't attach to a collapsed fieldset's children. Is this true for all events we'd like to attach?

I wonder if something like the following would be a general solution:

// Unhide any hidden parents.
var hiddenParents = [];
var parents = $(elt).parents();
for (i in parents) {
  if (parents[i].css('display') == 'none') {
    parents[i].css('display', 'block');
    hiddenParents.push(i);
  }
};

// Attach behavior.

// Rehide parents if needed.
if (hiddenParents.count) {
  for (i in hiddenParents) {
    parents[i].css('display', 'none');
  }
}

More appropriate status, given the new patch and concerns about JS. Sorry I don't have time to thoroughly review and test right now. :(

I don't uderstand why the hide() should be different that attaching a function.

Thanks for digging into this, I think you've hit on the key to the problem.

$().hide() will only set display: none on an element that is currently visible. (The method uses this.filter(":visible").) Since the button is inside a fieldset that is already not visible, no change is made.

So apparently all we need to do is explicitly set the css display property.

$('#edit-book-pick-book').css('display', 'none');

Unless I missed something,
this patch now implements a book module, complete with a book node type, using the new D6 APIs, etc.

If the goal was to come up with the best way to implement the "book" paradigm, then this patch does an admirable job.
The trouble is that the task at hand was to remove the notion of a "book" ..............

It's July the fourth, where does this patch go from here?

I suppose it couldn't hurt for books to live on as a contrib module to allow for the handbooks to live on "as is",
but the bottom line is that Drupal STILL desperately needs a "de facto" way of building hierachical websites.

The taxonomy/menu/books/panels/comments/outlines/category module dance is getting really frustrating
... and merely having a better book module doesn't really address the problem at hand :(

whoops, didn't mean to change the title .. now there's a usability issue ... anyway ...

@irstudio - yes, you missed the whole discussion here. this issue is titled "improve book module". go elsewhere if you want to discuss something else.

working through the demo site, the interface looks and feels good. Things we're were I expected them.

i also worked through the demo site using 3 different users of differing permission levels. i reported 1 bug to pwolanin concerning the fieldset on node creation for regular user. i also asked for an admin link to be made more accessible in help text. otherwise, looks lovely to me.

both minor bugs confirmed fixed.

a code review identifies only really minor issues:

• hook_submit() is gone. change comment for book_submit() noting that it is a fapi submit hander on node form. you might consider using book_nodeapi('presave') insted though. would be a bit easier for people writing import scripts.
• typos - search for 'repalce' and 'confrim'
• would be nice to have an is_node_form() function which does this madness:
  if (isset($form['type']) && isset($form['#node']) && $form['type']['#value'] .'_node_form' == $form_id). can be a different patch though.
• the line below from _book_parent_select() looks odd. specifically, shouldn't the left hand side be $form[whatever]:
  + $form = array('#type' => 'hidden', '#value' => -1, '#prefix' => '<div id="edit-book-plid-wrapper">', '#suffix' => '</div>', );
• perhaps reword: "Select the book you wish your page to be a part of." to "Add your post to a book."
• is $node->build_node supposed to be useful for book.module, or did that part of the node render patch get axed?. eaton?

that last bullet was meant to be node->build_mode

i see NODE_BUILD_PRINT getting set in the _pre() function but where is that flag used?

pwolanin explained that book navigation is now appended only if NODE_BUILD_NORMAL is set. sounds good to me.

this is RTBC.

I'm going to give this patch a bit of a work-out over the weekend. I just spent some time looking at the code, and I've some first requests:

1. It would be great if you could give a rough overview of what book_update_6000() does. I suggest you add two or three sentences to the PHPdoc of that function. This is minor, but it helps us understand that update function 6-12 months from now.

2. Can be rename the CSS class/ID 'bookSelect' to 'book-select'? We don't use camel casing in CSS classes/IDs?

3. modules/book/book.js has no documentation -- it would be nice if you could describe each behavior in one line.

4. I have a hard time remembering what 'plid' stands for. We're now exposing that term to the CSS which might confuse designers. (I know it confuses me.)

5. We now have a "add content to books" and a "outline posts in books" permission. I'm afraid that the distinction is not clear. I'd like to see us think about this some more, and maybe rename these permissions, merge them, or whatnot. This is my biggest gripe at the moment.

6. "AJAX callback to replace the book parent select." -- We could be a tad more verbose here. Not everyone might know what 'book parent select' means.

7.

 +  // We do not want to execute button level handlers, we want the form level
+  // handlers to go in and change the submitted values.

Would be nice if you also mention _why_ we want this.

8. "Like a node preview, to update options in the parent select." is rather abstract.

9. "Common helper function to update the book outline." -- what is meant with 'updating the book outline'?

10. "Update the book_id for a page and its children when we move it to a new book." -- this is quite techno lingo for API documentation. The $link parameter is not documented either.

11. + static $flat = array(); if (is_null($book_link)) {print("hello");} Debug code?

12. $type_obj -- we don't abbreviate variable names like that. Just write $type.

13. Why 'book_id' and not 'bid'? bid would be more consistent.

14.

+      $expire = max(ini_get('session.cookie_lifetime'), 86400);
+      cache_set($cid, $form, 'cache_form', $expire);

Why are we using the cookie_lifetime here? That's seems a little peculiar. (Just document that in the code.)

15. Get the data structure representing a subtree. the root item will be the ... -- incorrect punctiation and techo lingo.

16. A fully loaded menu link, -- typo: should be point, not comma.

17. book_node_type() lack documentation.

It's hard to review this patch. I just spent another hour with it, and still I don't grok all the aspects. The PHPdoc is spare and it makes a lot of assumptions -- it assumes that the reader is neck-deep into the book module already. I know a fair bit about the book module and yet, its documentation doesn't really help me.

It would be great if we could make the PHPdoc a bit more complete and if we could keep in mind that the PHPdoc is meant to teach people about the inner workings of the book module, and its functions. Often the PHPdoc doesn't help me figure out *why* you'd want to call this function or *when* you'd want to call this function. I like it when just reading the PHPdoc gives me a good grasp of what the module does, how it does it and why it does it like that. The current PHPdoc of the module doesn't get me to that level.

This is not a show-stopper, it just complicates the reviewing as it takes me significantly more time to connect the bits and pieces in my head.

"bid is already used in the {boxes} and {blocks} tables" - and in the, er, {batch} table...

Just a quick vote for "administer book outlines" as the permission name.

I'd rather have more clear variable and column names (book_id) than a little bit more phpdoc to tell me that "bid" in this module means "book id". @Dries: your request about "I have a hard time remembering what 'plid' stands for." seems to contradict your one about bid. pwolain can't change all of the terse, hard to remember variable/column names at once in this patch, but at least the new ones he's adding can be a little more self-documenting.

That said, I'm certainly a fan of clearly documented code, and figured that a seriously well-commented update.module was the only chance I had of getting it in core. Seems to have worked. ;)

Finally, "administer book outlines" also seems sane/good to me. I immediately understand what that's supposed to do, and it's clear to me how that's different from "add content to books". Renaming the existing permission is ok. If you want some help with the DB update path, take a look at signup_update_4() from signup.module, where I had to rename a bunch of confusingly named permissions.

Cheers,
-Derek

Thanks for the quick updates. I'll review this patch in the next 24 hours, though feel free to upload new versions or to solicit feedback from other people. A 10-minute pass over the latest patch shows that the code comments got significantly better. Good job, and much appreciated. :)

re: 169 -- ah, right! Time permitting, feel free to improve the code comments in the node.module too.

re: 171, dww: well, but bid is more consistent with the rest of Drupal, and if the module is called book.module, it's actually quite easy to guess. ;-)

I stand by my support for "book_id". WTF is a "vid"? Depending on what module you ask, it's wildly different things. :( It might be a Vocabulary ID, it might be a reVision ID, etc, etc. Just because the rest of core uses the way-too-short-to-be-useful variable/column names doesn't make it right. Self-documenting code is more useful than well-commented, cryptic code. I'll grant, in this particular case, "bid" is pretty clear, but I don't buy the "but the rest of core does it" argument in this case.

IMHO, this is something that cries out to be improved core-wide (just like the namespace collisions for the $node object caused by much the same problem -- e.g. WTF is a "pid"?). We can't fix all of it in this patch, or even in D6, but there's no reason not to make an incremental improvement here. ;)

This patch is looking much better. Haven't had time or testing environment to try out the patch properly, but I had a look at the latest patch, and have some feedback:

1. Why does book.module still need to generate blocks for navigation? Aren't these now redundant, since every book page has a menu item generated for it, hence navigation blocks are now available courtesy of menu.module?
2. This may be outside the scope of this patch, since it still desn't provide proper support for multiple books, but it would be good if we could make the book.module permissions per-book / per-menu, rather than global. E.g. instead of "edit book content", we could have "edit content in book xyz".

I'm definitely part of the majority now, who are saying "getting this into D6 is much better than getting no book.module improvements into D6". Hopefully it can still make it - what we have here at the moment will make a big difference, IMHO.

Spent some more time testing this patch.

1. The fieldset should be called 'Book outline' instead of 'Book outline options'. The word 'options' is redundant and the other fieldsets don't use it eiter.

2. The name 'book outline' is a really bad name, but as long we chose not to changes this module for the better and give up the book connotation, it's the best we can do, I'm afraid.

3. "This will be the top-level page in this book" should end with a point.

4. "The book to add this post to:" is not consistent with "Parent item:". They different in tone, the titles should not be instructions but just descriptions.

5. Personally, I'd prefer to merge both fields again. When I change the book, the parents change by means of AJAX. This is somewhat confusing as it's not very visual; it happens under the hood.

6. Having support for multiple books might lock us in. In the future, we'll want to integrate this closer with the menu module. In that case, I don't really care about the 'book' a page belongs to -- I only care about the parent menu item. Specifically, I should be able to start a new book under the existing about-page, for example.

I'm afraid that the current system takes too big of a step forward to the point that it will be too hard or impossible to undo this change. It's easy to add features but it's hard to take functionality out again. While the code looks good, and while the goals are useful, I think we failed to get consensus on how this should be integrated with the rest of Drupal. While there is nothing fundamentally flawed, I'm afraid it takes us into a direction that we'll regret later. I still think that integration with the menu module is key and that getting rid of the 'book notion' is crucial for wide-spread use of this module.

I'd like to brainstorm a bit about the future of this module, and how we could migrate from the proposed module to a version of the module that is easier to use, that gets rid of the book mantra, and that allows me to seamingly structure pages (and have navigation links be automatically generated). Imagine this gets committed to Drupal 6, what are the next steps for Drupal 7?

single select.

I don't use the book module for navigation any more for this reason, however, when I did:

History
-Asia
--Japan
---Taisho Period
----Rice Riots 1918
-----Some book
------Chapter 2
------Chapter 3
------Chapter 4
-----Osaka
------Chapter 1
------Chapter 2
--Korea
--China
-Africa
-Americas
-Europe
-Oceania

But with about 1500 nodes and more depth than that. This means a massive amount of scrolling up and down a smallish select menu, working section by section, scrolling past, scrolling back etc. etc. unable to use keyboard shortcuts to get where you want.

It's an absolute nightmare to navigate, as is the book menu select on drupal.org

The two select method cuts out a massive amount of that scrolling and eye-strain, and will make it much easier for me to maintain the area of our site which still uses book.module (now a couple of dozen full-length books with up to 35 chapters each and a smallish 'handbook').

The current ui for organising books is broken IMHO, the version in this patch is a massive, massive improvement. Even if it lasts for only Drupal 6 and is superceeded by something more drastic, I think it will make a big difference, both to me and others with relatively large numbers of nodes in the book module, and to Drupal.org itself.

@#180
catch, I couldn't agree with you more.
pwolanin, I'm loving the enhancements made by your patch. Well done!

After some further testing, most of the bugs/issues that I've been able to find have been corrected in the latest patch. (There is one relatively minor bug that pwolanin is aware of and is looking into involving some strange, intermittent, behaviour of sixth level items.)

So I read the whole patch and there were a few things that made me go "uhm" (esp. the use of each without a reset -- I had no idea that assignment comes with an array pointer reset) but those are commented now. I also was wondering whether menu_tree_all_data in a block will cause performance hit -- not because it's cached and even without that, in this case the query strongly resembles the one for a normal menu, so all is good. Next, there is a JS form -- that's a beauty, Drupal 6 features well used, it could be the example of how you do dynamic forms w/ D6 (it's the example in the book :P ). Note that we now have a $node->book which nicely keeps together book properties. All in all, what I see is a very nice use of all new D6 goodness -- I do not have any problems with how this coded.

I really like the fact of how this is independent of the menu.module and makes its own menu structure opening up quite a few interesting possibilities.
With all the main problems fixed this patch to me it seems this patch is ready be committed

And yes I did have a look at the code and tested it on my test server.

+ $('#edit-book-pick-book').css('display', 'none');
should be:
+ $('#edit-book-pick-book').hide();

Wouldn't it b e nicer if we change:
+ pb.setProgress(-1, Drupal.t('Updating parents...'));
to something more descriptive? (can't come up with something myself, but I think it could have a better progress message.

Further more, I like the functionality the patch is giving us.
Though, some questions:
- Why do we need the "new" node type 'book'? Is that neccesary for backwards compatibility, or do we just want another type to be immediatly associated with the book outlining structure?

book is necessary for backwards compatibility I think.

I just started testing this patch some more.

As user #1, I created a book with 3 pages and enabled the "Book navigation" block. However, no new block is showing up. This is confusing, and might be a bug?

I've to run for lunch now so I can't debug it myself. However, I figured I'd already report it. I should be able to test this some more this afternoon.

Committed to CVS HEAD. Thanks a ton, pwolanin. Good job.

Committed to CVS HEAD. Thanks a ton, pwolanin. Good job.

pwolanin, that this got committed is just proof of your indestructible will, i'd have given up on it long ago. congratulations.

I'm sorry. I hate to do this, but you guys seem to know the book module really well. Could you please help me out with this:
http://drupal.org/node/163993

Thank you for all your help in advance.