Made str_getcsv a removable formatter

[hannesvdvreken]

Like this, users get the option to call $writer->clearFormatters() and pass in any type of object and let their custom transformers do the converting to arrays.

[hannesvdvreken]

For example this would enable me to do this:

$this->writer = Writer::createFromFileObject(new SplTempFileObject())
    ->clearFormatters()
    ->addFormatter(function ($row) {
        // Formatter to transform every ORM object to array.
        if ($row instanceof Model) {
            return $row->toArray();
        }

        return $row;
    })
    ->addFormatter([$transformer, 'transform']);

// Insert all data
$writer->insertOne($transformer->getHeaders());
$writer->insertAll($repository->all()); // This returns a generator that yields every orm object

// When done, make a response or something.
return new Response((string) $writer);

[nyamsprod]

TL;DR

So you are basically enabling Writer::insertOne to handle any type of argument as long as you provide the formatter that go along with it

• by relaxing the formatter definition
• moving the Writer::insertOne usage of str_getcsv into a formatter and applying it by default to avoid BC break.

Question/Issue
By relaxing the formatter definition you are adding a BC break in any currently working formatter. Because They are all build knowing that they will receive an array. With your change the order in which they are register become important and may break the formatting.

[hannesvdvreken]

You're concerned about BC and rightfully so.

• I think, as the str_getcsv formatter is added in the constructor, it will always go first.
• Only when the clearFormatters() method is called before any other formatter is added, this default formatter will be removed. This is a risk, cause some people might call this "just to be sure". In the documentation there is nothing stating there are no formatters when a writer is constructed.
• About the formatters accepting arrays: when they pass something different to the insertOne method, this would throw an error. There is no automatic casting to arrays in PHP (afaik) because of array type hinted function arguments. So nobody will be passing it something else than arrays, thus formatters will not complain because of the relaxation (it's not defined in an interface). Logically I don't think this is a BC break.

[hannesvdvreken]

I'll make sure to send a PR to adjust this too if you decide to accept this PR:
https://github.com/thephpleague/csv/blob/gh-pages/inserting.md#csv-formatter

[nyamsprod]

Only when the clearFormatters() method is called before any other formatter is added, this default formatter will be removed.

But the documentation does states:

The Writer class will see if the row is an array, if not it will try to convert it into a proper array;

So you are indeed introducing a BC on expectations.

Also according to the documentation around formatters the signature is

function(array $row): array

So you are indeed changing a public API signature even if you are relaxing it, so to me this is also a BC break. It may not be obvious but the formatters goals is to format an already created/pushed array. It is not to generate this array to begin with. So here's what I would have done with your example. (I only kept the important parts).

function convertModelToArray(Iterator $iterator)
{
    foreach ($iterator as $model) {
        yield $model->toArray();
    }
}

$writer = Writer::createFromFileObject(new SplTempFileObject());
$writer->insertAll(convertModelToArray($repository->all()));

There's also a reason behind the current formatter signature. By expecting an array and returning an array you can more easily use the Pipeline pattern, which is the strength of using formatters.

Now to be totally honest I do feel that the str_getcsv portion of Writer::insertOne should be removed in the next major release to enforce the array typehint on the method. Enforcing rules are better for maintenance that relaxing ones and it is the way forward even in PHP7.

[hannesvdvreken]

Fair enough 😉

So here's what I would have done with your example.

Good suggestion! Very clean and efficient.

By expecting an array and returning an array you can more easily use the Pipeline pattern

The php league's pipeline doesn't have a restriction on what is piped or what one stage might expect, though: https://github.com/thephpleague/pipeline

I feel you're not inclined to merge this. I'm ok with this PR being closed.

[nyamsprod]

To tell the truth I'm torn because the idea is good (the part where you remove str_getcsv was a eye opener to me) but the consequences makes me hesitant :( .

Yes the League pipeline is not that strict because it needs to be general purpose I guess. When I adapted it in League/Uri the first thing I did was to enforce typechecking on input and output.

So yes I'm gonna reject the PR even if it has great part in it that will definetly be added in futur versions of the lib.

Thanks anyway.

PS: I see that you live in Belgium just like me, I'm in Brussels!! (damn I should go to more PHP groups in belgium but I always fail to find time for that)

[hannesvdvreken]

No problem, it was worth the try. Thanks for your help as well. Hope to see you at this meetup in 2 weeks?