Since 9.12.0 TabularDataReader::getRecords is stricter - exception is thrown with string keys for header

[pwolanin]

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

Q	A
Version	9.14.0

Question

There is a change from #499
7ccbe6d

This causes the Reader to throw an exception when the header keys are not numeric.

The header mapper indexes should only contain positive integer or 0. in /home/pwolanin/project/vendor/league/csv/src/Reader.php line 468

This behavior change is not documented or expected based on the release notes - is this a bug?

It worked fine before this release to have string keys for the header and the code could just call array_values() instead of throwing an exception.
https://github.com/thephpleague/csv/releases/tag/9.14.0

Checks before submitting

• [X ] Be sure that there isn't already an issue about this. See: Issues list
• [ X] Be sure that there isn't already a pull request about this. See: Pull requests
• [ X] I have read, searched and not found the information on the documentation website.
• [X ] I have read, searched and not found the information on PHP related forums and/or websites.
• [ X] This issue is about 1 question around the package with no business or domain specific logic related to a specific situation.
• [ X] The question has a descriptive title. For example: "Can I use the library with compressed CSV documents ?".

[nyamsprod]

@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.

[pwolanin]

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.

[nyamsprod]

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

[nyamsprod]

@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.

[pwolanin]

@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.

[nyamsprod]

@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.

[pwolanin]

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?

[nyamsprod]

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'])]);

[pwolanin]

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() ?

[nyamsprod]

doc block improve for internal computeHeader method