Skip to content

Allow to not skip empty not inline array root node #611

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

Merged
merged 6 commits into from
Jul 26, 2016

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented Jul 25, 2016

The current implementation skips the root node for not inline arrays when serializing into XML.

Given:

class User {
    /**
     * @Serializer\XmlList(inline = false, entry = "comment")
     * @Serializer\Type("array")
     */
    public $data = [];
}

The previous output was:

<result/>

The new behaviour is:

<result>
  <data/>
</result>

Should this be considered a BC break?
probably yes

This change is consistent with the existing JSON and YML implementations since the output is

{
  "data": []
}
data: []

@goetas
Copy link
Collaborator Author

goetas commented Jul 25, 2016

Should this be considered a BC break?

probably yes, will add an option to configure it

@goetas goetas changed the title Do not skip empty not inline array root node [WIP] Do not skip empty not inline array root node Jul 25, 2016
@schmittjoh
Copy link
Owner

It seems very marginal, I would be more inclined to just merge it. It only breaks if you literally compare string values, no?

@goetas
Copy link
Collaborator Author

goetas commented Jul 25, 2016

Please wait, will fix it tomorrow. I have some usecases

On 25 Jul 2016 6:41 pm, "Johannes" [email protected] wrote:

It seems very marginal, I would be more inclined to just merge it. It only
breaks if you literally compare string values, no?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#611 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAvaJ_X-QDpJWbvAhJIjAGwsWzkQQxErks5qZOcegaJpZM4JT9YV
.

@goetas
Copy link
Collaborator Author

goetas commented Jul 26, 2016

Added configuration option that allows to chose the default serialization strategy.

class User {
    /**
     * @Serializer\XmlList(inline = false, entry = "comment", skip_when_empty=false)
     * @Serializer\Type("array")
     */
    public $data = [];
}

this will add the new behaviour serializing it into:

<result>
  <data/>
</result>

@goetas goetas changed the title [WIP] Do not skip empty not inline array root node Allow to not skip empty not inline array root node Jul 26, 2016
@goetas
Copy link
Collaborator Author

goetas commented Jul 26, 2016

The last two commits are minor changes:
2d664fa is a minor changes that solves some edge cases when document contains namespaces.

3860411 simplifies the if/else conditions and checks for namespaces difined at node scope, not only at document scope

/**
* @var boolean
*/
public $skip_when_empty = true;
Copy link
Owner

Choose a reason for hiding this comment

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

Do we use this casing for other annotations? I think we already use camelCasing, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

true, will fix it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@schmittjoh schmittjoh merged commit 7b4b351 into schmittjoh:master Jul 26, 2016
@goetas
Copy link
Collaborator Author

goetas commented Jul 26, 2016

🎉

@goetas
Copy link
Collaborator Author

goetas commented Jul 26, 2016

thanks!

any plan for a 1.2.0 release?

(there are some PR pretty safe to merge that can be good to include in a hypothetical 1.2 release)

@goetas goetas deleted the nullable-array branch July 27, 2016 12:47
@goetas goetas added this to the v1.2 milestone Aug 3, 2016
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