? Success

User tests: Successful: Unsuccessful:

avatar bembelimen
bembelimen
8 Aug 2015

If you uninstall a component with categories, the categories will be deleted by the component. But the deletion just removes the entries without proper nested sets handling. So the rgt/lft values are wrong.

This patch fixes this issue.

How to test:

  1. Look into #__categories for the entry "root.1" and remember the "rgt" value
  2. Uninstall a component which uses categories
  3. Look into the table again => the rgt value did not change (although we have a lower number of categories)
  4. Apply patch
  5. Try again

=> the rgt value is now correct

avatar bembelimen bembelimen - open - 8 Aug 2015
avatar bembelimen bembelimen - change - 8 Aug 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 8 Aug 2015
Labels Added: ?
avatar mbabker
mbabker - comment - 8 Aug 2015

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.

avatar Bakual
Bakual - comment - 8 Aug 2015

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. :smile:
I checked both the models and the table. Do you know where it does the rebuild (serious question)?

avatar mbabker
mbabker - comment - 8 Aug 2015

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

avatar bembelimen
bembelimen - comment - 8 Aug 2015

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

avatar mbabker
mbabker - comment - 8 Aug 2015

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.

avatar Bakual
Bakual - comment - 8 Aug 2015

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.

avatar zero-24 zero-24 - change - 9 Aug 2015
Category Libraries
avatar zero-24 zero-24 - change - 9 Aug 2015
Easy No Yes
avatar zero-24 zero-24 - change - 20 Oct 2015
Status Pending Needs Review
Easy Yes No
avatar Hackwar
Hackwar - comment - 23 Feb 2016

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.

avatar chrisdavenport
chrisdavenport - comment - 7 May 2016

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.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7660.

avatar brianteeman brianteeman - change - 7 May 2016
Status Needs Review Pending
avatar wmchris wmchris - test_item - 1 Aug 2016 - Tested successfully
avatar wmchris
wmchris - comment - 1 Aug 2016

I have tested this item successfully on b0a9b31

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


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7660.

avatar mxkmp29
mxkmp29 - comment - 2 Aug 2016

tested it @icampus

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


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7660.

avatar mxkmp29 mxkmp29 - test_item - 2 Aug 2016 - Tested successfully
avatar mxkmp29
mxkmp29 - comment - 2 Aug 2016

I have tested this item successfully on b0a9b31

tested it @icampus

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


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7660.

avatar brianteeman brianteeman - change - 2 Aug 2016
Status Pending Ready to Commit
avatar brianteeman
brianteeman - comment - 2 Aug 2016

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7660.

avatar joomla-cms-bot joomla-cms-bot - change - 2 Aug 2016
Labels Added: ?
avatar rdeutz rdeutz - change - 13 Aug 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-08-13 16:58:27
Closed_By rdeutz
avatar rdeutz rdeutz - close - 13 Aug 2016
avatar rdeutz rdeutz - merge - 13 Aug 2016
avatar joomla-cms-bot joomla-cms-bot - close - 13 Aug 2016
avatar joomla-cms-bot joomla-cms-bot - change - 13 Aug 2016
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment