Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] add a context key to return always as collection for XmlEncoder#25369
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Simperfit commentedDec 7, 2017
Travis failure is unrelated. |
| * @param int|null $loadOptions A bit field of LIBXML_* constants | ||
| */ | ||
| publicfunction__construct(string$rootNodeName ='response',int$loadOptions =null) | ||
| publicfunction__construct(string$rootNodeName ='response',int$loadOptions =null/*, bool $alwaysCollection = false */) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
args can be appended to constructors, no need for the "BC layer"
| if (1 ===count($value) &&key($value)) { | ||
| $data[key($value)] =current($value); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
?
| } | ||
| /** | ||
| * @group collection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why adding this?
| $source = <<<'XML' | ||
| <?xml version="1.0"?> | ||
| <order_rows nodeType="order_row" virtualEntity="true"> | ||
| <order_row> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
indentation is strange, is it normal?
| ); | ||
| $this->assertEquals($expected,$this->encoder->decode($source,'xml')); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing blank line after
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
added
8dab446 to658fb0eCompareSimperfit commentedDec 9, 2017
AppVeyor failure is not related. |
Simperfit commentedDec 9, 2017
Status: Needs Review |
658fb0e toa582455Compare| * @param int|null $loadOptions A bit field of LIBXML_* constants | ||
| */ | ||
| publicfunction__construct(string$rootNodeName ='response',int$loadOptions =null) | ||
| publicfunction__construct(string$rootNodeName ='response',int$loadOptions =null,bool$alwaysCollection =false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$forceCollection?
a582455 to6f749bcComparedunglas commentedJan 11, 2018
@Simperfit can you rebase to force the CI to run again? |
6f749bc to7a3d918CompareSimperfit commentedJan 11, 2018
done@dunglas |
Simperfit commentedJan 26, 2018
This is ready to be merged. |
| foreach ($valueas$key =>$val) { | ||
| if (is_array($val) &&1 ===count($val)) { | ||
| if (is_array($val) &&1 ===count($val) && (!$this->forceCollection || !is_array($val[0]))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it will be easier to use if we don't create a new instance. Ill do the modification on both PRs
89a4381 tof1daa3fCompareSimperfit commentedFeb 2, 2018
@dunglas it now use a context key. |
| foreach ($valueas$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]))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should use snake_case also I suppose
| $value[$subnode->nodeName][] =$val; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let's keep this line :)
8f3a88d tob0daf42CompareSimperfit commentedFeb 5, 2018
done@nicolas-grekas |
b0daf42 to7c8709eCompare| * | ||
| * @param string $rootNodeName | ||
| * @param int|null $loadOptions A bit field of LIBXML_* constants | ||
| * @param int|null $loadOptions A bit field of LIBXML_* constants |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let's revert this: it's unrelated, and breaks merges
| foreach ($valueas$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]))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
usingempty() would make the code simpler I suppose
I don't understand the last condition!is_array($val[0]) - it feels unrelated. Is it?
SimperfitFeb 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
7c8709e to5319a86Compare5319a86 toadb428dCompare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedFeb 19, 2018
@nicolas-grekas done. |
dunglas commentedFeb 19, 2018
Thank you@Simperfit. |
…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
…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 <!-- seehttps://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/AAs 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
Uh oh!
There was an error while loading.Please reload this page.
This PR add a new
as_collectioncontext 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).