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 constructor arguement to return csv always as collection#25218
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
[Serializer] add a constructor arguement to return csv always as collection#25218
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| fclose($handle); | ||
| if (isset($context['alwaysAsCollection']) &&true ===$context['alwaysAsCollection']) { |
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.
The key should be in snake case, as for all other context’s keys.
By the way, can't it just becollection?
dunglas commentedNov 30, 2017
Should be merged in 4.1 as it's a new feature. |
cbe28c0 to7ebe8e0CompareSimperfit commentedNov 30, 2017
Status: Needs Review |
| } | ||
| fclose($handle); | ||
| if (isset($context['collection']) &&true ===$context['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.
Can beif ($context['collection'] ?? false) now that you targetmaster.
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.
This should be covered by Coding Standard. Is that missed?
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.
This is not covered by CS atm I think, since fabbot is not red.
ro0NL commentedNov 30, 2017 • 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.
Cant we just always return a collection? is there any real reason for mixed return values? Just curious :) otherwise edit: yeah sorry reading ticket now :( |
Simperfit commentedNov 30, 2017 via email
If we want to return only collection we need to do it in 5.0 since it’s aBC.maybe we should name it always_collection ?@dunglasLe 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 commentedNov 30, 2017
It really makes sense for beingCSV no? I dont understand why this logic must be here for "API" purpose. CSV is a collection of row(s) no? |
7ebe8e0 to1f0f748CompareSimperfit commentedDec 1, 2017
Status: Needs Review |
1f0f748 to855993bCompare| constENCLOSURE_KEY ='csv_enclosure'; | ||
| constESCAPE_CHAR_KEY ='csv_escape_char'; | ||
| constKEY_SEPARATOR_KEY ='csv_key_separator'; | ||
| constALWAYS_COLLECTION ='always_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.
does convention require us to prefix the value withcsv_?
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.
Good catch@ro0NL, I'm 👍 to use the prefix.
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.
Do we wanna toggle the default eventually? Side node; is#25227 related to this same convention in XML? If so shouldEncoderInterface know about the const in general to expose this logic to other encoders?
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.
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 theALWAYS_COLLECTION constant should be a general constant of which all encoders have to be aware and act on.
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.
Maybe we should add this on every encoders to be sure that we consistency using the context key
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.
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 that for sure this is required on the XML encoder. I don't know if it is required also on the others (JsonDecode andYamlEncoder).
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.
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.
Also theFixed tickets property of the opening comment should include also a reference to the issue I opened about the XML encoder... I think...
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 will take care of the needed modification
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.
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).
855993b to0b5588aCompareAerendir commentedDec 6, 2017
ping |
Simperfit commentedDec 6, 2017
Simperfit commentedJan 10, 2018
This is fixing a misbehaviour, should we really add a new normalizer to fix this ? |
b19ce4e tob22d1a0CompareSimperfit commentedFeb 2, 2018
PR Changed to remove the param in the constructor and add it as a context key. |
| } | ||
| fclose($handle); | ||
| if (isset($context['forceCollection']) &&$context['forceCollection']) { |
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.
AFAIK, we use snake case for all other keys, isn't it?
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.
done
23e0965 tob0bdaabCompare| } | ||
| fclose($handle); | ||
| if (isset($context['as_collection']) &&$context['as_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.
Maybe we could use a 3 value switch here (null/true/false) withnull (or not set) the current behavior (guess),true forcing a collection andfalse forcing an unique element.
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 think it's ok to saytrue should return the collection andfalse it the old behaviour.
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.
$context['as_collection'] ?? false?
Simperfit commentedFeb 16, 2018
ping@dunglas@nicolas-grekas this is ready. Travis failure is not related to this PR. |
Simperfit commentedFeb 16, 2018
Status: Needs Review |
b0bdaab tod19d05dComparedunglas commentedFeb 19, 2018
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 <!-- 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 singleCoding 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.Commits-------d19d05d [Serializer] add a context key to return csv always as collection
wadjeroudi commentedMay 3, 2018
@dunglas when will it be released ? it's only in master. |
dunglas commentedMay 3, 2018
wadjeroudi commentedMay 4, 2018
@dunglas There's no BC, it won't be in 3.4.x releases ? Thx. |
dunglas commentedMay 4, 2018
No, because it’s a new feature (we don’t allow non backward compatible code, even in 4.2). |
xabbuh commentedMay 4, 2018
@wadjeroudi New features are always only merged into the |
wadjeroudi commentedMay 4, 2018
I come after the "war", but I think it's not a new feature, it's a consistency fix IMHO. // If there is only one data line in the document, return it (the line), the result is not considered as a collection |
Uh oh!
There was an error while loading.Please reload this page.
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.