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

CommentFileSizeAuthor
#29 1310642-29.patch1.56 KBjoshi.rohit100
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,895 pass(es). View
#25 adding-1310642-25.patch1.8 KBgoogletorp
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,772 pass(es). View
#21 interdiff-1310642-19-21.txt679 bytesadci_contributor
#21 tabledrag-decouple-depth-1310642-21.patch1.78 KBadci_contributor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch tabledrag-decouple-depth-1310642-21.patch. Unable to apply patch. See the log in the details link for more information. View
#19 drupal-1310642-19-tabledrag-decouple-depth%3D0-from-meaning-unlimited-nesting-to-0-nesting.patch1.77 KBankitgarg
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,902 pass(es). View
#10 drupal-admin-menu.png12.29 KBAlan D.
#10 drupal-user-menu-0-depth-2.png6.02 KBAlan D.
#10 drupal-user-menu-0-depth.png13.49 KBAlan D.
#8 drupal-1310642-8-tabledrag-decouple-depth=0-from-meaning-unlimited-nesting-to-0-nesting.patch2.33 KBAlan D.
PASSED: [[SimpleTest]]: [MySQL] 59,759 pass(es). View
#7 countries-region-5.png3.17 KBAlan D.
#7 countries-region-4.png3.51 KBAlan D.
#7 countries-region-3.png4.42 KBAlan D.
#7 countries-region-2.png7.52 KBAlan D.
#7 countries-region-1.png6.96 KBAlan D.
#2 tabledrag-depth-1310642-2.patch2.22 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,810 pass(es). View
tabledrag-0-depth-parent.patch2.18 KBAlan D.
PASSED: [[SimpleTest]]: [MySQL] 33,335 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

xjm’s picture

Thanks 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.

xjm’s picture

FileSize
2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 33,810 pass(es). View

Oh, here's a reroll for core/.

nod_’s picture

Status: Needs review » Needs work

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?

chrisshattuck’s picture

I 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!

lokapujya’s picture

Issue summary: View changes
Issue tags: +Needs reroll

Seems 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?

nod_’s picture

block admin page

Alan D.’s picture

Um.... 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.

Alan D.’s picture

Issue tags: -Needs reroll
FileSize
2.33 KB
PASSED: [[SimpleTest]]: [MySQL] 59,759 pass(es). View
Alan D.’s picture

Status: Needs work » Needs review
Alan D.’s picture

I 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.

Alan D.’s picture

Alan D.’s picture

Note 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

lokapujya’s picture

Ok, 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.)

lokapujya’s picture

.

Alan D.’s picture

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
ankitgarg’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,902 pass(es). View

Rerolled.

lokapujya’s picture

Status: Needs review » Needs work
+++ b/core/misc/tabledrag.js
@@ -58,7 +58,7 @@ Drupal.tableDrag = function (table, tableSettings) {
-  this.maxDepth = 0; // Maximum amount of allowed parenting.
+  this.maxDepth = -1; // Maximum amount of allowed parenting. -1 if unlimited.

why is the change to the comment removed in the reroll?

adci_contributor’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch tabledrag-decouple-depth-1310642-21.patch. Unable to apply patch. See the log in the details link for more information. View
679 bytes

Update #19 in according to #20.
Please review.

Status: Needs review » Needs work

The last submitted patch, 21: tabledrag-decouple-depth-1310642-21.patch, failed testing.

nod_’s picture

Issue tags: +Needs reroll
googletorp’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,772 pass(es). View

Rerolled the patch from #21

nod_’s picture

Looks good to me. If someone could test that and it works as expected, feel free to RTBC I'm happy with the JS.

droplet’s picture

maartendeblock’s picture

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

No longer applies correctly, needs a reroll.

joshi.rohit100’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.56 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,895 pass(es). View

re-rolled.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

droplet’s picture

Status: Needs review » Needs work
Issue tags: +Needs JS testing, +JavaScriptTest

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.