Skip to content

Add an XmlAttributeMap#164

Merged
schmittjoh merged 1 commit intoschmittjoh:masterfrom
adrienbrault:xml-attribute-map
Oct 11, 2012
Merged

Add an XmlAttributeMap#164
schmittjoh merged 1 commit intoschmittjoh:masterfrom
adrienbrault:xml-attribute-map

Conversation

@adrienbrault
Copy link
Copy Markdown
Contributor

Hi,

I needed this, so I share it.

I'm not sure if the results i'm expecting for the json and yaml formats are right.
In the tests i expect:

attributes:
    name: firstname
    value: Adrien

But should it be

name: firstname
value: Adrien

Any feedback is welcome.

@schmittjoh
Copy link
Copy Markdown
Owner

Did you check @XmlKeyValuePairs? After a quick look, it looks like what you are proposing.

@adrienbrault
Copy link
Copy Markdown
Contributor Author

Just checked it, it doesnt produce the expected XML.

I'm using this class:

<?php

/**
 * @Serializer\XmlRoot("input")
 */
class Input
{
    /**
     * @Serializer\XmlAttributeMap
     */
    public $attributes = array(
        'type' => 'integer',
        'value' => '1',
        'name' => 'search[page]',
    );
}

With @XmlKeyValuePairs I get

<input>
    <attributes>
        <type>
            <![CDATA[integer]]>
        </type>
        <value>
            <![CDATA[1]]>
        </value>
        <name>
            <![CDATA[search[page]]]>
        </name>
    </attributes>
</input>

Whereas with @XmlAttributeMap I get what i want :

<input type="integer" value="1" name="search[page]"/>

@adrienbrault
Copy link
Copy Markdown
Contributor Author

@schmittjoh ? Is there something missing in my implementation, tests ?

@schmittjoh
Copy link
Copy Markdown
Owner

Looks good, could you rebase this on latest master?

It should only require a minor update in the base test case.

@adrienbrault
Copy link
Copy Markdown
Contributor Author

Rebase on master done

Comment thread Serializer/XmlSerializationVisitor.php Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Did you add the Traversable check here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Huh ? !$v instanceof \Traversable

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Just asking because I've removed the special handling for traversables in the master branch. However, in that case we can probably keep it.

Could you add an extra test case to avoid regressions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm going to remove the Traversable handling, because YamlSerializationVisitor::75 uses array_keys. On a Traversable object it fails.

@adrienbrault
Copy link
Copy Markdown
Contributor Author

@schmittjoh I removed the Traversable handling, and added a test in XmlSerializationTest.php to check an exception is thrown

schmittjoh added a commit that referenced this pull request Oct 11, 2012
@schmittjoh schmittjoh merged commit a4bf528 into schmittjoh:master Oct 11, 2012
@schmittjoh
Copy link
Copy Markdown
Owner

Thanks, merged!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants