Bug in Magento\Framework\Simplexml\Element::appendChild

[tim-reynolds]

The bug is in the call to:
$source->children()

If an XML node has no child nodes and no attributes, this resolves to FALSE
However, if the XML node has no children and has one or more attributes it resolves to TRUE when clearly it should not.

See the line of code in question:
https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Simplexml/Element.php#L329

This line needs to be changed to

if(count($source->children())) {

Please see this Gist
https://gist.github.com/tim-reynolds/ac20289acdcfcde3633f

The bug exists in Magento 1.X as well as 2.0.

I would really blame PHP SimpleXMLElement but we can't readily fix that.

[anupdugar]

Thanks for submitting this pull request. We will process it internally through ticket MAGETWO-34069.
Also using \SimpleXMLElement::count is another option.
It counts the children of an element

if ($source->count()) {

ex:

$node = new \SimpleXMLElement('<sample testAttr="attrVal">test</sample>');
if ($node->count()) {
    echo 'has child';
} else {
    echo 'no child';
}

[tim-reynolds]

Thanks for the quick response Anup!

I did not yet create a pull request proper as my local version of Magento2 is outdated and I was concerned about setup for unit tests.

Did you need a proper pull request for this?

[anupdugar]

@tim-reynolds Pull request is highly encouraged. Here's our contribution guide:
https://github.com/magento/magento2/wiki/Contribution-Guide

Here's the Unit test in question : https://github.com/magento/magento2/blob/develop/dev/tests/unit/testsuite/Magento/Framework/Simplexml/ElementTest.php

For running this Unit test:

• Navigate to m2_project_root/dev/tests/unit
• ../../../vendor/phpunit/phpunit/phpunit testsuite/Magento/Framework/Simplexml/ElementTest.php

Feel free to check out our documentation regrading installation and updating your dependencies http://devdocs.magento.com/guides/v1.0/install-gde/install/prepare-install.html

[alankent]

One of the reasons for submitting a pull request is so your name is associated with the code as the contributor of the solution. So it's not mandatory to get it fixed (we can add to our bug backlog), it's just the best way to give you full credit for your contribution. You will have your name as a contributor in github for the world to see.

[tim-reynolds]

@alankent - Yeah, hopefully my name is already in. I think I've sent at least one maybe two bug patches into the project, but that was early when they weren't giving attribution on fixes.

Working on a PR now.

[anupdugar]

The associated Pull request has been merged, closing the issue. Thanks again for your contribution!