Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/xml parser issue #7339

Merged
merged 3 commits into from
Feb 25, 2017
Merged

Conversation

dvynograd
Copy link
Contributor

@dvynograd dvynograd commented Nov 7, 2016

Test case:

<?xml version="1.0" encoding="utf-8"?>
<items>
    <item>10</item>
    <item>20</item>
    <item>30</item>
</items>

Output

Array
(
    [items] => Array
        (
            [item] => Array
                (
                    [0] => Array
                        (
                            [0] => 10
                            [1] => 20
                        )

                    [1] => 30
                )

        )

)

After fix

Array
(
    [items] => Array
        (
            [item] => Array
                (
                    [0] => 10
                    [1] => 20
                    [2] => 30
                )

        )

)

@thlassche
Copy link
Contributor

Good work!

@dvynograd
Copy link
Contributor Author

Thanks

@maghamed maghamed self-requested a review February 16, 2017 14:45
@maghamed maghamed self-assigned this Feb 16, 2017
@maghamed
Copy link
Contributor

@dvynograd well done! Thanks!

@maghamed maghamed added this to the February 2017 milestone Feb 22, 2017
@maghamed maghamed closed this Feb 22, 2017
@okorshenko okorshenko self-assigned this Feb 24, 2017
@okorshenko okorshenko reopened this Feb 24, 2017
if (
(is_string($content[$node->nodeName]) || !isset($content[$node->nodeName][0]))
|| (is_array($value) && !is_array($content[$node->nodeName][0]))
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be really nice to cover new introduced behavior with unit test.

Class implementation looked scary even before this change and every next fixed use case may break previous fixes if there is no enforcement with automated tests.

@mmansoor-magento mmansoor-magento merged commit 5bb5dd9 into magento:develop Feb 25, 2017
@okorshenko
Copy link
Contributor

@dvynograd thank you for your contribution to Magento 2 project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants