Skip to content

Fix/xml parser issue#7339

Merged
mmansoor-magento merged 3 commits intomagento:developfrom
dvynograd:fix/xml-parser-issue
Feb 25, 2017
Merged

Fix/xml parser issue#7339
mmansoor-magento merged 3 commits intomagento:developfrom
dvynograd:fix/xml-parser-issue

Conversation

@dvynograd
Copy link
Copy Markdown
Contributor

@dvynograd dvynograd commented Nov 7, 2016 •
edited by okorshenko
Loading

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
Copy Markdown
Contributor

thlassche commented Nov 9, 2016

Good work!

@dvynograd
Copy link
Copy Markdown
Contributor Author

dvynograd commented Nov 9, 2016

Thanks

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

maghamed commented Feb 21, 2017

@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
orlangur
orlangur reviewed Feb 24, 2017
View reviewed changes
lib/internal/Magento/Framework/Xml/Parser.php
if (
(is_string($content[$node->nodeName]) || !isset($content[$node->nodeName][0]))
|| (is_array($value) && !is_array($content[$node->nodeName][0]))
) {
Copy link
Copy Markdown
Contributor

@orlangur orlangur Feb 24, 2017

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
Copy Markdown
Contributor

okorshenko commented Feb 25, 2017

@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

Reviewers

@orlangur orlangur orlangur left review comments

+1 more reviewer

@maghamed maghamed maghamed approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@maghamed maghamed

@okorshenko okorshenko

Labels

Award: advanced bugfix Progress: accept

Projects

None yet

Milestone

February 2017

Development

Successfully merging this pull request may close these issues.

7 participants

@dvynograd @thlassche @maghamed @okorshenko @orlangur @vrann @mmansoor-magento