If somebody tries to reorder more than 31 pages in a book, he will face the fact that pages will not appear in the order s/he has put them in. The cause: the appropriate select has only 15 for #delta, which renders the reorder form useless if a book has more than 31 pages sharing the same depth. The solution: increase the #delta - 50 should be fairly enough, I think.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Boobaa’s picture

Status: Active » Needs review
FileSize
388 bytes

The attached patch increases the #delta to 50.

Galt1069’s picture

I made the change in the patch manually and am not seeing it reflected in the book weight dropdown. In other words, despite making the changes shown in the patch the weight value still goes from -15 to 15.

Boobaa’s picture

Version: 6.14 » 6.15
FileSize
1.31 KB

Patch updated as Galt is right in #2.

Boobaa’s picture

Version: 6.15 » 7.x-dev
FileSize
1.29 KB

Patch ported to D7.

willmoy’s picture

Title: More than 31 book pages in a book » Reordering fails with more than 31 book pages in a book
Category: feature » bug
Status: Needs review » Needs work

"If somebody tries to reorder more than 31 pages in a book... pages will not appear in the order s/he has put them in."

That's a bug, and borderline critical. According to @Galt1069 the patch doesn't work but even if it did, I think the logic needs changing. The patch just makes the bug rarer. Surely we need to make #delta = n / 2 + 1, where n is the number of pages in the book, so the bug cannot arise? It would probably be good to maintain a minimum #delta of 15 in any case too.

Denes.Szabo’s picture

I have just tested it. Everything seems good. The ordering worked with more than 31 book pages.

willmoy’s picture

What about a book with > 102 pages?

kaakuu’s picture

Subscribed.

Denes.Szabo’s picture

I just have seen the code. It is a quite compicated: the delta must depends from the parent book element.

Maybe, we should add a max_depth variable to the book admin page and when sb need more than 102 page then he can change it by the admin form.

Boobaa’s picture

FileSize
2.1 KB

@willmoy in #5: You're almost right: the #delta should depend on the number of chapters the current node's parent has, not the number of the chapters the book has - at least IMHO. So here is the patch updated accordingly - anyway, I must admit that it could be faster (in the first hunk, in the case of complete book reordering) that it's not a must to call db_query() for each node displayed, but the nodes sharing the same parent could share the result of that particular db_query().

@Denes.Szabo in #9: I don't know what you mean by that max-depth thingy, and I think adding another admin page for such a settings is making things even more complicated - and I think we can avoid it, anyway.

Boobaa’s picture

Status: Needs work » Needs review
FileSize
2.26 KB

Ok, depth-dependent #delta is simply wrong. Think about a book with 40 chapters, and with 40 subchapters each: on that book's reorder page (admin/content/book/%node) the #delta must be at least 801 (40*40/2+1), since the user should be able to pull all the second-level chapters (nodes) into the first level, while still being able to reorder them arbitrarily.

According to the above reason we should compute the #delta only once while displaying that page - the attached patch is updated in such a way.

willmoy’s picture

I will have a better look and actually test this as soon as I can but some comments in the mean time:

(1) Do we know what this looks like without javascript? Doesn't it revert to drop down lists? Pity the poor (blind?) sucker who tries to reorder 800 pages that way with only one free space at any one time. However, that may be outside the scope of this patch.

(2) Should be AS before siblings, as far as a quick look at core code suggests. The MySql manual says "it is good practice to be in the habit of using AS explicitly when specifying column aliases." [1] Similar advice from PostgreSQL. [2]

(3) Nit pick: 'siblings' should be renamed as the query doesn't learn anything about the book hierarchy. (I may be wrong on that but the description of bid is "The book ID is the {book}.nid of the top-level page."). grep -nR db_query modules/* | grep ") AS " doesn't show any particular naming scheme for calculated variables so I suggest something like 'total_nodes' 'node_count'.

[1] http://dev.mysql.com/doc/refman/5.0/en/select.html
[2] http://www.postgresql.org/docs/current/static/sql-select.html : "(You can omit AS, but only if the desired output name does not match any PostgreSQL keyword (see Appendix C). For protection against possible future keyword additions, it is recommended that you always either write AS or double-quote the output name.)"

Denes.Szabo’s picture

Status: Needs review » Reviewed & tested by the community

I have just tried it. It seems it works, I not found any problem.

Boobaa’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.25 KB

@willmoy in #12:

(1): You're right: this does work even with JS disabled, and yes, it's terrible to reorder pages without JS. But this is how drupal_add_tabledrag() works, so yes, that argument is outside of the scope of this patch, although does stand it's ground.

(2) and (3): The "sibling" AS alias isn't even needed since $result->fetchField() is being used - patch rerolled with this correction.

willmoy’s picture

FileSize
1.67 KB

Boobaa's patch at #14 is good as far as I can see, (although drupal_static() is overkill) but this patch avoids the extra database query by doing count() on the data array in the calling function and passing that as an extra parameter called $delta, which is also passed on when the function recurses.

Tested, but only up to 4 children. I will test more thoroughly later but no reason it should break.

I think the comment could perhaps use some rephrasing.

Boobaa’s picture

Status: Needs review » Needs work

@willmoy in #15: Looks like you've forgotten about the other hunk, which calculates the correct #delta for individual book node edit form's dropdown. Gonna look after it later, if nobody else does so.

Boobaa’s picture

Status: Needs work » Needs review
FileSize
2.76 KB

Rerolled @willmoy's patch in #15 to contain that other hunk which belongs to the individual book node edit form's weight dropdown's #delta calculation.

Denes.Szabo’s picture

Status: Needs review » Reviewed & tested by the community

I tested it, there was no problem. I used ~35 book pages.

willmoy’s picture

Status: Reviewed & tested by the community » Needs work

Yep. The extra query isn't avoidable in _book_add_form_elements() because the parent item drop down list doesn't include all the nodes in the book. (A node itself, its own children, bottom level nodes to prevent a book being more than MENU_MAX_DEPTH deep).

I don't much fancy the UX of exposing the weight system to end users, and extremely large books show why it's a bad idea, but we can't fix that in D7.

We do need to amend the comment:
"// Delta needs to be an integer greater than 15 for consistency with the UI
"// elsewhere"
no longer applies.

I'll re-roll in a few hours and take out the mention of the minimum, if no one objects/beats me to it. Other than that RTBC.

willmoy’s picture

Status: Needs work » Needs review
FileSize
2.6 KB

Rerolled with modified comment.

Boobaa’s picture

Status: Needs review » Reviewed & tested by the community

Looks more straightforward.

marcp’s picture

Any chance this will get backported to 6.x?

kaakuu’s picture

Will the codes in the patches work for 6x?
6x functionality is badly needed.

willmoy’s picture

Unless/until it's applied to HEAD, no chance of a backport. If the 7.x committers think it's worth backporting, I'll roll a patch for you, or if any of you can code, it should be a fairly easy adaptation.

marcp’s picture

FileSize
2.63 KB

Here's a backport for D6 -- the only place there's a real change from the D7 patch is in the db_query() call. The D6 version does it old-school since it doesn't have the new database API.

This should really make it into Drupal 6 -- how is this working on Drupal.org?

marcp’s picture

Any chance of this getting committed? Would love to see this in Drupal 6.

willmoy’s picture

#20: book-589440-10.patch queued for re-testing.

willmoy’s picture

Still works, still RTBC. I think committers are concentrating on upgrade and critical issues to get D7 out of the door, though.

JanZ’s picture

@willmoy in #5: Use of #delta = n / 2 + 1 is not practical. IMO it should be #delta n + 1

