Since 9.12.0 TabularDataReader::getRecords is stricter - exception is thrown with string keys for header about csv HOT 10 CLOSED

@pwolanin thanks for using the package.

The change is described on the CHANGELOG in the 9.12.0 release notes.
The change is described in the issue/RFC #498 and implemented with the following PR #499
The change is documented on the documentation website getRecords

The described issue is discussed there too.

Also getRecords signature has not changed. the docblock always expected a list of string array<string> and not an hashmap array<array-key, string>. If you use strings as key it was already an unmaintained behaviour which was not enforced in the code. Now by implementing the full mapping feature, the behavioir is enforced.

from csv.

In my copy of the code at 9.14.0 it has this:

Parameters:
array<array-key, string|int> $header

As the doc for:

\League\Csv\Reader::computeHeader()

Which is the method that throws the exception. So I think there is a problem or unexpected change here.

from csv.

@pwolanin as stated the change is expected and described in various locations of the package.

from csv.

@pwolanin what I may do is maybe update the change and do an array_values only in case all the keys are strings. This may reduce what your see as being a breaking changes with the following result if your array consist of mixed key (string and integer) an exception would still be thrown.

Would that be acceptable as a mitigation path for you ?

Keep in mind that this mitigation would only work in v9 and will be removed in v10. Whenever that version comes out.

from csv.

@nyamsprod I think that would be helpful to avoid a perceived API change. In my case I can fix the exception by calling array_values() in my code on the header array, but other people may not have as much control.

from csv.

@pwolanin question can you share a snippet on how you submitted a records with an header with string keys ?

Reason I am asking is because the mitigation is breaking more things than expected so I am not confident in implemeting it.

from csv.

So this is in the context of a Drupal CSV migration.
https://git.drupalcode.org/project/migrate_source_csv/-/blob/8.x-3.x/src/Plugin/migrate/source/CSV.php?ref_type=heads#L271

My code is a subclass of CSV.php and overrides the public function initializeIterator() to add some extra logic. Looking now I see there is an array_values() call in CSV.php.

My code does this after constructing the header, and where I just added the array_values() call to fix the exception:

return $this->getGenerator($this->getReader()->getRecords(array_values($header)));

If I condense that logic down, it's basically:

$csv = fopen($path, 'r');
$reader  = Reader::createFromStream($csv);
$records = $reader->getRecords($header);
return $this->getGenerator($records);

Perhaps the difference is that the header array is being passed in to getRecords() and you're expecting an empty array coming in for your more common use?

from csv.

yes either an empty array or a array with integer as array-key. I find it strange or uncommon to give header an array with array-key as string.

So adding some mitigation adds some side-effects inside the internal codebase that I do not want to tackle. So I am a bit torn in a sense that I believe the error message is a better way of handling the situation as it

• clearly. state the issue
• give hints to the developer on how to solve it

While the proposed work around adds another type of implicit rule about conversion the user may not be aware of.

Also with the current code, the following is already acceptable

$reader = Reader::createFromString($csv);
dump([...$reader->getRecords(['1' => 'toto', 4 => 'tata', '3' => 'tutu'])]);

As PHP will implicitly convert the string '1' into an integer.

On the other hand, to me, the following should have failed a long time ago. At least if it use to work, to me, it was a bug:

$reader = Reader::createFromString($csv);
dump([...$reader->getRecords(['toto' => 'toto', 'tata' => 'tata', 'tutu' => 'tutu'])]);

from csv.

Sure, I don't think it's essential to add the mitigation since the fix should usually be easy.

Maybe fix the doc on \League\Csv\Reader::computeHeader() ?

from csv.

doc block improve for internal computeHeader method

from csv.