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

Simperfit · 2017-11-30T06:27:55Z

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 ;).

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.

dunglas · 2017-11-30T07:23:26Z

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

@@ -153,6 +153,10 @@ public function decode($data, $format, array $context = array())
        }
        fclose($handle);

+        if (isset($context['alwaysAsCollection']) && true === $context['alwaysAsCollection']) {


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

dunglas · 2017-11-30T07:53:42Z

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

Simperfit · 2017-11-30T09:06:10Z

Status: Needs Review

dunglas · 2017-11-30T09:14:13Z

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

@@ -153,6 +153,10 @@ public function decode($data, $format, array $context = array())
        }
        fclose($handle);

+        if (isset($context['collection']) && true === $context['collection']) {


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

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

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

ro0NL · 2017-11-30T19:23:22Z

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 · 2017-11-30T19:36:30Z

If we want to return only collection we need to do it in 5.0 since it’s a BC. maybe we should name it always_collection ? @dunglas Le jeu. 30 nov. 2017 à 20:23, Roland Franssen <notifications@github.com> a écrit :

…

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. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#25218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSq8j8RxM7BsfygOCOuXJGwjRxiuWJZks5s7wC-gaJpZM4QwFUy> .

ro0NL · 2017-11-30T19:38:40Z

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 · 2017-12-01T16:14:46Z

Status: Needs Review

ro0NL · 2017-12-02T16:16:35Z

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

@@ -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';


does convention require us to prefix the value with csv_?

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

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?

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.

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

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 ?

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.

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...

I will take care of the needed modification

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 · 2017-12-06T12:29:12Z

ping

Simperfit · 2017-12-06T13:28:09Z

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

Simperfit · 2018-01-10T10:51:45Z

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

Simperfit · 2018-02-02T13:20:49Z

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

nicolas-grekas · 2018-02-04T15:47:28Z

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

@@ -150,6 +150,10 @@ public function decode($data, $format, array $context = array())
        }
        fclose($handle);

+        if (isset($context['forceCollection']) && $context['forceCollection']) {


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

jvasseur · 2018-02-05T14:21:49Z

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

@@ -150,6 +150,10 @@ public function decode($data, $format, array $context = array())
        }
        fclose($handle);

+        if (isset($context['as_collection']) && $context['as_collection']) {


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.

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

$context['as_collection'] ?? false?

Simperfit · 2018-02-16T08:43:31Z

ping @dunglas @nicolas-grekas this is ready.

Travis failure is not related to this PR.

Simperfit · 2018-02-16T09:58:01Z

Status: Needs Review

dunglas · 2018-02-19T14:05:25Z

Thanks @Simperfit.

… 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  | 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](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 · 2018-05-03T15:37:19Z

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

dunglas · 2018-05-03T22:26:31Z

@wadjeroudi in May: https://symfony.com/roadmap?version=4.1

wadjeroudi · 2018-05-04T04:55:49Z

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

dunglas · 2018-05-04T05:02:06Z

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

xabbuh · 2018-05-04T07:19:59Z

@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 · 2018-05-04T09:30:57Z

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

carsonbot added Status: Needs Review Serializer Bug labels Nov 30, 2017

Simperfit mentioned this pull request Nov 30, 2017

[Serializer] CSV Encoder returns inconsistent result #21616

Closed

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 30, 2017

dunglas requested changes Nov 30, 2017

View reviewed changes

Simperfit changed the base branch from 3.3 to master November 30, 2017 09:04

Simperfit force-pushed the bugfix/add-a-test-for-context-key-to-disable-data-inconsistency branch from cbe28c0 to 7ebe8e0 Compare November 30, 2017 09:05

carsonbot added Status: Needs Review and removed Status: Needs Work labels Nov 30, 2017

dunglas requested changes Nov 30, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 30, 2017

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

Simperfit force-pushed the bugfix/add-a-test-for-context-key-to-disable-data-inconsistency branch from 7ebe8e0 to 1f0f748 Compare December 1, 2017 16:14

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 1, 2017

Simperfit force-pushed the bugfix/add-a-test-for-context-key-to-disable-data-inconsistency branch from 1f0f748 to 855993b Compare December 1, 2017 17:25

ro0NL reviewed Dec 2, 2017

View reviewed changes

Aerendir mentioned this pull request Dec 4, 2017

[Serializer] XML Collections not correctly hydrated #25227

Closed

Simperfit force-pushed the bugfix/add-a-test-for-context-key-to-disable-data-inconsistency branch from 855993b to 0b5588a Compare December 4, 2017 17:11

Simperfit force-pushed the bugfix/add-a-test-for-context-key-to-disable-data-inconsistency branch from b19ce4e to b22d1a0 Compare February 2, 2018 13:20

nicolas-grekas reviewed Feb 4, 2018

View reviewed changes

Simperfit force-pushed the bugfix/add-a-test-for-context-key-to-disable-data-inconsistency branch 2 times, most recently from 23e0965 to b0bdaab Compare February 5, 2018 09:11

jvasseur reviewed Feb 5, 2018

View reviewed changes

carsonbot added Status: Needs Review and removed Status: Needs Work labels Feb 16, 2018

dunglas approved these changes Feb 19, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 19, 2018

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

d19d05d

Simperfit force-pushed the bugfix/add-a-test-for-context-key-to-disable-data-inconsistency branch from b0bdaab to d19d05d Compare February 19, 2018 13:54

dunglas approved these changes Feb 19, 2018

View reviewed changes

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

javiereguiluz mentioned this pull request Mar 2, 2018

[Serializer] add a constructor argument to return CSV always as collection symfony/symfony-docs#9383

Closed

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

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

teohhanhui mentioned this pull request Nov 5, 2019

Test and fix API Platform with Symfony 4.4 api-platform/core#3009

Merged

1 task

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

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

Conversation

Simperfit commented Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

dunglas commented Nov 30, 2017

Simperfit commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL commented Nov 30, 2017 • edited Loading

Simperfit commented Nov 30, 2017 via email

ro0NL commented Nov 30, 2017

Simperfit commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aerendir Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Aerendir commented Dec 6, 2017

Simperfit commented Dec 6, 2017

Simperfit commented Jan 10, 2018

Simperfit commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented Feb 16, 2018

Simperfit commented Feb 16, 2018

dunglas commented Feb 19, 2018

wadjeroudi commented May 3, 2018

dunglas commented May 3, 2018

wadjeroudi commented May 4, 2018

dunglas commented May 4, 2018

xabbuh commented May 4, 2018

wadjeroudi commented May 4, 2018

Simperfit commented Nov 30, 2017 •

edited

Loading

ro0NL commented Nov 30, 2017 •

edited

Loading

Aerendir Dec 4, 2017 •

edited

Loading

dunglas Dec 6, 2017 •

edited

Loading

Simperfit Feb 16, 2018 •

edited

Loading