When having a large number of pages in a book (I'm working on a D6 site with more than 240 pages [short stories] in a book all on the same depth) you will have to start reordering pages around page 33 even when you had the foresight to start at weight -15.
Being confronted with the issue I was very happy to find the D6 patch of marcp (#27) but I did change delta to n + 1 to avoid having to spent time on reordering pages later on.

The only other little issue I had was that the book menu for these 240 pages would become very, very long. I changed it to only show 21 pages in the menu. When on the index page the first 21. When progressing from the first to the last, on page 12 you will see links to page 2-22 in the menu, on page 13 links to page 3-23 and so on and when getting to the last pages, the last 21 pages in the book.

For this I added some code to the function book_block in modules/book/book.module (see patch file).

Boobaa’s picture

@JanZ: your patch seems to lack what willmoy has already done in #20. Would you be so kind to reroll the patch to have willmoy's changes included as well? While at it, please have an eye on coding standards, too.

JanZ’s picture

FileSize
4.41 KB

@Boobaa I hadn't come across the coding standards yet. Thanks for the link.

Regarding the patch without willmoy's and marcp changes, that was intentional on my part.

Attached a patch for modules/book that includes marcp's changes and mine - for D6 (according to Drupal's coding standards hopefully).

marcp’s picture

@Booba and @JanZ - I think it would be best to keep the functionality in the D6 patch as close as possible to the D7 patch. The book menu reworking should be a separate issue and should first make it into D7.

JanZ’s picture

@marcp The book menu issue is definitely a separate issue, you are right.

I do notice that with the D6 patch there is a minor issue. Using the url http://drupalsite/node/add/book?parent=### (so adding a page from within the book you are working on) the dropdown with weight is adjusted to the number of pages.
However, changing within that page to another book will keep the weight dropdown using those numbers. Using that dropdown to enter a high numbered weight for a book with a low number of pages might be an issue (haven't tried that).
A similar issue arises with using the url http://drupalsite/node/add/book
Then the $node->book['bid'] is 0 (function _book_add_form_elements in book.module) and the dropdown for weight is at -15 to 15.

I don't know if the same thing happens in D7.

JanZ’s picture

FileSize
5.3 KB

Variation on the delta issue in book.module. Delta is set taking into account all books, but also the level on which the pages are (looking at the parent page, mlid in menu_links). This will make the dropdown for weight not larger than necessary and of the same length independent from the starting point of adding the book page.

The patch file contains the code for the book menu issue too.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

The logic of the patch in #20 seems flawed - we need to calculate the delta for each level, not pass it down from outside the recursion.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.58 KB

This patch is simpler, and I think it's more correct.

Status: Needs review » Needs work

The last submitted patch, book-589440-36.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.56 KB

oops - forgot to remove the prefix.

jhodgdon’s picture

In looking at this patch, it looks like maybe the first hunk of the patch should be taking into account that the existing weight could be larger than the computed value it's using for the min/max weight. Maybe:

       'weight' => array(
         '#type' => 'weight',
         '#default_value' => $data['link']['weight'],
         '#delta' => max($delta, abs($data['link']['weight']),
       ),

?

pwolanin’s picture

Hmm, in order to find the max weight, we'd have to iterate over all the links at that level.

jhodgdon’s picture

I guess so. But the line suggested above would at least make sure that the item's current weight is one of the choices shown on that item.

pwolanin’s picture

I see - some some items might have a different delta? wonder if that would throw the JS off?

mlncn’s picture

Priority: Normal » Critical
Issue tags: +dgd7
FileSize
2.46 KB

I'm going to call this critical because it happened to me ;-) Indeed, every book page over 30 at a level in the hierarchy to just stop reording (silent fail) is a pretty severe crippling of the book module, and that's the user experience with JavaScript enabled. (Especially since book module does not display of sub-level titles indented, rather one has to click to see them, so this likely pushes people away from deep hierarchies toward flat ones).

Tested pwolanin's patch, and it works, but i think the cases being brought up mean that both the admin page and the individual node form or outline pages need the same approach.

A Drupal book page can have its book, level, and weight all set at the same time (from the node add/edit form).

Thus it seems that the available weight must include all possible options– from a single book admin screen a person could collapse a book into a flat hierarchy, requiring deltas equal to the number of all potential book nodes.

I'm less concerned with the node/add form itself, though, since anyone setting book outline weights there while adding or editing a node is flying blind anyway.

Therefore, deltas (combined negative and positive) equal to the number of nodes in the single relevant book is enough.

Could we just not divide by two (as JanZ suggests in #29 and deem this sufficient range and flexibility that any weight that manages to get outside this range can be truncated to within this range?

This patch does not do that but i would be happy to re-roll to double our delta to give administrators room to maneuver.

I haven't found a representation of the book tree that might be already loaded or cached from which to get maximum weights, and (even though i said i don't care about them) i do think the range should be the same for an individual node as for on the edit title and weights admin page.

So here's a patch that adds a bit of a performance hit for editing or outlining nodes (loading a book subtree that it did not before), but covers all our bases for a given level of a book hierarchy. It also adds 1 to the maximum range of weights found, so that one can always put a node at the very beginning or end, but i'll reroll without this if that addition isn't wanted.

pwolanin’s picture

Status: Needs review » Needs work

Do we really need to foreach the whole tree? Do we really want a delta that might be in the thousands?

catch’s picture

Priority: Critical » Normal

This is just a normal bug, not critical, has never worked properly. I'll split the difference at priority major 'cos I think this is a more general issue with drag and drop implementation in core, menu has a similar limitation iirc, taxonomy doesn't but also crashes your site over a few thousand terms.

pwolanin’s picture

Loading hundreds or more of selects each with thousands of weights seems like a no-go to me in terms of performance.

Let's fix the main case that's experienced - the edge cases are not readily fixable.

mlncn’s picture

Status: Needs work » Needs review

The foreach is just for the current level of the hierarchy, as you noted in #40. Digging into the undefined variable warning makes clear that running book_menu_subtree_data() as it is currently formed is just way too much extra overhead for this purpose, though.

The alternative consistent approach, in my opinion, is to only check the count of book nodes at a given level, as the your #38 patch does also, and force any values that happen to be outside of this range (because they transferred in from another book/level) into this range.

So long as we are going to let people reweight from within individual nodes, they should have the same range as they do on the admin page. This still means loading the menu tree (or accessing saved info) for editing/outlining individual nodes to get that count, so not much of a performance gain, just dropping the foreach.

What the range has to be is such a judgement call i feel like making it a theme function to make it easily overridable!

Is there an issue where changing the select dropdown to a textfield has been discussed? I cannot find one, and that would make this moot. The range could be -1000 to 1000 happily.

@catch-- yes: i consider this critical for book module, but not for Drupal as a whole! I swear i've had books with well over 30 pages at one level in Drupal 6, so i think this issue is new-- what are the drag and drop issues you refer to? Screen readers? ...it'd be awesome to use the change-weights-and-titles-at-the-same-time approach used by book for menus!

mlncn’s picture

Status: Needs review » Needs work

Didn't mean to change status.

marcingy’s picture

Priority: Normal » Major

Changing to major as per tag.

MustangGB’s picture

Tag update

Starminder’s picture

subscribing - this was not a problem too terribly long ago, but has somehow crept in. It's killing me, I have an 800 page book that's out of order and i can't fix it.

KarenS’s picture

I have a different idea. We need to create a simple system for core to handle the common cases but make it possible for contrib or custom modules to alter the value to take care of the edge cases. Currently this hard-coded value in core is both too small and not overrideable.

So we could change the hard-coded delta to be a value derived from a helper function that invokes hook_book_delta($bid). For core, we could either return 15 or 30 regardless of the $bid for a simple system with no performance overhead, but contrib or custom modules would then be able to use the $bid and create variables or tables to keep track of the 'right' delta for various books in the system.

For instance, I am trying to create a book_parent module that creates a different content type for a book parent to store additional information about the book as a whole. That module could also compute the an appropriate delta for the book when any of the book pages are updated, then implement hook_book_delta() with a simple lookup to find the right delta for the current $bid.

Boobaa’s picture

KarenS: sounds good, but should one install another module just to be able to reorder more than 31 pages in a book, even in the case if core could serve all the needs of the site anyway?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.59 KB

Testing this out in the UI, the JS seems ok with the change suggested by jhodgdon in #39.

KarenS’s picture

@Boobaa, the problem I see is anything that involves adding a hard-coded value has an option to be the wrong value but there is no way to override it without hacking core or doing lots of wonky things. There should be a reasonable default plus a way to override.

pwolanin’s picture

I'm suspecting the tabledrag JS is still wrong in some way.

Boobaa’s picture

@KarenS, as I see this patch changes the default to an always-reasonable value. I get your point of view, but why do we need a way to override a default that's always reasonable?

pwolanin’s picture

Status: Needs review » Needs work

I'm trying to understand tabledrag.js, but my best guess from a quick look is that it expect all rows to have the same delta, but the code is a bit obscure to me.

Also, CNW since I forgot a call to abs().

Discussing with ksenzee, it seems likely that for the JS to work reliably, every item displayed in the admin page for the DnD table must use the same delta. This may, however, make us sad in as much as the JS is likely to pick big numbers when we'd prefer it to stay around 15.

mlncn’s picture

Status: Needs work » Needs review
FileSize
1.95 KB

Reroll. With this patch, at least the admin reorder page works, and the node edit page doesn't break it. I think the node edit page should be made aware of how big the delta needs to be, but this at least fixes admin reordering, which i think should be a D7 release blocker.

Status: Needs review » Needs work

The last submitted patch, book-589440-59.patch, failed testing.

jhodgdon’s picture

I don't think that test failure has anything to do with this patch:

The value of node last_comment_timestamp is the comment #3 created date.
comment.test line 474
CommentInterfaceTest->testCommentNodeCommentStatistics

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: -dgd7

#59: book-589440-59.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +dgd7

The last submitted patch, book-589440-59.patch, failed testing.

jhodgdon’s picture

Now it's having trouble checking out the repository:
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [:pserver:anonymous:anonymous@cvs.drupal.org:/cvs/drupal/drupal].

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: -dgd7

#59: book-589440-59.patch queued for re-testing.

jhodgdon’s picture

Issue tags: +dgd7

#59: book-589440-59.patch queued for re-testing.

mlncn’s picture

Priority: Major » Critical

Does this have to be critical to prevent D7 from being released with broken ordering for large-ish outlines? I only re-rolled the patch so i'm considering RTBCing.

jhodgdon’s picture

Priority: Critical » Major

Have pwolanin's comments in #58 been addressed?

Also, this is not a critical issue, see above #45.

mlncn’s picture

FileSize
1.67 KB

Outlining breaking at 30 nodes would be a sad way to release Drupal 7.

#58 is addressed now; proper use of absolute to catch outlier negative as well as positive weights when assigning deltas on both the node/edit form and the bulk outline form.

marcp’s picture

I tested this patch out pretty well and it seems to fix the bug. Ran across #1007746: Reordering fails with more than 100 items in a menu which could use a similar fix.

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

i'm going to take "pretty well" as RTBC because, have i mentioned, we shouldn't release with this broken?

marcp’s picture

I got this to break by, I think, moving a small sub-tree up a level but haven't reproduced it yet. The patch is solid for moving items around within a given level of the tree.

There's a patch for devel at #303143: Generate hierarchical books that makes testing this by hand a little easier.

If pwolanin's "Let's fix the main case that's experienced - the edge cases are not readily fixable" comment above in #46 still holds then this is RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This patch is a nice improvement. I've been bitten by this before too on Drupal.org. The patch is also a lot less invasive than I thought it would be.

In the interest of fixing this issue before D7.0:
Committed #69 to HEAD.

We should back-port this to D6 as well, but first, we need tests. ;) Ha! ;)

Looks like a similar fix for menu is covered at #1007746: Reordering fails with more than 100 items in a menu. Is there one for taxonomy, too?

tarzadon’s picture

I hope this will be backported to D6 soon. We really need the drag and drop reordering to work. We're using the book-589440-2-D6.patch right now to do the reordering.

mlncn’s picture

I'm unsure how to write tests for this, because as tarzadon writes, what we all actually want to *work* is the drag-and-drop re-ordering. Is it possible to interpret a page well enough that we can read the order of nodes on the book parent page, after submitting a form with fifty generated nodes, specifically ordered? Or should it just be code that looks for the maximum weights in the (hidden by javascript) select dropdowns on the re-order page, not submitted yet?

tarzadon’s picture

I just noticed that drag-and-drop works (in D6) if you have less than 31 nodes published. I have about 40 right now attached to a book.

I also noticed that even after applying the book-589440-2-D6 patch, drag-and-drop still assigns the nodes weights from -15 to 15 (JS I presume?). I was able to drag-and-drop the 31st node everywhere except at the bottom of the list, because every node over 30 will have a weight of 15 at the bottom list.

Not sure if this helps anyone.

tarzadon’s picture

Another point is when I add a node, the options for weights only go from -15 to 15. It's only when I edit the node that I see the other weights.

jhodgdon’s picture

Drupal 6 has not been fixed yet, so yes it is broken in Drupal 6.

TomiMikola’s picture

FileSize
2.13 KB

I had difficulties in dealing with large books ( > 1000 pages per each, 3-4 levels in hierachy). Especially the book administration page drag and drop didn't work when items had to be moved between chapters. So I modified the delta formation as follows. Now it works without troubles. It's for Drupal 6.

TomiMikola’s picture

FileSize
2.21 KB

Updated #79 since delta values were too small on book edit form.

I believe that this would be more workable for large and deeply hierarchical books, than the one presented in #69.

rolfmeijer’s picture

subscribe

Boobaa’s picture

@TomiMikola: Am I getting you right that you count the book's pages 8 levels deep (and not deeper), then count the topmost-level chapters, and take these two's quotient? This approach has two major implications, at least:

1. It may bail out with a division by zero (think about a book that has only one topmost-level chapter).

2. It prevents a deeply hierarchical book to be flattened out in one step properly.

Seriously, this approach seems to be simply wrong: if a book has 1000 pages, delta must be at least 501, if I'm taking every aspect right into account. Anyway, feel free to convince me that your approach works just fine even in the aforementioned (I must admit) corner cases as well.

TomiMikola’s picture

@Boobaa: Thank you for your good feedback! I'm glad you had the time to review the patch. You are absolutely right that the division operation provides an error when the book hierarchy is flat.

Anyway, the problem I tried to chase is that #69 solution calculates different delta value of each subtree in the book hierarchy. Due to this approach weight adjustments do not work when you move the book pages between subtrees, if their deltas vary a lot.

I run a simple manual test applying #69 patch for a vanilla Drupal 6.20 installation, generated 50 pages for a book. Please see the attached screen shots. I also updated my patch here, applied it and run the same test. See the difference in the screen shots.

jhodgdon’s picture

Is it possible that rather than having a select list with lots and lots of entries, we could just have a numeric text field?

Boobaa’s picture

Status: Needs work » Needs review
FileSize
1.73 KB

The idea jhodgdon suggests sounds great and does not break client-side tabledrag JS. Here it is in the form of a patch; while at it, it reverts things made in #69 and webchick has committed in #73.

pwolanin’s picture

Version: 7.x-dev » 8.x-dev

bugs need to be fixed in HEAD first

jhodgdon’s picture

So I'm confused about this latest patch. Does it still support drag and drop if it is not a weight field any more?

Boobaa’s picture

FileSize
1.73 KB

@jhogdon: Yes.
@pwolanin: Here you are.

pwolanin’s picture

So... is there any validation required for the values? Seems a likely if switching from a select box to a text area (in which case this is CNW)? Valid values are still just integers?

Boobaa’s picture

@pwolanin: You got reason. Will look into it.

pwolanin’s picture

Status: Needs review » Needs work

ok, setting to CNW until there is some validation

Boobaa’s picture

Status: Needs work » Needs review
FileSize
1.91 KB

Here it comes.

pwolanin’s picture

Status: Needs review » Needs work

Progress, but _element_validate_integer() is private to Field module, and should not be called from Book.

Since there are multiple places in core where we might want to replace the weight element with such a open text integer, perhaps we should define a new core element type?

At the least, in Book or elsewhere in core, we'll need ot make a public version of that validation function.

pwolanin’s picture

Or, you could propose making that a public function and/or moving it more centrally into core as a public function since Feild module is required by core.

pwolanin’s picture

adding tags

cweagans’s picture

Can _element_validate_integer() be made into a public function? I don't see how that could introduce any problems, and that function is generic enough that it would be useful to other modules. It kind of seems that a generic validator function is something that should be public anyways.

cweagans’s picture

Whoops. Re-tagging.

Boobaa’s picture

Status: Needs work » Needs review
FileSize
2.12 KB

Firstly, as chx recommended on IRC, here is the same patch with some additional comments about why do we have a textfield for an element named weight.

Secondly, I'm not sure if _element_validate_integer() should be the only one to be moved to form.inc; it looks like _element_validate_integer_positive() and _element_validate_number() should be candidates, too. Anyway, I'm not sure if this change fits into this issue, or it should be a different one.

Lastly, I for one don't think we need a new element type in core – though this very same approach would be welcomed ie. on reordering multivalue fields (think image galleries with hundreds of pictures, for example).

chx’s picture

Status: Needs review » Reviewed & tested by the community

Stop here. This is the exact kind of patch where the patch should just go in and a followup issue should be opened where these validators move into form.inc with a better name (and no underscore).

chx’s picture

Status: Reviewed & tested by the community » Needs work

Uh what i was thinking, it needs a test first.

Boobaa’s picture

OK, let me sum things up.

From the server/PHP/Form API side it doesn't matter whether the weight element is a weight or a textfield as long as the textfield returns integers. This is ensured by using the aforementioned _element_validate_integer.

From the client/browser/JS/jQuery side it doesn't matter whether the weight widget is a select or a textfield, since the misc/tabledrag.js code (lines 768-793) can handle both.

The interconnection between the two is simple: since tabledrag.js will generate nothing but integers in both the cases (select and textfield), all we have to do is validate them server-side for being integers. (Things are only a bit more messed up with tabledrag.js, though: it does takes the allowed/available values into account in the case of selects; OTOH it simply generates integers in the case of textfields.)

Anyway, I still don't know how to address all of these in a test.
1. The bug is already solved in #69/#73, so I can't forge a failing test: the committed version "works" for (almost) unlimited number of book pages (but can generate _huge_ HTML for big number of them: every page's weight select should have all the page numbers as select options).
2. The reorder works before and after this patch, almost the same (from server-side POV).
3. AFAIK Selenium and/or other JS testing framework is not available for D8 for the time being, not speaking about D7 or even D6.

So, please give me some advice how to write test(s) that:
1. fails with a select (before the patch),
2. passes with a textfield (after the patch).
I think simply grepping the HTML for select/textfield widgets is not quite the thing to do.

chx’s picture

Status: Needs work » Reviewed & tested by the community

OK I am convinced it can't be tested and that it won't cause issues. I am surprised tabledrag already contains support for textfields but it does:

        else {
          // Assume a numeric input field.
Boobaa’s picture

According to #102, removing "Needs tests" tag.

The patch applies cleanly (without offsets) to 7.x HEAD as well, thus removing "needs backport to D7" tag as well.

It doesn't apply to 6.x HEAD at all, expect a -D6 patch soonish.

catch’s picture

Issue tags: +Needs backport to D7

Please don't take out the "Needs backport to D7" tag, this is being used to track the fix for backport to Drupal 7. It doesn't mean that the patch itself needs to be ported (in some cases it might, some not).

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

I still object to the use of _element_validate_integer() if it's not part of the public API.

AFAIKT, it's not actually used in core at the moment.

sadly, Views seems to be using one of these other non-public validate functions already:

views/plugins/views_wizard/views_ui_base_views_wizard.class.php:140:      '#element_validate' => array('_element_validate_integer_positive'),
views/plugins/views_wizard/views_ui_base_views_wizard.class.php:266:      '#element_validate' => array('_element_validate_integer_positive'),
Dave Reid’s picture

See also #1174444: Make the _element_validate_* functions in field.module available for all contrib modules to use - for all intensive purposes they're public API since they're available in field.module (a required module for core).

catch’s picture

Boobaa’s picture

Status: Needs work » Needs review

Related issue got committed, let's see what the testbot says.

richardmetzger’s picture

I just might want to add that this patch itself might not be enough to make reordering work.

In my case it was for a D6 installation, where I applied #83 and still didn't get the book reordered. After some investigations I found that submitting the form at ?q=admin/content/book/nnn didn't get processed because the POST carried more variables than allowed by the default of "suhosin.post.max_vars" (=200).

So setting e.g. "suhosin.post.max_vars=2048" in php.ini was necessary as well.
(Richard)

Boobaa’s picture

Please stick to the issue itself; various PHP implementations' issues do not belong here.

richardmetzger’s picture

@Boobaa
With all due respect, I can understand your position from the (your) point of view as Drupal core developer. But that still doesn't remove potential dependencies on the underlying SW stack when considering the error as seen by the user when "reordering doesn't work". Which is of course nothing that you can fix - or should code around.
Again - my apologies - but I just wanted to point out an additional dependency beyond your control, that could (and did in my case) contribute to the error that reordering failed. Also my sole intention with #109 was to provide some feedback to other D6 admins/users, that might have the same problem until an "official" D6 backport is available.
It wasn't my intention to criticize any of your work - to the contrary - thanks for working on Drupal.

catch’s picture

#107: book-589440-98.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, book-589440-98.patch, failed testing.

Boobaa’s picture

Status: Needs work » Needs review

#107: book-589440-98.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D6, +dgd7, +Needs backport to D7

The last submitted patch, book-589440-98.patch, failed testing.

xjm’s picture

Just a note, catch requested retests to demonstrate that the patch needs to be rerolled. In retrospect just setting these NW with a comment might have been a good idea. :) In any case, the patch needs to be rerolled on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

xjm’s picture

Issue tags: +Novice

Tagging as novice to reroll the patch.

kathyh’s picture

FileSize
2.16 KB

Re-rolled for D8 /core change

kathyh’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright, I think this is ready again.

chx’s picture

Note #102 : this can't be tested (cos you'd need to litereally drag the rows in a JS capable browser to see the problem)

catch’s picture

Version: 8.x-dev » 7.x-dev

This is the essentially the same patch that was RTBC'ed in May, just been stuck for a while it looks like. Committed/pushed to 8.x, moving back to 7.x.

webchick’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

Hm. I'm not sure about this for D7.

It goes from looking like this:

Select box for weight

To looking like this:

Big wide text field for weight

We fixed the actual major bug back in January, before D7's release. I don't understand what problem moving this weight field (unlike all other weight fields in the whole rest of the system) to a textfield is trying to solve? The only thing I could parse out was from Boobaa's comment in #101 that it reduces the length of HTML on the admin/content/book page. If that's it, it's not worth introducing this UI change in D7, I don't think. And if it's a "lots of things with weights" problem, then let's fix it properly for all weight fields, not just in Book module, IMO.

So, I'm not sure what to do here. If D8's going to change this to a text field, at the very least it needs a width on the field so it doesn't look so patently ridiculous. But unless someone can demonstrate what is broken enough in D7 to justify this inconsistent UI change, I'm inclined to leave D7 as-is, with the original bug fixed and keeping the select box. But in the meantime, poor D6 still has the original major bug that's been held up by this discussion. :\

Moving to 8.x and "needs work". If it were up to me, I'd roll back the text field patch in book module and introduce a #type = weight in a separate issue that switched it to a text field if the amount gets up above some threshold (500 or whatever), then bump this issue back down to 6.x so we can get the original bug fixed, before all the kittens got murdered in here. However, it's not up to me. :)

catch’s picture

OK so I just read back through the issue and this is what I think happened:

webchick committed the patch on Jan 3rd. However left it as needs work and asked for tests before we could backport to D6.

Somewhere in the following months the issue took a complete diversion into select vs. textfield.

In hindsight, the patch should either have been held up for tests before commit to D7 (or we decide we don't need them), or we should not have held up the D6 backport. But here we are in November...

So:

1. If we are thinking about a weight element or consistently applying this across drag and drop weight fields, that should go in a new issue. We can rollback the last patch committed here, or leave it in and change it as part of that issue.

2. If we can't test this, then there is no point holding up the D6 patch on it - that means either this patch should be moved to D6, or we open up yet another follow-up for the backport and just mark this one closed.

I'll let people who've been following the issue closer than me validate this before taking action on either of those though.

catch’s picture

Version: 8.x-dev » 6.x-dev
Status: Needs work » Patch (to be ported)

Just seen #1346760: Add a scalable weight select element which is exactly the same issue for performance, and supports webchick's suggestion that this should be a centralized weight element which we can use for all drag and drop forms.

That means this goes all the way back to Drupal 6 for the straight backport I think. If there are more follow-ups here for 7.x or 8.x let's do them in new issues.

webchick’s picture

Yay! Thanks, catch! :)

webchick’s picture

Issue tags: -Needs backport to D7

Removing backport tag, as this is done.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.11 KB

Here's my first stab at a D6 port. Since element_validate_integer() doesn't exist in D6, I added it as a helper function in book.module. I'm not sure if this is best practice by any means so please, correct me so I can learn!

tarzadon’s picture

Version: 6.x-dev » 6.22

We have been using the book-589440-2-D6.patch for over a year now. I just noticed today that after I add the 31st node, it is sorted correctly in /admin/content/book/### and has the correct weight assigned. But, when I generate a view using Book: Hierarchy, the 31st+ nodes always come last on the list. I'm not sure if the problem is with Views or the Book Module...

xjm’s picture

Version: 6.22 » 6.x-dev
tarzadon’s picture

Version: 6.x-dev » 6.22

I've answered my own question.
I was using the patch from http://drupal.org/node/817748#comment-4033620 and for some reason it stopped working. I applied the patch from http://drupal.org/node/817748#comment-4265772 and now the Book:Hierarchy views shows properly.

cweagans’s picture

Version: 6.22 » 6.x-dev

Please do not change the version again. This needs to be fixed in dev, then it might be rolled into a release.

Albert Volkman’s picture

FileSize
2.11 KB

Renaming my file from #128.

tarzadon’s picture

The patch works. Drag and drop also works properly now.

RedRat’s picture

Status: Needs review » Reviewed & tested by the community

I have tested patch from #133 on my 6.26 installation and it works fine. Any chance to see it in the next release?

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/modules/book/book.admin.incundefined
@@ -182,9 +182,13 @@ function _book_admin_table_tree($tree, &$form) {
-        '#type' => 'weight',
+        // Using a textfield and a validator instead of a simple weight select
+        // to avoid unnecessarily oversized HTML for books with hundreds of
+        // pages.
+        '#type' => 'textfield',
+        '#element_validate' => array('_book_validate_integer'),
         '#default_value' => $data['link']['weight'],
-        '#delta' => 15,
+        '#title' => t('Weight for @title', array('@title' => $data['link']['title'])),

At least document why are you not using a weight field. What are the UI implications of this change?

+++ b/modules/book/book.moduleundefined
@@ -1093,3 +1095,11 @@ function book_menu_subtree_data($item) {
+// Helper function borrowed from upstream.
+// http://api.drupal.org/api/drupal/includes--form.inc/function/element_validate_integer/7
+function _book_validate_integer($element, &$form_state) {
+  $value = $element['#value'];
+  if ($value !== '' && (!is_numeric($value) || intval($value) != $value)) {
+    form_error($element, t('%name must be an integer.', array('%name' => $element['#title'])));
+  }

The documentation of this function does not use phpdoc and does not explain what it does. Where does it come from is much less interesting.

RedRat’s picture

Well, I have tested patch from #133 more thoroughly and find that on a large books with about 200 pages last pages can't be reordered: when I reorder them and press "Save..." all returns to previous state. But if I change parameters of page directly in the DB, then it starts to place correctly.

Shoffner’s picture

Tried using this patch on a drupal 7 site. I can update weight from the node edit page, but any changes made to the weight when attempting to edit order on the book hierarchy page get lost upon hitting save. It happens if I'm using the js drag and drop, or if I'm typing in integers.

Shoffner’s picture

Version: 6.x-dev » 7.22
Priority: Major » Normal

Tried using this patch on a drupal 7 site. I can update wight from the node edit page, but any changes made to the weight went attempting to edit order on the book hierarchy page get lost upon hitting save. It happens if I'm using the js drag and drop, or if I'm typing in integers.

michaelkoehne’s picture

Same problem in 7.21. I can't reorder the book.
450+ pages, 200pages in same level.

Waiting for working patch..

michaelkoehne’s picture

Have tryed patch #98 and it doesn't work...
can somebody pls help?

joaomachado’s picture

I too need the book module to handle more than 31 weights, I need more like 67 in D7. It looks like the patch has been created but not moved to core?

Can anyone shed some light on this, it is kind of a deal breaker for me on my current project...

Joao

Bastlynn’s picture

Issue summary: View changes

There is another possible cause for this one, I want to drop this info here for anyone who's waiting for a patch or hasn't been able to get the solutions listed here working and was directed to this issue:

If you are having trouble with drag and drop not saving for : taxonomy, menus, book module or other such things...

AND

You only see this behavior when the list is long. Aka, it works as expected until you reach an arbitrary number of elements being manipulated.

Then you should take a close look at your PHP settings for max_input_vars.

By default this .ini setting is around 1000 elements, and as your list gets longer and longer you can easily hit this limit. If Drupal hits that limit, you will see partial returns in form values as well as an E_WARNING. More importantly - form validate and form submit functions are NOT called and the form fails. (This should probably be a situation where a watchdog gets thrown to keep this from being a mystery for the developer.)

More information on setting max_input_vars here:
http://php.net/manual/en/info.configuration.php#ini.max-input-vars

Don't arbitrarily change this value - only change it if you need it. This is a security concern to avoid getting slammed with over-large DoS hash collision attacks. So use with care and only when absolutely needed.

avisconti’s picture

As Bastlynn noted, increasing my max_input_vars in php.ini did the trick when none of the patches worked for my long book. I then added further hundreds of pages and ran into the issue again (reorders not saving), bumped max_input_vars a bit higher, and it was fixed. Thanks, @Bastlynn!

Sequencing.com’s picture

Also experiencing this issue on Drupal 7. On a book with more than 50 weights reordering the child pages fails.

priya.chat’s picture

Assigned: Unassigned » priya.chat
priya.chat’s picture

Assigned: priya.chat » Unassigned
heddn’s picture

Version: 7.22 » 6.x-dev
Issue tags: -Needs backport to D6, -Novice, -dgd7

A fix was committed to D7 & D8 already. See the nice summary in #125. Moving this back to D6 for backport.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.