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

Many, many times it is written in the documentation differently than it actually is. #551

Closed
gander opened this issue Jan 11, 2025 · 10 comments

Comments

@gander
Copy link

gander commented Jan 11, 2025

Bug Report

Many, many times it is written in the documentation differently than it actually is.

Information Description
Version 9.21.0
PHP version 8.x
OS Platform Linux

Summary

e.g. here https://csv.thephpleague.com/9.0/reader/tabular-data-reader/ getRecords

The getRecords enables iterating over all records from the current object. If the optional $header argument is given, it will be used as a mapper on the record and will update the record header and the value position.

Currently, each call to getRecords() returns a records mapped from headers. Each method returns mapped if I have set setHeaderOffset.

@nyamsprod
Copy link
Member

@gander thanks for using the package could you provide an example of what you are describing because I fail to understand the actual issue you are referring to 🤔

@nyamsprod nyamsprod self-assigned this Jan 11, 2025
@gander
Copy link
Author

gander commented Jan 11, 2025

CSV with header row

$reader = Reader::createFromStream(fopen(__DIR__.'/input.csv', 'r'))
    ->setHeaderOffset(0)
    ->getRecords()
;

Result:

[[
 "x" => "A",
 "y" => "B",
 "z" => "C",
]]

Should be:

[[
 0 => "A",
 1 => "B",
 2 => "C",
]]

@gander
Copy link
Author

gander commented Jan 11, 2025

Following documentation, $header argument should be TRULY optional, and to map, should be used code:

$records = $reader->getRecords($reader->getHeader());

@gander
Copy link
Author

gander commented Jan 11, 2025

For now, my workaround:

$reader = Reader::createFromStream(fopen(__DIR__.'/input.csv', 'r'));
$header = $reader->slice(0, 1)->first();
$records = iterator_to_array($reader->slice(1));

@nyamsprod
Copy link
Member

nyamsprod commented Jan 11, 2025

CSV with header row

$reader = Reader::createFromStream(fopen(__DIR__.'/input.csv', 'r'))
    ->setHeaderOffset(0)
    ->getRecords()
;

Result:

[[
 "x" => "A",
 "y" => "B",
 "z" => "C",
]]

Should be:

[[
 0 => "A",
 1 => "B",
 2 => "C",
]]

You are incorrect ... it is doing exactly what it is said in the documentation.

By setting the header offset you are instructing the Reader class to use the content of the row at index 0 as the header and whenever you call the getRecords method the header is calculated and used to create the associative array that you have. Hence why I fail to understand your issue. Also the library remove the selected header record from the result as it is the header and not the actual content of the CSV.

If you wish not to use the header offset and have it included in your CSV object just do not use setHeaderOffset or use setHeaderOffset(null).

So this is definitely not a bug but expected behaviour.

@gander
Copy link
Author

gander commented Jan 11, 2025

So how do you use setHeaderOffset, get the header, and have the records not mapped with the header?

@nyamsprod
Copy link
Member

nyamsprod commented Jan 11, 2025

if you do not care about the header being set just iterate over the value and use array_values

$reader = Reader::createFromPath(__DIR__.'/input.csv', 'r');
$reader->setHeaderOffset(0);
iterator_to_array($reader->map(fn (array $row) => array_values($row)))

@gander
Copy link
Author

gander commented Jan 11, 2025

Still need header, some data mapped, and not mapped, so my workaround

$header = $reader->slice(0, 1)->first();
$reader->slice(666, 1)->mapHeader($header)->first();

@gander
Copy link
Author

gander commented Jan 11, 2025

I still think that there is room for improvement, but in the documentation. In one place it says that setHeaderOffset will result in mapping, but for every single function that has a different result, depending on the use of setHeaderOffset, there should be such an annotation.

@nyamsprod
Copy link
Member

I really do not understand what you are trying to achieve but I guess it serves your business logic and that's fine. The only thing I know it's that what you are doing is not a workaround because there is no bug to work around 🤷🏾‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants