Joomla! Issue Tracker | Joomla! CMS #18404 - Removing menu children on component install can destroy the whole menu tree

Fixed in Code Base
1 Nov 2017
Medium
Build: staging
# 18404
Diff
klas:patch-2

Pending

User tests: Successful: Unsuccessful:

klas
24 Oct 2017

Menu table inherits Nested and since delete without second argument set to false, removes node and its children. As all child ids that need to be removed are already collected in previous step this means:
a) errors are generated if tree is consistent - all children are already removed in first delete, so all following deletes fail
b) with inconsistent trees this can delete random ranges of menus as delete only looks at lft-rgt values. This is not a unusual scenario as admin menus were previously not checked for consistency - I just had the whole menu wiped out due to faulty record.

To prevent this I propose that we delete only explicit records as I assume was also originally intended due to ids collection in previous step.

Summary of Changes

Set Children parameter to false in delete

Testing Instructions

Copy existing record from menu tree from one of installed 3rd party components (needs to be installable, not core) to create a faulty record: : copy existing record, change parent to non-existing id and set lft to 0 and rgt to 99999. It needs to be an admin menu (client = 1 ).

Update(reinstall) component with faulty record.

Expected result

Record disapears, the rest of the records remain.

Actual result

All records are erased.

Documentation Changes Required

joomla-cms-bot - change - 24 Oct 2017

Category

⇒

Libraries

904d02b 24 Oct 2017

Removing children can destroy the whole menu tree

klas - open - 24 Oct 2017

klas - change - 24 Oct 2017

Status

New

⇒

Pending

klas - change - 24 Oct 2017

Title

…

Removing ~~children~~ can destroy the whole menu tree

Removing menu children on component install can destroy the whole menu tree

klas - edited - 24 Oct 2017

klas - change - 24 Oct 2017

The description was changed

klas - edited - 24 Oct 2017

csthomas - comment - 24 Oct 2017

It helps because it do not need to delete children, which are already checked.

But it still leaves a mess after call this:

joomla-cms/libraries/src/Table/Nested.php

Lines 625 to 659 in e0e8c95

    
           			// Delete the node. 
        
           			$query->clear() 
        
           				->delete($this->_tbl) 
        
           				->where('lft = ' . (int) $node->lft); 
        
           			$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED'); 
        
           			// Shift all node's children up a level. 
        
           			$query->clear() 
        
           				->update($this->_tbl) 
        
           				->set('lft = lft - 1') 
        
           				->set('rgt = rgt - 1') 
        
           				->set('level = level - 1') 
        
           				->where('lft BETWEEN ' . (int) $node->lft . ' AND ' . (int) $node->rgt); 
        
           			$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED'); 
        
           			// Adjust all the parent values for direct children of the deleted node. 
        
           			$query->clear() 
        
           				->update($this->_tbl) 
        
           				->set('parent_id = ' . (int) $node->parent_id) 
        
           				->where('parent_id = ' . (int) $node->$k); 
        
           			$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED'); 
        
           			// Shift all of the left values that are right of the node. 
        
           			$query->clear() 
        
           				->update($this->_tbl) 
        
           				->set('lft = lft - 2') 
        
           				->where('lft > ' . (int) $node->rgt); 
        
           			$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED'); 
        
           			// Shift all of the right values that are right of the node. 
        
           			$query->clear() 
        
           				->update($this->_tbl) 
        
           				->set('rgt = rgt - 2') 
        
           				->where('rgt > ' . (int) $node->rgt); 
        
           			$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED');

klas - change - 24 Oct 2017

The description was changed

klas - edited - 24 Oct 2017

klas - change - 24 Oct 2017

The description was changed

klas - edited - 24 Oct 2017

csthomas - test_item - 27 Oct 2017 - Tested successfully

csthomas - comment - 27 Oct 2017

I have tested this item ✅ successfully on 904d02b

Code review.

Besides my doubts (above) but there is no better solution now. I mark it as success.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18404.}

alikon - comment - 27 Oct 2017

@csthomas open a new issue for that

alikon - test_item - 27 Oct 2017 - Tested successfully

alikon - comment - 27 Oct 2017

I have tested this item ✅ successfully on 904d02b

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18404.}

franz-wohlkoenig - change - 27 Oct 2017

Status

Pending

⇒

Ready to Commit

franz-wohlkoenig - comment - 27 Oct 2017

RTC after two successful tests.

izharaazmi - comment - 30 Oct 2017

@alikon @csthomas What happens to the children after we delete a parent node, with this PR?

csthomas - comment - 30 Oct 2017

It depends how much is your database table corrupted.

If there is no errors then there should not be any children because all menu items of the component are deleted, one by one.
Otherwise, for example, if the values of column lft and rgt are corrupted for items X and Y that are contained (based on lft and rgt values) in item Z and you want to delete Z, then X and Y will be move one level up, lft and rgt will be updated, [X,Y].parent_id = Z.parent_id. This is the good side. The bad side is, when you delete corrupted item, you could move others menu items to one level up and other menu items may start be corrupted too.

If you want to read the code then take a look at code that I pasted above, joomla-cms/libraries/src/Table/Nested.php line 625.

Such operation, delete, runs a few sql queries in non atomic operation, executed separately and based on lft column, which is not unique in database tables.

mbabker - close - 1 Nov 2017

mbabker - merge - 1 Nov 2017

mbabker - change - 1 Nov 2017

Status	Ready to Commit	⇒	Fixed in Code Base
Closed_Date	0000-00-00 00:00:00	⇒	2017-11-01 12:04:27
Closed_By		⇒	mbabker
Labels	Added: ?

Add a Comment

Older
Newer

Joomla! Issue Tracker - CMS

[#18404] - Removing menu children on component install can destroy the whole menu tree

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Add a Comment

	// Delete the node.
	$query->clear()
	->delete($this->_tbl)
	->where('lft = ' . (int) $node->lft);
	$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED');

	// Shift all node's children up a level.
	$query->clear()
	->update($this->_tbl)
	->set('lft = lft - 1')
	->set('rgt = rgt - 1')
	->set('level = level - 1')
	->where('lft BETWEEN ' . (int) $node->lft . ' AND ' . (int) $node->rgt);
	$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED');

	// Adjust all the parent values for direct children of the deleted node.
	$query->clear()
	->update($this->_tbl)
	->set('parent_id = ' . (int) $node->parent_id)
	->where('parent_id = ' . (int) $node->$k);
	$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED');

	// Shift all of the left values that are right of the node.
	$query->clear()
	->update($this->_tbl)
	->set('lft = lft - 2')
	->where('lft > ' . (int) $node->rgt);
	$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED');

	// Shift all of the right values that are right of the node.
	$query->clear()
	->update($this->_tbl)
	->set('rgt = rgt - 2')
	->where('rgt > ' . (int) $node->rgt);
	$this->_runQuery($query, 'JLIB_DATABASE_ERROR_DELETE_FAILED');