In theory it should work, in reality we should probably be using JTableCategory to do the deletions since that will handle updating the tree indexes (lft/rgt) too.

in reality we should probably be using JTableCategory to do the deletions since that will handle updating the tree indexes (lft/rgt) too.

In theory, it should do that. In practice I couldn't find where it does. 😄
I checked both the models and the table. Do you know where it does the rebuild (serious question)?

JTableNested::delete() deals with it (JTableCategory extends this class and doesn't override the method).

I'm not sure, if server survive a delete-call when there are thausands of categories, which will be rebuild after every deletion. Here we make the rebuild only once...

IMO we shouldn't be using rebuild as a general function but rather use the API the "right" way. It would mean a few more queries (we should be able to do a query to get a list of all the parent categories for the extension and delete those one by one which by default deletes its children too), but it would be less prone to error. #7551 demonstrates one case where altering stuff in a nested table then running rebuild can cause issues.

JTableNested::delete() deals with it ( JTableCategory extends this class and doesn't override the method).

Blind me. I looked at JTableNested::delete() but was looking for a call to rebuild(). Didn't expect it doing the SQL directly.

In theory I agree with @mbabker that we should use the API to delete the entries. In praxis this would kill every server. Even when deleting just a few dozen categories, I had very good servers crap out. Our nested sets implementation for editing is absolutely horrible in terms of performance. The fact that we need rebuild() at all is a problem in itself. As long as we don't fix our nested sets implementation, this is the only viable solution. (And even then I fear it is not going to be enough)

From my perspective, this is good to go. Since this is one of these "can't be tested"-changes, I could give this a placebo-"tested-successfully", but otherwise we can simply merge this in.

Michael is of course correct, but I don't see any reason not to merge this PR in the meantime. Improving JTableNested is another (important) issue which should be looked at separately.

I have tested this item ✅ successfully on b0a9b31

Works as described in test scenario. rgt value changed successfully. @icampus

tested it @icampus

followed the description. rgt value changed sucessfully after the patch.

I have tested this item ✅ successfully on b0a9b31

tested it @icampus

followed the description. rgt value changed sucessfully after the patch.

RTC