improve book module: use nodeapi and menu API
pwolanin - May 24, 2007 - 01:54
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | book.module |
| Category: | task |
| Priority: | normal |
| Assigned: | pwolanin |
| Status: | closed |
Description
We should be able to get improvements in both function and performance by integrating with the new menu system.
See:
Core hierarchical page structuring module
http://groups.drupal.org/drupal-dojo/book-project
These improvements require this patch to go in first: http://drupal.org/node/145058
#1
second link should be: Transforming Book Module Into Robust Outliner for Drupal 6.0
#2
initial schema patch:
mlid - will allow join to menu_links
nid - for nodes
bookid - will equal the nid of the top-level node
child_type - define the node type for "add child page" link
#3
starting to hack at the code- nothing functional yet.
#4
subscribe
#5
snapshot of progress.
#6
another snapshot
#7
module installs, functions starting to work (a little)
#8
more functionality
#9
book export working reasonably
#10
better yet
#11
mostly just CSS and admin pages need fixing.
#12
here is a slight schema change... bookid => book_id in indexes
#13
more working- changed menu callbacks so enable or diable a module to trigger a menu rebuild
#14
Admin pages seem to work now.
#15
fixed a bug with removing pages form the outlien
Also, modified .info file and code in hook_uninstall
#16
fixed the problem with book navigation - the calls to l() had not been converted to use the new $options array and so weren't getting the right CSS classes applied.
Also, put the right value in menu_links for 'module'. Simplify uninstall code. Re-roll for new FAPI 3 parameter order for hook_form_alter. Fixed bugs in hook_nodeapi, and book_outline_submit.
Except for the update path I think this is very close to RTBC- review, please!
#17
subscribing
#18
book node generation script for 5.x to test update. Adapted for a 4.7 Devel module script.
Put at root level in your Drupal directory and visit the script. Edit the code to change the # of nodes generated.
You should probably generate a few users by hand or with Devel module first.
#19
now with update code!
#20
first try @ an upgrade path
#21
update.php stuff got in...
#22
need node_types_rebuild and menu_rebuild after node type save
#23
re-rolled with based on code comments and suggestions on IRC from dmitrig01 .
fixed book_node_visitor_html_pre() missing parameter. RTBC?
#24
tested. Seems to work flawlessly.
This baby is ready to fly. RTBC
#25
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?
#26
@Dries - I will address code style and comment issues as soon as I can (tonight).
I think the module renaming should wait for another patch, but I will very happily take that on once this gets in.
Part of the point of the way this is coded is to avoid any dependence on menu module. Menu module is now an optional module and, in a sense, this becomes an alternative to menu module. So, I think the fair comparison in terms of use of menu.inc functions is menu module itself.
Also, menu module shouldn't operate except on menu links that and menus that it created itself. The book administration page is a convenience feature, but something menu module can't offer.
$node->book_link contains a menu link, with additional book-module data.
$node->child_type determines the type in the "Add child page" link. Since a hard-coded 'book' type is gone, we need some way to specifiy this type. Having this also lets different books have different child-type links, since this property is automatically inherited from the parent node for non-admins.
I think the separate book notion is a critical feature - this makes the drop-down size manageable, and enables a lot of other possibilities. I thought this was the time for new features? Obviously, adding more JS prettiness to this UI later is an important possibility.
#27
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]#28
@Frando - being able to add non-node pages into books is something this patch moves closer to enabling. However, there are some difficult issues with making that work smoothly that I don't want to address here.
Also, I don't really understnad your suggestion re: 2 drop down. I think if we can AJAX the form, we can make this UI better (e.g. select the book, then the second drop-down or other interface is filled in with appropriate links), but it's not going to change the underlying functionality that's in place in this patch.
#29
massively updated code comments.
#30
Regarding:
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.
#31
fix book TOC in the book navigation
#32
after several discussions on IRC:
-I added an AJAX callback (though Josh K will comment on whether or not this is necessary).
-To help enable an add-on JS UI, I also put the form in the Outline tab in the same fieldset as on the node form.
- added a 'menu_name' attribute to the select for use with the AJAX callback.
-Per request of Josh K and others, added to the node type form a setting to exclude a node type from being added to book outlines.
#33
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.
#34
@Dries - I can try to clarify child_type in the form text. Simply put - all operations are handled by hook_nodeapi after this patch. So, there is no default "book" type and we need a way to specify the type that appears in the "Add child page" link. Doing this per node adds a lot of flexibility. Also, this option only appears for users with 'administer nodes' permission, so it won't be a concern for non-admins. For normal users, it inherits from the parent node.
@Dries - The menu system integration means that there is a 1-1 mapping between a "book" and a "menu". So this is the most logical way to code the integration. Changing the way this is operating in book would require substantial re-coding, and I think it is an important feature in terms of usability, reducing the amount of data loaded, etc. In any case, I expect we will continue to try to improve the UI with subsequent patches.
#35
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.
#36
@Dries - the reason I like this is that it means that different books can be set to have different child types. For example, if sepeck splits the handbooks into 'official' and 'unofficial', the 'offical' books could have a child-type node that can only be added by a member of the docs team, while 'unofficial' pages have a child types that can be added by any user.
Any type can be added as a child to any node via the outline tab for a user with the 'Outline posts in books' permission. I think this is an important granularity in permissions, and is a closer mimic to the current functionality.
#37
'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.
#38
@moshe - customizing the link is easy - becomes "Add story as child" or "Add new story"? I think having an intermediate form would be aggravating, plus see above.
$info = node_get_types('type', $node->child_type);t('Add @type_name as child', array(@type_name => $info->name));
#39
@mosh, Dries - how about an additional link and intermediate form that are only available to users with the 'Outline' permission (note, however, I can't do any coding til tonight - and not much even then). Or, this can be an additional patch if it's not very important to get done now?
#40
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.
#41
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:
<?php$.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.
#42
@josh_k : The only thing easy to cut is the AJAX callback I just added.
I'm frustrated. I want to make this module *better*, and I tried to follow this formula:
we're not quite there yet (e.g. no clean URL integration yet), but closer than before.
Patch attached- removes unneeded AJAX callback. Also includes a fix for the query in menu_tree_all_data(), which the menu module patch must have broken.
#43
I see know, the broken query was probably from this bad commit: http://drupal.org/node/147873
however, the commit seems not to have been fully rolled back.
#44
fixes notice:
notice: Undefined variable: output in /Users/Shared/www/drupal6/modules/book/book.module on line 927.Also, additional whitespace and comment cleanup.
#45
need to rewrite the update function per: http://drupal.org/node/144765#comment-245805
#46
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.
#47
@Dries - ok -I'm glad we're on the same page. I would just like to see this in, as the basis for making further changes. I think some concept of separate structures/outlines/etc is helpful, however, in orgnizaing a large site.
fixed (and tested) updated code in attached patch.
#48
@Dries - I really don't quite understand where our points of difference are, other than relatively trivial ones of user interface. It seems to me that any module for outlining/structuring a site needs to support multiple containers/hierarchies/outlines (whatever you want to call them). In 5.x, they are only weakly separated as different "books". I think it represents an essential strengthening of the module to move to the point where these "containers" can really be treated (themed, permissioned, localized, etc) as separate entities. My thinking is informed by all the possibilities that are implemented for Organic Groups.
Are you thinking that these "containers" should not be represented by nodes? Are you envisioning something totally different?
#49
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.
#50
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...
#51
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...
#52
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.
#53
/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.
#54
per this devel list discussion: http://lists.drupal.org/pipermail/development/2007-May/024194.html and this issue: http://drupal.org/node/148420 I changed the code so all book module data is held in the array $node->book
Also, fixed a few PHP notices, and added a message to the node_delete_confirm form.
#55
@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...
#56
@dww - regardless of where http://drupal.org/node/148420 ens up, clearly we're moving towards (at the least a best practice) of keeping things out of the top-level of $node.
Since we seem to have a little time, I can do a rename if Dries is will to give me a specification ASAP. The only tricky part about that- how do we handle the update? Put a check in the X_install() function to see if {book} exists or if book module is in the {system table}? Should it delete book from the {system} table?
#57
per http://drupal.org/node/147873 change the menu query to be Oracle-friendly.
#58
@Dries - should the module be renamed in this patch?
In thinking about this, 'child_type' could equally well be set per node or per "container". Are there settings that should be supported in core on a per-container (a.k.a. per-book) basis?
#59
new patch to accommodate FAPI changes. Setting to "needs review" as a synonym for "needs feedback"!
#60
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:
#61
@Dries - there is a problem with what you are asking for, which mainly pertains to the book navigation elements and navigation through the hierarchy.
If the book hierarchy can include non-node pages (implicit if we're adding to Navigation), then many of the fundamental assumptions of the module break. For example, Drupal has no easy way to have me show "Add child page" on /admin.
Or, is what you're saying that each book should get added to Navigation, rather than being listed separately at /book?
#62
@Dries - reading again what you wrote, it seems like you are asking for what the menu module already does with menu-on-the-fly in the node form. I think the menu module should be enabled by default, and we should consider relocating the menu module's fieldset on the node form so it's more visible.
#63
@Dries - this thread (though a couple years old) may be worth looking back at: http://drupal.org/node/31896
I think we need to distinguish among use cases:
1) brochure/static site
content added mostly or entirely by a few admins, who may be novices
relatively small (~10-100 pages)
all links can be in the navigation menu or primary links
links are a mix of node and callbacks
2) informational site
content added mostly or entirely by a few admins
relatively large (~100's-1000's of pages)
need a separate menu for each site section (e.g. FAQ vs. user guide)
links are nodes
3) community site
content added by many users (non-admins)
relatively large (~100's-1000's of pages)
need a separate menu for each site section (e.g. FAQ vs. user guide)
may need different access control for different site sections
links are nodes
The thread I reference above deals essential with case #1. I think menu otf is really the right solution for this case, so we should think about how to make it most accessible/usable for new admins. This also seems to be use case you have in mind.
For book module, I have in mind use cases #2 and #3. #3 can be represented by the d.o handbooks, and is the use case *I* have in mind as I'm trying to improve the book module. Even though book module may also be useful for case #1, I think it would be a huge mistake to orient the module to that use case.
#64
re-roll for current HEAD - also previous patch picked up some unrelated changes to other files.
#65
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.
#66
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.
#67
following up on discussion with Josh- this is a partial recode that makes the child-type link fixed as the type of the top node in the book.
I think it all works, but I didn't test the update code yet.
#68
After sleeping on this, I still think that using node types are the basis for whether one can add posts to a book.
First off, this will provide a fairly transparent upgrade from 5.x - users that could previously add nodes of type "book" will still be able to add to the book outline.
Second, I think that having 2 permissions per book will become very unwieldy on the Access control page- 12 books == 24 permissions. Making or replicating node types is pretty easy, and since typically one node type can be used for several books, the net number of permissions will usually be less. The only case my suggestion degrades in terms of the total number of permissions is if you want a separate node type for each of many books. Obviously, this type-based permission will be overridden by admins with 'outline posts' or 'administer nodes' permissions.
If we can use the permission to add a node of a given type as implicit permission to add posts to a book (i.e. container) whose root node is of the same type, and if we can have either an "update" button or AJAX on the node form (in addition to what's already on the outline form) to load the right book tree, I think I can see a path forward that will make most everyone happy.
#69
Also - I plan to put the module on the Drupal diet - admin and export (printer-friendly) functions will get split into a .inc file (they use in common a large function, but aren't used on normal page views)
#70
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.
#71
minor change to block output to avoid one link hanging out one the left.
thinking about how to put book selection in the node form, but I think I need to refactor the various form element code to avoid excess duplication.
#72
form elements are refactored so that they are common to the node form and outline tab, but the submit functions need to be re-written.
#73
totally updated form and submit functions. Node-type permissioning. Prepared AJAX callback function per http://drupal.org/node/150859
<?php
/**
* AJAX callback to repalce the book parent select.
*
* @param $build_id
* The form's build_id.
* @param $book_id
* A book_id form from among those on the form.
* @return
* Prints the replacement HTML
*/
function book_form_update($build_id, $book_id) {
$cid = 'form_'. $build_id;
$cache = cache_get($cid, 'cache_form');
if ($cache) {
$form = $cache->data;
// Validate the book_id.
if (isset($form['book']['book_id']['#options'][$book_id])) {
$book_link = $form['#node']->book; print(drupal_to_js($form['book']['book_id']['#options']));
$book_link['book_id'] = $book_id;
// Get the new options and update the cache.
$form['book']['plid'] = _book_parent_select($book_link);
$expire = max(ini_get('session.cookie_lifetime'), 86400);
cache_set($cid, $form, 'cache_form', $expire);
$form_state = array();
$form['#post'] = array();
$form = form_builder($form['form_id']['#value'] , $form, $form_state);
$output = drupal_render($form['book']['plid']);
print(drupal_to_js($output));
}
}
exit();
}
?>
dmitrig01 promised to write the JS which should act on both the node form and the outline tab form to:
Needs testing!
#74
ignore
print(drupal_to_js($form['book']['book_id']['#options']));"in the sample code - it's not in the patch ;-)#75
minor cleanup of book.install
#76
Here is the long-awaited ajax!
#77
with the js file too
#78
for real this time
#79
fixing the base url
#80
ok, after absurdly slow progress with the JS I think I have it working on both the outline tab and the node form.
Also fixed some brokeness in the submit functions.
#81
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. ;)
#82
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.
#83
@joemoraca: the schema is changed, so either do a clean install or run the update code.
@webchick: I think from my conversations with Dries, Josh, etc. that the menu module is the right module to be considered the default site structuring tool. See: http://drupal.org/node/151583 for a patch moving us towards making that module work better. I think (as above) the book module should remain somewhat specialized for handling large number of nodes. A fundamentally important point is, however, that book and menu will use much of the same API code, so that gives us a smaller task in terms of performance optimizations in later patches.
Note that you may want to create a additional content types to use in books, especially if you want to test the interactions with use access for user != #1.
There is a script above to generate lots of book pages for 5.x if you want to try the update code. I'll try to post one for 6.x.
#84
@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
#85
@pwolanin: please come into IRC :P
#86
@webchick (and everyone) module attached - it will generate lots of content for you in the book hierarchy for testing purposes. Requires the patch on book module first, of course.
Note, rename to .zip or just unzip (extension changed to bypass upload restriction).
#87
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.
#88
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. :)
#89
@Dries - have you tried it with the new JS helper? All possible parents are available in the form this way, without any extra clicks. Any node can now be put into a book directly from node/add as you wished, etc. I think I worked pretty hard to address the concerns you had. Note webchick's review above and the module for generating content.
Also, this is a pretty complete re-write of the module using the mneu API, so I'm not sure I understand what you mean by the patch being "large"? Trying to read the patch itself seems unlikely to be productive in this case - like with this new issue where chx just posted the new version of the comment module: http://drupal.org/node/152417
#90
ah, there was a minor snafu with the JS and form code. This one should work better in picking a new parent ndoe.
#91
and - forgot some debugging code
#92
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.
#93
the RTL problem is, I think, not due to this patch, but is a bug in the existing code. However, attached patch fixes this by checking if the locale module exists, since the constant is defined there (why not in locale.inc?).
I think that when this patch goes in, integration with the path module would be a good next step. I'd like to be able to see each node in the book hierarchy have a path "fragment" so that a full path can be made of the fragments from a given node to the root.
I think the menu and book modules serve different purposes (see #63 and #83), and so is it does not make sense to try to combine them. However, both do use the menu system hierarchy in {menu_links} (with this patch).
#94
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:
#95
whoop. that bad query i listed above actually happens twice. attached patch fixes the one i missed...
#96
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: