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 constructor arguement to return csv always as collection #25218

Merged
merged 1 commit into from Feb 19, 2018

Conversation

@Simperfit
Contributor

Simperfit commented Nov 30, 2017

Q A
Branch? 4.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #21616
License MIT
Doc PR TODO create a doc PR for the 3 ways of getting csv collection, or a single

Coding in the train again ;).
img_9980

This is to be able to add a new behaviour to the csv encoder when passing the alwaysAsCollection context key, this will return a collection even if there is only one element.

@@ -153,6 +153,10 @@ public function decode($data, $format, array $context = array())
}
fclose($handle);
if (isset($context['alwaysAsCollection']) && true === $context['alwaysAsCollection']) {

This comment has been minimized.

@dunglas

dunglas Nov 30, 2017

Member

The key should be in snake case, as for all other context’s keys.
By the way, can't it just be collection?

@dunglas

This comment has been minimized.

Member

dunglas commented Nov 30, 2017

Should be merged in 4.1 as it's a new feature.

@Simperfit Simperfit changed the base branch from 3.3 to master Nov 30, 2017

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 30, 2017

Status: Needs Review

@@ -153,6 +153,10 @@ public function decode($data, $format, array $context = array())
}
fclose($handle);
if (isset($context['collection']) && true === $context['collection']) {

This comment has been minimized.

@dunglas

dunglas Nov 30, 2017

Member

Can be if ($context['collection'] ?? false) now that you target master.

This comment has been minimized.

@TomasVotruba

TomasVotruba Nov 30, 2017

Contributor

This should be covered by Coding Standard. Is that missed?

This comment has been minimized.

@Simperfit

Simperfit Dec 1, 2017

Contributor

This is not covered by CS atm I think, since fabbot is not red.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Nov 30, 2017

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Nov 30, 2017

Cant we just always return a collection? is there any real reason for mixed return values?

Just curious :) otherwise collection might not be the best name, as collection=false still returns a collection, but only if it's >1.

edit: yeah sorry reading ticket now :(

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 30, 2017

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Nov 30, 2017

It really makes sense for being CSV no? I dont understand why this logic must be here for "API" purpose. CSV is a collection of row(s) no?

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 1, 2017

Status: Needs Review

@@ -26,6 +26,7 @@ class CsvEncoder implements EncoderInterface, DecoderInterface
const ENCLOSURE_KEY = 'csv_enclosure';
const ESCAPE_CHAR_KEY = 'csv_escape_char';
const KEY_SEPARATOR_KEY = 'csv_key_separator';
const ALWAYS_COLLECTION = 'always_collection';

This comment has been minimized.

@ro0NL

ro0NL Dec 2, 2017

Contributor

does convention require us to prefix the value with csv_?

This comment has been minimized.

@dunglas

dunglas Dec 2, 2017

Member

Good catch @ro0NL, I'm 👍 to use the prefix.

This comment has been minimized.

@ro0NL

ro0NL Dec 2, 2017

Contributor

Do we wanna toggle the default eventually? Side node; is #25227 related to this same convention in XML? If so should EncoderInterface know about the const in general to expose this logic to other encoders?

This comment has been minimized.

@Aerendir

Aerendir Dec 4, 2017

Hi at all of you, thank you @ro0NL to link to my issue #25227.

After some digging in the code, I found that the problem is intentional: there is an explicit check on the number of elements in the collection and if it is only one, the collection is removed.

See my ticket #25227 for more details.

I don't know why the behavior is this, anyway it is not correct in the scenario that I reported. Read the ticket and understood why the behavior is this.

So, having an option to make serializer skip the removing of the collection is really helpful if not required: as is, currently I cannot use the Serializer to consume the API (that, just to say, is of PrestaShop, so this problem is related to all people who use Serializer to consume such API.).

The strange thing is that this behavior is not present in the Json Encoder, as I'm consuming also an Json API and never had this same problem.

I've not checked the code of the JsonEncoder, and maybe I will do, but I think that somewhere should be stated clearly which has to be the behavior of ALL encoders when they manage collections.

Becuase currently it is really inconsistent and someone who uses the Serializer without having the patience of going deeper in the code will simply think that it doesn't work (as, in reality, is: it currently doesn't work).

So, in the end, I think that the ALWAYS_COLLECTION constant should be a general constant of which all encoders have to be aware and act on.

This comment has been minimized.

@Simperfit

Simperfit Dec 4, 2017

Contributor

Maybe we should add this on every encoders to be sure that we consistency using the context key

This comment has been minimized.

@Simperfit

Simperfit Dec 4, 2017

Contributor

BTW I think this one should be focusing on CSV and then ill made a follow PR for all of the others encore, @dunglas @ro0NL @Aerendir WDYT ?

This comment has been minimized.

@Aerendir

Aerendir Dec 4, 2017

I think that for sure this is required on the XML encoder. I don't know if it is required also on the others (JsonDecode and YamlEncoder).

As this is going to be a global requirement, something that someone MUST take into account when creating a new encoder, I think that this should be fixed in the same PR as we are going to introduce a global constant that will be used by more than only the CSV encoder.

This comment has been minimized.

@Aerendir

Aerendir Dec 4, 2017

Also the Fixed tickets property of the opening comment should include also a reference to the issue I opened about the XML encoder... I think...

This comment has been minimized.

@Simperfit

Simperfit Dec 5, 2017

Contributor

I will take care of the needed modification

This comment has been minimized.

@dunglas

dunglas Dec 6, 2017

Member

This is a specific problem to the CSV format. JSON and YAML aren't affected because they have both the notion of array and of object, and don't depend of headers. Regarding the XML encoder, it's not because of the format, it's basically an implementation problem (that will be hard to fix because of BC, but from a broader point of view, the XML support in the Serializer is poor and needs more love).

IMO we should keep this flag CSV only for now (we can add a new one for XML if it's a quick fix), and after a second though, maybe should be introduce a constructor flag instead of a context option (this behavior must be changed globally IMO).

@Aerendir

This comment has been minimized.

Aerendir commented Dec 6, 2017

ping

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 6, 2017

@Aerendir Before doing it I'm waiting for @dunglas POV about that.

@Simperfit Simperfit changed the title from [Serializer] add a context key to return csv always as collection to [Serializer] add a constructor arguement to return csv always as collection Dec 7, 2017

@Aerendir

This comment has been minimized.

Aerendir commented Dec 12, 2017

Hi at all of you folks (let's be inclusive :) ), any news about this?

I need the collections be deserialized correctly ASAP.

I'd like to send a PR for the XMLEncoder, but which path should have I to follow? Will we have an interface? Or I simply adapt the modifications of this PR adapting them for the XMLEncoder?

ping @Simperfit, @dunglas

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 12, 2017

@Aerendir

This comment has been minimized.

Aerendir commented Dec 12, 2017

@Simperfit , any idea about when it will be ready? 👼

@Aerendir

This comment has been minimized.

Aerendir commented Dec 21, 2017

@Simperfit , I've update my issue adding the result of a simple test I did.

Practically, commenting the code that transforms nodes with only element, all the generated array is filled with keys 0 as they are not removed anymore.

This breaks the current implementations and also is not a desirable thing.

Did you checked this behavior on this PR?

Unfortunately I have no data to test this PR, so I cannot tell you an opinion about, but I think you should give my comment a look and check the behavior is not the same with your behavior.

Read here the comment: #25227 (comment)

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 21, 2017

@ostrolucky

ostrolucky suggested changes Jan 6, 2018 edited

Same suggestion as I mentioned here #24508 (comment)

Can we stop adding so many configuration parameters into constructor? It's not scalable and brittle. Can't manipulate it later once it's there due to BC promise.

So again, I suggest to just add a normalizer.

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Jan 10, 2018

This is fixing a misbehaviour, should we really add a new normalizer to fix this ?
cc @dunglas

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Feb 2, 2018

PR Changed to remove the param in the constructor and add it as a context key.

@@ -150,6 +150,10 @@ public function decode($data, $format, array $context = array())
}
fclose($handle);
if (isset($context['forceCollection']) && $context['forceCollection']) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

AFAIK, we use snake case for all other keys, isn't it?

This comment has been minimized.

@Simperfit

Simperfit Feb 5, 2018

Contributor

done

@@ -150,6 +150,10 @@ public function decode($data, $format, array $context = array())
}
fclose($handle);
if (isset($context['as_collection']) && $context['as_collection']) {

This comment has been minimized.

@jvasseur

jvasseur Feb 5, 2018

Contributor

Maybe we could use a 3 value switch here (null/true/false) with null (or not set) the current behavior (guess), true forcing a collection and false forcing an unique element.

This comment has been minimized.

@Simperfit

Simperfit Feb 16, 2018

Contributor

I think it's ok to say true should return the collection and false it the old behaviour.

This comment has been minimized.

@dunglas

dunglas Feb 19, 2018

Member

$context['as_collection'] ?? false?

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Feb 16, 2018

ping @dunglas @nicolas-grekas this is ready.

Travis failure is not related to this PR.

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Feb 16, 2018

Status: Needs Review

@dunglas

This comment has been minimized.

Member

dunglas commented Feb 19, 2018

Thanks @Simperfit.

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

3 checks passed

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

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

feature #25218 [Serializer] add a constructor arguement to return csv…
… always as collection (Simperfit)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Serializer] add a constructor arguement to return csv always as collection

| Q             | A
| ------------- | ---
| Branch?       |  4.1
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #21616
| License       | MIT
| Doc PR        | TODO create a doc PR for the 3 ways of getting csv collection, or a single

Coding in the train again ;).
![img_9980](https://user-images.githubusercontent.com/3451634/33417042-f13063e4-d59f-11e7-8f30-143da768b1d7.JPG)

This is to be able to add a new behaviour to the csv encoder when passing the alwaysAsCollection context key, this will return a collection even if there is only one element.

Commits
-------

d19d05d [Serializer] add a context key to return csv always as collection
@wadjeroudi

This comment has been minimized.

wadjeroudi commented May 3, 2018

@dunglas when will it be released ? it's only in master.

@dunglas

This comment has been minimized.

Member

dunglas commented May 3, 2018

@wadjeroudi

This comment has been minimized.

wadjeroudi commented May 4, 2018

@dunglas There's no BC, it won't be in 3.4.x releases ? Thx.

@dunglas

This comment has been minimized.

Member

dunglas commented May 4, 2018

No, because it’s a new feature (we don’t allow non backward compatible code, even in 4.2).

@Simperfit Simperfit deleted the Simperfit:bugfix/add-a-test-for-context-key-to-disable-data-inconsistency branch May 4, 2018

@xabbuh

This comment has been minimized.

Member

xabbuh commented May 4, 2018

@wadjeroudi New features are always only merged into the master branch and will be available with the next minor version (which right now will be Symfony 4.1). Symfony 3.4 will only receive bug fixes during its maintenance period but no new features.

@wadjeroudi

This comment has been minimized.

wadjeroudi commented May 4, 2018

I come after the "war", but I think it's not a new feature, it's a consistency fix IMHO.
From the first place, the csvencoder should have always considered the result as a collection.
Anyway thank you ^ ^.

// If there is only one data line in the document, return it (the line), the result is not considered as a collection

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

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