thephpleague/csv

Can I manipulate/replace header before serialization?

eldair opened this issue · 11 comments

eldair commented

(Fill in the relevant information below to help triage your issue.)

Q A
Version 9.9.0

Question

Hello, is there a Reader option/method to manipulate (or replace) header before data serialization?

eldair commented

I see there was a header formatter feature but it was removed here #466
Pull request says it should be moved to an extension but which extension is that?

If you look into the documentation you will see that you can change/update the header easily with the getRecords method .

I hope this answer your question.

eldair commented

@nyamsprod Yeah, I see it. Can we add headers as an argument for jsonSerialize (and getItterator) method which calls getRecords? I can make a pull request, it is a small non-breaking change

Why 🤔 ? These methods do not take any arguments in PHP

eldair commented

So that I can pass headers to getRecords via jsonSerialize method instead of making my own jsonSerialize method

Change this:

    /**
     * @throws Exception
     */
    public function jsonSerialize(): array
    {
        return array_values([...$this->getRecords()]);
    }

into this

    /**
     * @throws Exception
     */
    public function jsonSerialize(array $header = []): array
    {
        return array_values([...$this->getRecords($header)]);
    }

So I can do this $file->jsonSerialize($newHeader); instead of array_values([...$file->getRecords($newHeader)])

Nothing prevent you from using a Statement class instead to achieve your goal 🤔. No need to update change or BC break current codebase

eldair commented

I don't understand :| could you type an example?

On the other hand this wouldn't be any break, just arguments passthrough (it could just use ...func_get_args() as argument for getRecords call). If there is a method which just wraps and calls another method of the same class, why shouldn't it pass arguments when it has no arguments of its own? Also there is no other access to headers since they are replaced/set in getRecords.

I was writting from my phone here's a simple example.

<?php

use League\Csv\Reader;
use League\Csv\Statement;

$data = <<<CSV
field 1,field 2, field 3
1,2,3
CSV;

$csv = Reader::createFromString($data);
$csv->setHeaderOffset(0);
$result = Statement::create()->process($csv, ['field one', 'field two', 'field three']);
echo json_encode($result, JSON_PRETTY_PRINT), PHP_EOL;

The output will be

[
    {
        "field one": "1",
        "field two": "2",
        "field three": "3"
    }
]

You can learn more about the Statement class in the documentation

eldair commented

@nyamsprod thanks for the example but how is that better? I would still need to do serialization but with one additional class in the middle and more lines of code.
Would you accept pull request to update jsonSerialize method I don't see why that method shouldn't pass any arguments to it's call to getRecords, please explain.

it is better because:

  • You do not need to update/change the current package
  • you can perform the serialization you were expecting at least as far as I understood it

From a package maintainer here's how I see it. You are trying to offload your own business requirement onto the package while the package does not really need the feature you are requesting. My question is who should carry the burden of that maintenance ? The package or the one consuming it in a very specific way 🤔

I presented you with at least two alternatives to solve your issue without having to change a single line of code or deviate from PHP standard and best practices. Changing an established contract in the PHP community does not seem reasonable to me given that you request is already solved within the capabilities offered by the current codebase.

TL;DR:

  • updating jsonSerialize signature is a no go because it goes against PHP contracts;
  • using/calling jsonSerialize directly in code is against PHP best practices;
eldair commented

Thanks for the explanation, I completely understand your point of view and I did not see that it implements JsonSerializable interface.