Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.When the max depth is set to 2 or more levels, the tree renders all of the values irrespective of the depth, and when a value is clicked, the depth of that value is automatically adjusted to the set max depth. This is perfect.
IE: Two levels allowed, but three are rendered.
a
--b
----c
User clicks c
a
--b
--c
But at the max depth, passing 0 as the depth, the desired / expected behavior when the user clicks a item deeper than the first level, it should fallback to the root level
a
--b
c (was ----c)
But the tree actually allows any level to be rendered, which is the bug.
Using -1 as the depth limiter appears to work nicely
| Comment | File | Size | Author |
|---|---|---|---|
| #47 | interdiff_43-47.txt | 3.38 KB | rishabh vishwakarma |
| #47 | 1310642-47.patch | 1.49 KB | rishabh vishwakarma |
| #46 | 1310642-nr-bot.txt | 144 bytes | needs-review-queue-bot |
| #43 | 1310642-43.patch | 2.74 KB | _utsavsharma |
| #43 | interdiff_42-43.txt | 2.35 KB | _utsavsharma |
Comments
Comment #1
xjmThanks for the patch. The code looks straightforward to me.
Before/after manual testing and screenshots would be good here. To that end, it would be good to have steps to reproduce, including dummy code if needed. Tagging as novice for that.
Comment #2
xjmOh, here's a reroll for core/.
Comment #3
nod_It sounds to me like it's doing what it's supposed to. If you count from 0, depth = 2 will give you three levels. does depth = 1 gives the expected behavior? Or am I missing something here?
Comment #4
chrisshattuck commentedI played for a while in the UI with some different places where drupal_add_tabledrag() is used - in the book and menu interface - and don't quite understand where the problem is. I'd be happy to add some screenshots to explain what's going on and do some testing if I had an example of where this occurs. Thanks!
Comment #5
lokapujyaSeems like maxDepth of 0 (which is the default) means no max depth. So, this patch changes the default to -1 and then uses -1 to mean "no max depth" and intends to use 0 to mean that no children levels are allowed.
I added some book pages and played around with the maxDepth on the book module at admin/structure/book/. But, no child levels for Book doesn't make sense. Is there a use case where a drupal_add_tabledrag() with no child levels?
Comment #6
nod_block admin page
Comment #7
alan d. commentedUm.... this was not a book issue, it was a table drag issue.
Discovered when playing with the sandbox project https://drupal.org/sandbox/aland/1311114 Very specific use-case. Firstly the motive for this module was to have a taxonomy like system that could handle the geoname dataset - all 5 million records. (Try taxonomy post 10,000 or so records and watch your site die...)
Disclaimer, this sandbox has had no love in years, it throws notices on install, but appears to be functional still.
Each country has a set number of allowed sub-regions and this determines the max length, for AG there are 4 by default and this is what the screenshots reflect.
Added some regions to play with
Dragging them around, max depth is 4, no issues
On save, the child regions are hidden in the tree after two levels (just in case you try this out and go wtf...). This bypasses the memory limits both server side and client side with large dnd tables
Clicking a-a
Depth limited to 2, no issues. i.e we are already 2 levels deep (a, a-a)
Click list regions on a-a-a
We are currently on the 4th level. So no nesting allowed and the sets the depth parameter to limit it 0, however you can happily drag to any depth.
Comment #8
alan d. commentedComment #9
alan d. commentedComment #10
alan d. commentedI guess to address the above question more clearly, to allow this function to handle dynamic data where depth is potentially anything form 0 to xyz. Say a contrib module that limits the depth of a menu, many themes use a flat level menu, other menus at 2 level, etc, and you want to also limit this on the admin pages. Overrides theme_menu_overview_form() as well as other functions and sets the configuration to only to allow 'what_ever_this_is_in_drupal_8_again' to 1 [sorry, forgetting the config manager stuff atm), so $limit = 0 is passed to drupal_add_tabledrag()
drupal_add_tabledrag('menu-overview', 'match', 'parent', 'menu-plid', 'menu-plid', 'menu-mlid', TRUE, variable_get('what_ever_this_is_in_drupal_8_again', MENU_MAX_DEPTH) - 1);
A usable manual test procedure without pain for Drupal 8.x, say MENU_MAX_DEPTH was configurable, and then set to 1. (BTW, it's not and things blow up if you play with this constant). So update theme_menu_overview_form() to set the depth at 0 to mimic this.
- drupal_add_tabledrag('menu-overview', 'match', 'parent', 'menu-plid', 'menu-plid', 'menu-mlid', TRUE, MENU_MAX_DEPTH - 1);
+ drupal_add_tabledrag('menu-overview', 'match', 'parent', 'menu-plid', 'menu-plid', 'menu-mlid', TRUE, 0);
Edit a menu with items - http://drupal/admin/structure/menu/manage/account
Prior to the patch
You can nest the links
Post patch
Limited to a vertical dragging only.
Revert the 0 to MENU_MAX_DEPTH - 1 and edit the main administration menu and dnd to your hearts content.
http://drupal/admin/structure/menu/manage/admin
You can not go past 9 levels.
Comment #12
alan d. commented8: drupal-1310642-8-tabledrag-decouple-depth=0-from-meaning-unlimited-nesting-to-0-nesting.patch queued for re-testing.
Comment #14
alan d. commentedNote these were two different fatal testing errors triggered, one per test run:
First test: FormTest::testMultiFormSameNameErrorClass()
Second test: RenderTest::testDrupalRenderRenderCachePlaceholder()
Maybe re-test in a few days
Comment #15
lokapujyaOk, this seems like a reasonable change to allow only one level. Should we have some automated tests?
Just want to point out that the Block Admin page only has one level and seems to work (allows only vertical dragging.) However, I think it uses it's own tabledrag functionality.
Seems like the use of MENU_MAX_DEPTH should be investigated (separately.)
Comment #16
lokapujya.
Comment #17
alan d. commented8: drupal-1310642-8-tabledrag-decouple-depth=0-from-meaning-unlimited-nesting-to-0-nesting.patch queued for re-testing.
Comment #18
jhedstromComment #19
ankitgarg commentedRerolled.
Comment #20
lokapujyawhy is the change to the comment removed in the reroll?
Comment #21
adci_contributor commentedUpdate #19 in according to #20.
Please review.
Comment #24
nod_Comment #25
googletorp commentedRerolled the patch from #21
Comment #26
nod_Looks good to me. If someone could test that and it works as expected, feel free to RTBC I'm happy with the JS.
Comment #27
droplet commentedComment #28
maartendeblock commentedNo longer applies correctly, needs a reroll.
Comment #29
joshi.rohit100re-rolled.
Comment #31
droplet commentedComment #42
sahil.goyal commentedI have rerolled the #29 patch as it is no longer getting compatible with the 9.4.x and also attaching the reroll_diff, making the issue to same state as it needs tests for JS as added in tags as well.
Comment #43
_utsavsharma commentedFixed CCF for #42.
Please review.
Comment #44
_utsavsharma commentedComment #46
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #47
rishabh vishwakarma commentedAdding reroll against 10.1.x
Comment #48
ranjith_kumar_k_u commentedRe-rolled #43 for 9.5
Comment #49
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #50
bhaveshithape commentedAdding Rerole for d10, As previous patch failed to apply on d10.
Comment #51
smustgrave commentedRemoving credit for #50 as #47 still applies to 10.1, was duplicate patch. All patches should include an interdiff so that may have helped.
This still needs test coverage.
Comment #53
pameeela commentedLooked at this for Bug Smash triage, but I can't work out how to reproduce the issue. If anyone is facing this on their current D10 site could you please update the issue summary with specific steps to reproduce?