Posted by Boobaa on September 27, 2009 at 7:42pm
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | book.module |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | dgd7, needs backport to D6, Novice |
Issue Summary
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.
Comments
#1
The attached patch increases the
#deltato 50.#2
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.
#3
Patch updated as Galt is right in #2.
#4
Patch ported to D7.
#5
"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.
#6
I have just tested it. Everything seems good. The ordering worked with more than 31 book pages.
#7
What about a book with > 102 pages?
#8
Subscribed.
#9
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.
#10
@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.
#11
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.
#12
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.)"
#13
I have just tried it. It seems it works, I not found any problem.
#14
@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.#15
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.
#16
@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.
#17
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.
#18
I tested it, there was no problem. I used ~35 book pages.
#19
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.
#20
Rerolled with modified comment.
#21
Looks more straightforward.
#22
Any chance this will get backported to 6.x?
#23
Will the codes in the patches work for 6x?
6x functionality is badly needed.
#24
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.
#25
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?
#26
Any chance of this getting committed? Would love to see this in Drupal 6.
#27
#20: book-589440-10.patch queued for re-testing.
#28
Still works, still RTBC. I think committers are concentrating on upgrade and critical issues to get D7 out of the door, though.
#29
@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).
#30
@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.
#31
@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).
#32
@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.
#33
@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.
#34
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.
#35
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.
#36
This patch is simpler, and I think it's more correct.
#37
The last submitted patch, book-589440-36.patch, failed testing.
#38
oops - forgot to remove the prefix.
#39
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']),
),
?
#40
Hmm, in order to find the max weight, we'd have to iterate over all the links at that level.
#41
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.
#42
I see - some some items might have a different delta? wonder if that would throw the JS off?
#43
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.
#44
Do we really need to foreach the whole tree? Do we really want a delta that might be in the thousands?
#45
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.
#46
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.
#47
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!
#48
Didn't mean to change status.
#49
Changing to major as per tag.
#50
Tag update
#51
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.
#52
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.
#53
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?
#54
Testing this out in the UI, the JS seems ok with the change suggested by jhodgdon in #39.
#55
@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.
#56
I'm suspecting the tabledrag JS is still wrong in some way.
#57
@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?
#58
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.
#59
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.
#60
The last submitted patch, book-589440-59.patch, failed testing.
#61
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
#62
#59: book-589440-59.patch queued for re-testing.
#63
The last submitted patch, book-589440-59.patch, failed testing.
#64
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].
#65
#59: book-589440-59.patch queued for re-testing.
#66
#59: book-589440-59.patch queued for re-testing.
#67
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.
#68
Have pwolanin's comments in #58 been addressed?
Also, this is not a critical issue, see above #45.
#69
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.
#70
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.
#71
i'm going to take "pretty well" as RTBC because, have i mentioned, we shouldn't release with this broken?
#72
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.
#73
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?
#74
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.
#75
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?
#76
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.
#77
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.
#78
Drupal 6 has not been fixed yet, so yes it is broken in Drupal 6.
#79
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.
#80
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.
#81
subscribe
#82
@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.
#83
@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.
#84
Is it possible that rather than having a select list with lots and lots of entries, we could just have a numeric text field?
#85
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.
#86
bugs need to be fixed in HEAD first
#87
So I'm confused about this latest patch. Does it still support drag and drop if it is not a weight field any more?
#88
@jhogdon: Yes.
@pwolanin: Here you are.
#89
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?
#90
@pwolanin: You got reason. Will look into it.
#91
ok, setting to CNW until there is some validation
#92
Here it comes.
#93
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.
#94
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.
#95
adding tags
#96
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.
#97
Whoops. Re-tagging.
#98
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).
#99
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).
#100
Uh what i was thinking, it needs a test first.
#101
OK, let me sum things up.
From the server/PHP/Form API side it doesn't matter whether the
weightelement 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
weightwidget is a select or a textfield, since themisc/tabledrag.jscode (lines 768-793) can handle both.The interconnection between the two is simple: since
tabledrag.jswill 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 withtabledrag.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.
#102
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.
#103
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.
#104
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).
#105
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'),
#106
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).
#107
Pre-emptive re-roll, just marked #1174444: Make the _element_validate_* functions in field.module available for all contrib modules to use RTBC.
#108
Related issue got committed, let's see what the testbot says.
#109
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)
#110
Please stick to the issue itself; various PHP implementations' issues do not belong here.
#111
@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.
#112
#107: book-589440-98.patch queued for re-testing.
#113
The last submitted patch, book-589440-98.patch, failed testing.
#114
#107: book-589440-98.patch queued for re-testing.
#115
The last submitted patch, book-589440-98.patch, failed testing.
#116
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.
#117
Tagging as novice to reroll the patch.
#118
Re-rolled for D8 /core change
#119
#120
Alright, I think this is ready again.
#121
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)
#122
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.
#123
Hm. I'm not sure about this for D7.
It goes from looking like this:
To looking like this:
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. :)
#124
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.
#125
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.
#126
Yay! Thanks, catch! :)
#127
Removing backport tag, as this is done.
#128
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!
#129
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...
#130
#131
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.
#132
Please do not change the version again. This needs to be fixed in dev, then it might be rolled into a release.
#133
Renaming my file from #128.