Skip to content
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 headers label support for CsvEncoder using csv_headers #55334

Closed
wants to merge 2 commits into from

Conversation

Seb33300
Copy link
Contributor

@Seb33300 Seb33300 commented May 18, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #55048
License MIT

Improve the existing csv_headers option of CsvEncoder to pass CSV headers labels.

$contextBuilder = (new CsvEncoderContextBuilder())->withHeaders([
    'Prénom' => 'firstName',
    'Nom' => 'lastName',
    'Nested field label' => 'nested.field',
]);

@@ -11,6 +11,7 @@ CHANGELOG
* Add `CamelCaseToSnakeCaseNameConverter::REQUIRE_SNAKE_CASE_PROPERTIES` context option
* Deprecate `AbstractNormalizerContextBuilder::withDefaultContructorArguments(?array $defaultContructorArguments)`, use `withDefaultConstructorArguments(?array $defaultConstructorArguments)` instead (note the missing `s` character in Contructor word in deprecated method)
* Add `XmlEncoder::CDATA_WRAPPING_PATTERN` context option
* Add headers label support for `CsvEncoder` using `csv_headers`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this to a new 7.2 section, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping I still have a chance to see it merged in 7.1 branch since the final version is not released yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Seb33300
Copy link
Contributor Author

I just added a test

@@ -242,14 +242,38 @@ public function testEncodeCustomHeaders()
CsvEncoder::HEADERS_KEY => [
'b',
'c',
'n.0',
'n.k',
Copy link
Contributor Author

@Seb33300 Seb33300 May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headers for nested data was already supported but not tested.

@valtzu
Copy link
Contributor

valtzu commented Jun 2, 2024

Shouldn't the mapping be used in both, encoding & decoding? Or is that already the case 🤔

Also I'm wondering, would it make more sense to implement this mapping in MetadataAwareNameConverter so that it would work with any format, not only csv?

@Seb33300
Copy link
Contributor Author

Seb33300 commented Jun 2, 2024

@valtzu not sure to understand your question.

This PR is not about mapping but adding labels to generated CSV, which is relevant only when exporting.

@valtzu
Copy link
Contributor

valtzu commented Jun 2, 2024

adding labels

@Seb33300 I assume by adding labels you mean csv column names / header – and those may exist when exporting or importing csv data.

To me it feels quite unintuitive that you can not import the exported file using decode with the same context as was used with encode.

$data = [['firstName' => 'foo', 'lastName' => 'bar']];

$context = [
    CsvEncoder::HEADERS_KEY => [
        'Prénom' => 'firstName',
        'Nom' => 'lastName',
    ],
];

$encoded = $this->serializer->encode($data, 'csv', $context);

/* =
Prénom,Nom
foo,bar
*/


$decoded = $this->serializer->decode($encoded, 'csv', $context);

/* =
Array
(
    [0] => Array
        (
            [Prénom] => foo
            [Nom] => bar
        )

)
*/

I would expect $decoded be equal to $data.

If this was implemented in a name converter, it would work both ways. Basically same as #[SerializedName]/#[SerializedPath] – but just get those from context instead of static configuration. As I understood from your comment, you'd be happy with such approach.

It would be something like 👇 (such thing is not implemented)

$csv = $serializer->serialize($data, 'csv', [
    AbstractNormalizer::ATTRIBUTES => ['firstName', 'lastName'],
    MetadataAwareNameConverter::MAPPING => [
        YourEntity::class => [
            'Prénom' => 'firstName',
            'Nom' => 'lastName',
        ],
    ],
]);

@Seb33300
Copy link
Contributor Author

Seb33300 commented Jun 2, 2024

You are right.
Thanks for the feedback.

@Seb33300 Seb33300 closed this Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Serializer] csv_headers not working as expected
4 participants