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

[Serializer] add a context key to return always as collection for XmlEncoder #25369

Merged
merged 1 commit into from Feb 19, 2018

Conversation

Projects
None yet
6 participants
@Simperfit
Contributor

Simperfit commented Dec 7, 2017

Q A
Branch? 4.1
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25227
License MIT
Doc PR

This PR add a new as_collection context key in order to return always as a collection instead of returning a single elements when there are only one array.

there are only one PR for the CsvEncoder don't wanted to have only One PR containing the two changes. It feel better to have two PR that fix the behaviour on two different things. it's easy to review and to revert if it break something (which should not since we are testing the behaviour).

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 7, 2017

Travis failure is unrelated.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 8, 2017

/**
* Construct new XmlEncoder and allow to change the root node element name.
*
* @param string $rootNodeName
* @param int|null $loadOptions A bit field of LIBXML_* constants
*/
public function __construct(string $rootNodeName = 'response', int $loadOptions = null)
public function __construct(string $rootNodeName = 'response', int $loadOptions = null/*, bool $alwaysCollection = false */)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 8, 2017

Member

args can be appended to constructors, no need for the "BC layer"

@@ -257,7 +262,6 @@ private function parseXml(\DOMNode $node, array $context = array())
if (1 === count($value) && key($value)) {
$data[key($value)] = current($value);

This comment has been minimized.

@nicolas-grekas
@@ -515,6 +515,39 @@ public function testDecodeIgnoreWhiteSpace()
$this->assertEquals($expected, $this->encoder->decode($source, 'xml'));
}
/**
* @group collection

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 8, 2017

Member

why adding this?

$source = <<<'XML'
<?xml version="1.0"?>
<order_rows nodeType="order_row" virtualEntity="true">
<order_row>

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 8, 2017

Member

indentation is strange, is it normal?

);
$this->assertEquals($expected, $this->encoder->decode($source, 'xml'));
}

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 8, 2017

Member

missing blank line after

This comment has been minimized.

@Simperfit

Simperfit Dec 11, 2017

Contributor

added

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 9, 2017

AppVeyor failure is not related.

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 9, 2017

Status: Needs Review

*/
public function __construct(string $rootNodeName = 'response', int $loadOptions = null)
public function __construct(string $rootNodeName = 'response', int $loadOptions = null, bool $alwaysCollection = false)

This comment has been minimized.

@fabpot

fabpot Jan 3, 2018

Member

$forceCollection?

@dunglas

This comment has been minimized.

Member

dunglas commented Jan 11, 2018

@Simperfit can you rebase to force the CI to run again?

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Jan 11, 2018

done @dunglas

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Jan 26, 2018

This is ready to be merged.

@@ -335,7 +336,7 @@ private function parseXmlValue(\DOMNode $node, array $context = array())
}
foreach ($value as $key => $val) {
if (is_array($val) && 1 === count($val)) {
if (is_array($val) && 1 === count($val) && (!$this->forceCollection || !is_array($val[0]))) {

This comment has been minimized.

@dunglas

dunglas Jan 26, 2018

Member

After a second thought, wouldn't it be better to allow to change this behavior through the context instead of forcing to register a new instance of the class?

This comment has been minimized.

@Simperfit

Simperfit Jan 26, 2018

Contributor

I think it will be easier to use if we don't create a new instance. Ill do the modification on both PRs

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Feb 2, 2018

@dunglas it now use a context key.

foreach ($value as $key => $val) {
if (is_array($val) && 1 === count($val)) {
if (is_array($val) && 1 === count($val) && (!(isset($context['forceCollection']) && $context['forceCollection']) || !is_array($val[0]))) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

should use snake_case also I suppose

@@ -333,9 +332,8 @@ private function parseXmlValue(\DOMNode $node, array $context = array())
$value[$subnode->nodeName][] = $val;
}
}

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

let's keep this line :)

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Feb 5, 2018

@@ -41,8 +41,7 @@ class XmlEncoder implements EncoderInterface, DecoderInterface, NormalizationAwa
/**
* Construct new XmlEncoder and allow to change the root node element name.
*
* @param string $rootNodeName
* @param int|null $loadOptions A bit field of LIBXML_* constants
* @param int|null $loadOptions A bit field of LIBXML_* constants

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 11, 2018

Member

let's revert this: it's unrelated, and breaks merges

@@ -335,7 +334,7 @@ private function parseXmlValue(\DOMNode $node, array $context = array())
}
foreach ($value as $key => $val) {
if (is_array($val) && 1 === count($val)) {
if (is_array($val) && 1 === count($val) && (!(isset($context['as_collection']) && $context['as_collection']) || !is_array($val[0]))) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 11, 2018

Member

using empty() would make the code simpler I suppose
I don't understand the last condition !is_array($val[0]) - it feels unrelated. Is it?

This comment has been minimized.

@Simperfit

Simperfit Feb 16, 2018

Contributor

I just looked after it and !is_array($val[0]) makes it possible to return the value directly instead of having two arrays:

$expected = array(
            '@nodeType' => 'order_row',
            '@virtualEntity' => 'true',
            'order_row' => array(array(
                'id' => 16,
                'test' => 16,
            )),
        );

vs

$expected = array(
            '@nodeType' => 'order_row',
            '@virtualEntity' => 'true',
            'order_row' => array(array(
                'id' => array(16),
                'test' => array(16),
            )),
        );

Do you mean replacing

is_array($val) && 1 === count($val)

to

!empty($val)

it's not possible

@nicolas-grekas

Please update the main description so that people looking at this PR can understand what's going on here.
Please also check if a doc PR should be done.

@Simperfit Simperfit changed the title from [Serializer] add a contruct key to return always as collection for XmlEncoder to [Serializer] add a context key to return always as collection for XmlEncoder Feb 19, 2018

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Feb 19, 2018

@dunglas

This comment has been minimized.

Member

dunglas commented Feb 19, 2018

Thank you @Simperfit.

@dunglas dunglas merged commit adb428d into symfony:master Feb 19, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

dunglas added a commit that referenced this pull request Feb 19, 2018

feature #25369 [Serializer] add a context key to return always as col…
…lection for XmlEncoder (Simperfit)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Serializer] add a context key to return always as collection for XmlEncoder

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #25227
| License       | MIT
| Doc PR        |

This PR add a new `as_collection` context key in order to return always as a collection instead of returning a single elements when there are only one array.

there are only one PR for the CsvEncoder don't wanted to have only One PR containing the two changes. It feel better to have two PR that fix the behaviour on two different things. it's easy to review and to revert if it break something (which should not since we are testing the behaviour).

Commits
-------

adb428d [Serializer] add a context key to return always as collection for XmlEncoder

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

fabpot added a commit that referenced this pull request Jun 30, 2018

feature #27715 [Serializer] Deprecate CsvEncoder as_collection false …
…default value (ogizanagi)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Serializer] Deprecate CsvEncoder as_collection false default value

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

As already expressed in #25369 and related issues, this behavior is quite counter-intuitive. It may be fine for write-API with a single document in the body but I think such CSV APIs are way less common than file-based ones, expecting collections. So I think this behavior should be opt-in explicitly in required cases, always dealing with collections by default.
This is still an arbitrary decision, but trying to make it based on use-cases and user's experience with CSV.

Note: perhaps we could find a better name for this as the semantic of setting `as_collection` to `false` to get the single row directly would not be really obvious.
Also, it could throw an exception when getting multiple rows where only one was expected.

Commits
-------

bce59c8 [Serializer] Deprecate CsvEncoder as_collection false default value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment