Fix duplicate block rendering with getSortedChildren()

[justinbeaty]

Description (*)

If you try to add a block to a core/text_list, you'll get unexpected behavior if another block has been defined with the same name or alias. I consider this a bug because it's not that both blocks are rendered, but the second block is rendered twice.

For example, this renders "ÄÄ":

<block type="core/text_list" name="test">
    <block type="core/text" name="test_a">
        <action method="setText"><text>A</text></action>
    </block>
    <block type="core/text" name="test_a">
        <action method="setText"><text>Ä</text></action>
    </block>
</block>

While the first block will be overwritten by $this->_children[$alias] = $block;, a duplicate array element will be added to $this->_sortedChildren.

Then when we loop through the sorted children in Mage_Core_Block_Text_List, we will encounter the block name test_a twice, call getBlock('test_a') twice, and run toHtml() twice.

magento-lts/app/code/core/Mage/Core/Block/Text/List.php

Lines 27 to 38 in ca4f321

	protected function _toHtml()
	{
	$this->setText('');
	foreach ($this->getSortedChildren() as $name) {
	$block = $this->getLayout()->getBlock($name);
	if (!$block) {
	Mage::throwException(Mage::helper('core')->__('Invalid block: %s', $name));
	}
	$this->addText($block->toHtml());
	}
	return parent::_toHtml();
	}

You could just not add duplicate block names, but it's actually useful if you want to override a block in a more specific handle without having to add <action method="unsetChild">.

I also replaced unset() with array_splice() in the unsetChild method. This seems like a better solution than the solution merged from PR #1108 which I reverted.

Related Pull Requests

#1108

Manual testing scenarios (*)

Add the following into your local.xml:

<?xml version="1.0"?>
<layout version="0.1.0">
    <default>
        <reference name="content">

            <block type="core/text" name="test_1_title">
                <action method="setText"><text><![CDATA[<h5>Test 1</h5>]]></text></action>
            </block>
            <block type="core/text_list" name="test_1">
                <block type="core/text" name="test_1_a">
                    <action method="setText"><text>A</text></action>
                </block>
                <block type="core/text" name="test_1_a">
                    <action method="setText"><text>Ä</text></action>
                </block>
            </block>

            <block type="core/text" name="test_2_title">
                <action method="setText"><text><![CDATA[<h5>Test 2</h5>]]></text></action>
            </block>
            <block type="core/text_list" name="test_2">
                <block type="core/text" name="test_2_a">
                    <action method="setText"><text>A</text></action>
                </block>
                <block type="core/text" name="test_2_b">
                    <action method="setText"><text>B</text></action>
                </block>
                <block type="core/text" name="test_2_c">
                    <action method="setText"><text>C</text></action>
                </block>
                <action method="unsetChild">
                    <name>test_2_b</name>
                </action>
                <block type="core/text" name="test_2_b" before="test_2_c">
                    <action method="setText"><text>B</text></action>
                </block>
            </block>

            <block type="core/text" name="test_3_title">
                <action method="setText"><text><![CDATA[<h5>Test 3</h5>]]></text></action>
            </block>
            <block type="core/text_list" name="test_3">
                <block type="core/text" name="test_3_c" after="test_3_b">
                    <action method="setText"><text>C</text></action>
                </block>
                <block type="core/text" name="test_3_a" before="test_3_b">
                    <action method="setText"><text>A</text></action>
                </block>
                <block type="core/text" name="test_3_b" after="test_3_a">
                    <action method="setText"><text>B</text></action>
                </block>
                <action method="unsetChild">
                    <name>test_3_a</name>
                </action>
            </block>

            <block type="core/text" name="test_4_title">
                <action method="setText"><text><![CDATA[<h5>Test 4</h5>]]></text></action>
            </block>
            <block type="core/text_list" name="test_4">
                <block type="core/text" name="test_4_d" after="test_4_c">
                    <action method="setText"><text>D</text></action>
                </block>
                <block type="core/text" name="test_4_c" after="test_4_b">
                    <action method="setText"><text>C</text></action>
                </block>
                <block type="core/text" name="test_4_a" before="test_4_b">
                    <action method="setText"><text>A</text></action>
                </block>
                <block type="core/text" name="test_4_b" after="test_4_a">
                    <action method="setText"><text>B</text></action>
                </block>
                <action method="unsetChild">
                    <name>test_4_a</name>
                </action>
                <action method="unsetChild">
                    <name>test_4_b</name>
                </action>
            </block>

        </reference>
    </default>
</layout>

Results:

Questions or comments

I think I correctly replicated the issue in #1108, but if there's another test case please let me know. Ping @lemundo-team and @sreichel.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[sreichel]

Nice. :)

Did a few tests and looks good.

(Any ideas to cover it with tests?)

[justinbeaty]

Any ideas to cover it with tests?

I added the 3 test cases that failed in M1.9 / #1108.