CsvEncoder handling variable structures and custom header order #24256

backbone87 · 2017-09-19T10:31:31Z

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

This PR improves the CsvEncoder to handle variable nesting structures and adds a context option that allows custom csv header order.

backbone87 · 2017-09-19T10:41:37Z

fyi: the header merging of variable array structures is specifically optimized for collection like substructures, to maintain good ordering. the testcase included is not such a case

backbone87 · 2017-09-19T10:50:28Z

@dunglas would be nice if you could review, since the original impl is done by you

nicolas-grekas · 2017-09-20T05:26:15Z

@backbone87 I changed the base branch to 3.4 because that's the correct target. Could you please rebase on 3.4 on your side?

backbone87 · 2017-09-20T06:10:32Z

@nicolas-grekas done

dunglas · 2017-09-20T05:57:06Z

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

@@ -177,7 +180,7 @@ public function supportsDecoding($format)
     * @param string $keySeparator
     * @param string $parentKey
     */
-    private function flatten(array $array, array &$result, $keySeparator, $parentKey = '')
+    protected function flatten(array $array, array &$result, $keySeparator, $parentKey = '')


Can we avoid adding protected methods? Decoration is usually a better extension point (apply for all methods), and it's easier to maintain for us...

actually the flatten function is pure and can be made public static
about the options: you have no way to access the default settings of the CsvEncoder, which requires duplicating the settings (in code and DI) in either case (inheritance or composition)

i would be happy to make it private, because i totally understand your reasoning for it, but compositions or subtypes need to be able to access the settings, imho

edit: i actually tried to extend the CsvEncoder in my current project and add the features from this PR via inheritance, but in the end i needed to copy over the whole encoder. one could actually use decoration and prepare the data acoordingly, so the current encoder doesnt error out, but i still need to copy at least the key_separator setting

Well, making things protected means that we have to maintain backward compatibility for it forever (well, until the next major version). And inheritance-based extension points are the most painful ones when dealing with BC layers.
So we are very careful about introducing them. Please keep this method private for now. Making it accessible to the outside should be a feature request on its own (with arguments justifying the increased maintenance cost)

dunglas · 2017-09-20T05:58:14Z

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

+            $this->flatten($value, $flattened, $keySeparator);
+            $value = $flattened;
+        }
+        unset($value);


What's the benefit of unsetting this var explicitly?

its a cleanup of the dangling reference var. if one would do $value = 'foo' after the loop, he would overwrite the last iterated item in the array.

Yes but $value is not accessed after.

its more of a safety for futher modifications (i do it always as a best practice), this gets lost fast and could cause confusion (though in this case is covered by testcases)

dunglas · 2017-09-20T06:10:55Z

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

-            } elseif (array_keys($result) !== $headers) {
-                throw new InvalidArgumentException('To use the CSV encoder, each line in the data array must have the same structure. You may want to use a custom normalizer class to normalize the data format before passing it to the CSV encoder.');
-            }
+        $headers = array_merge(array_values($headers), array_diff($this->extractHeaders($data), $headers));


Computing the header list dynamically like you've done is very interesting but it can be very costly on large datasets. Shouldn't it introduced as an option? It would be nice to be able to choose between the previous behavior and your implementation.

it should be in the same complexity class as the rest of the encoder. we can reduce the cost a little bit by removing the array_keys in the loop (that indeed gets executed for each item). but other than that its just n*m isset calls for the previously supported cases, which compared to normalization and actual CSV encoding, is negliable.

dunglas · 2017-09-20T06:19:01Z

src/Symfony/Component/Serializer/Tests/Encoder/CsvEncoderTest.php

-    public function testEncodeNonFlattenableStructure()
+    public function testEncodeVariableStructure()
+    {
+        $value = array(


What about:

array( 'a' => array('a', 'b'), 'a' => 'foo', )

This case looks not easy to handle, it's one of the reasons of the current behavior.

actually this one gets handled just fine, but the result may be a little bit unexpected:

a.0,a.1,a a,b, ,,foo

i dont think we need to handle this differently in the encoder, because that one is indeed a custom normalizers job

The output is a bit weird but why not.

btw this is more a problem of the flattening algorithm, which is untouched by the PR.
compare it with the previously support case of:

[ ['a' => ['foo', 'bar'], 'a.0' => 'baz'], ]

dunglas

Very good improvements @backbone87!

fabpot · 2017-09-27T04:33:53Z

Can you add a note in the component CHANGELOG before we merge? Thanks.

backbone87 · 2017-09-27T07:14:18Z

@fabpot done, but i guess, i need to rebase?

fabpot · 2017-09-27T14:20:11Z

Thank you @backbone87.

…der order (Oliver Hoff) This PR was squashed before being merged into the 3.4 branch (closes #24256). Discussion ---------- CsvEncoder handling variable structures and custom header order | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23278 | License | MIT | Doc PR | TBD This PR improves the CsvEncoder to handle variable nesting structures and adds a context option that allows custom csv header order. Commits ------- d173494 CsvEncoder handling variable structures and custom header order

carsonbot added Status: Needs Review Feature labels Sep 19, 2017

backbone87 force-pushed the csv-encoder branch from 0719b76 to 3c2aa91 Compare September 19, 2017 10:37

backbone87 force-pushed the csv-encoder branch from 3c2aa91 to dd8de08 Compare September 19, 2017 10:43

nicolas-grekas changed the base branch from master to 3.4 September 20, 2017 05:25

nicolas-grekas added this to the 3.4 milestone Sep 20, 2017

backbone87 force-pushed the csv-encoder branch from dd8de08 to d5060ac Compare September 20, 2017 05:54

dunglas requested changes Sep 20, 2017

View reviewed changes

backbone87 force-pushed the csv-encoder branch from d5060ac to 16c1988 Compare September 20, 2017 12:08

Oliver Hoff added 2 commits September 20, 2017 15:24

allow variable structures for CSV encoder

ad893fb

pass CSV headers via context to CsvEncoder

4aad92a

backbone87 force-pushed the csv-encoder branch from 16c1988 to 4aad92a Compare September 20, 2017 13:25

dunglas approved these changes Sep 21, 2017

View reviewed changes

nicolas-grekas approved these changes Sep 26, 2017

View reviewed changes

fabpot approved these changes Sep 27, 2017

View reviewed changes

CHANGELOG entries for the CsvEncoder improvements

cc465b3

fabpot closed this Sep 27, 2017

javiereguiluz mentioned this pull request Oct 3, 2017

[Serializer] CsvEncoder handling variable structures and custom header order symfony/symfony-docs#8455

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

lajosthiel mentioned this pull request Nov 24, 2020

[Serializer] when normalizing, order the properties in a way controllable by the user #27441

Open

Seb33300 mentioned this pull request May 18, 2024

[Serializer] csv_headers not working as expected #55048

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CsvEncoder handling variable structures and custom header order #24256

CsvEncoder handling variable structures and custom header order #24256

backbone87 commented Sep 19, 2017

backbone87 commented Sep 19, 2017

backbone87 commented Sep 19, 2017

nicolas-grekas commented Sep 20, 2017

backbone87 commented Sep 20, 2017

dunglas Sep 20, 2017

backbone87 Sep 20, 2017

backbone87 Sep 20, 2017 •

edited

Loading

stof Sep 20, 2017

dunglas Sep 20, 2017 •

edited

Loading

backbone87 Sep 20, 2017

dunglas Sep 20, 2017

backbone87 Sep 20, 2017

dunglas Sep 20, 2017 •

edited

Loading

backbone87 Sep 20, 2017

dunglas Sep 20, 2017 •

edited

Loading

backbone87 Sep 20, 2017

dunglas Sep 20, 2017

backbone87 Sep 20, 2017 •

edited

Loading

dunglas left a comment

fabpot commented Sep 27, 2017

backbone87 commented Sep 27, 2017

fabpot commented Sep 27, 2017

CsvEncoder handling variable structures and custom header order #24256

CsvEncoder handling variable structures and custom header order #24256

Conversation

backbone87 commented Sep 19, 2017

backbone87 commented Sep 19, 2017

backbone87 commented Sep 19, 2017

nicolas-grekas commented Sep 20, 2017

backbone87 commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backbone87 Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Sep 20, 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

dunglas Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backbone87 Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

dunglas left a comment

Choose a reason for hiding this comment

fabpot commented Sep 27, 2017

backbone87 commented Sep 27, 2017

fabpot commented Sep 27, 2017

backbone87 Sep 20, 2017 •

edited

Loading

dunglas Sep 20, 2017 •

edited

Loading

dunglas Sep 20, 2017 •

edited

Loading

dunglas Sep 20, 2017 •

edited

Loading

backbone87 Sep 20, 2017 •

edited

Loading