Fix regression bug for duplicate block rendering with getSortedChildren()

[bucha]

Description (*)

This is a fix for regression bug I noticed while updating one of our projects from OpenMage 20.10.2 to 20.11.0 which I was able to trace back to #4337

Within this project we've basically implemented a holepunch-system in which we run the following code to replace a given block by another block:

$layout->unsetBlock($targetBlockName);
$layout->setBlock($targetBlockName, $replacementBlock);
$parent->setChild($targetBlockAlias, $replacementBlock);

Running OpenMage 20.10.2 this approach worked perfectly.
On OpenMage 20.11.0 however we experienced a problem:
One of the blocks we replace using this method is inside a list-block. A list-block will first sort its children and then render them in order to append their html output to its own output. We noticed that the output of the replacement-block was not in the list-block's output any longer. When looking at this phenomenon in an xdebug session I noticed that the list-block contained the replacement-block in its $_children but $_sortedChildren was empty.

It turns out that the recently introduced call to $this->unsetChild($alias); in Mage_Core_Block_Abstract was responsible for this behavior:

https://github.com/OpenMage/magento-lts/blame/v20.11.0/app/code/core/Mage/Core/Block/Abstract.php#L468

I have therefor removed the call to $this->unsetChild($alias); and instead introduced a check in insert() whether a given block is already known in $_sortedChildren and if so it will be removed from $_sortedChildren (before then being appended again).

This should be in line with @justinbeaty 's original intentions which I also checked against the unit-tests of #4337.

Related Pull Requests

• Fix duplicate block rendering with getSortedChildren() #4337
• Ensure correctly sorted block children after unset some of them #1108

Manual testing scenarios (*)

I've added another test within the unit tests:

	public function testSortInstructionsAfterReplaceChild()
	{
		$layout = Mage::getModel('core/layout');

		$parentBlock = $layout->createBlock('core/text_list', 'parent');

		$childBlockA = $layout->createBlock('core/text', 'target_block')->setText('A');
		$parentBlock->insert($childBlockA, '', false, 'child');

		$childBlockB = $layout->createBlock('core/text', 'target_block')->setText('B');

		// Replacing the block but keeping its order within the parent
		$layout->unsetBlock('target_block');
		$layout->setBlock('target_block', $childBlockB);
		$parentBlock->setChild('child', $childBlockB);

		$this->assertSame('B', $parentBlock->toHtml());
	}

[sreichel]

Thanks for that detailed description. 👍

Checks did not run, it is your first PR?

[bucha]

Yes it's my first PR

[sreichel]

Thank you.

Please run composer run php-cs-fixer:fix to fix the issues.

[justinbeaty]

@bucha Thank you, looking at the code it looks fine since your new check is right before the other sorted children logic. And if the unit tests are passing then it seems all okay (thanks also for fixing that one test.)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sreichel]

@all-contributors add bucha code

[allcontributors]

@sreichel

I've put up a pull request to add @bucha! 🎉