thephpleague / csv

CSV data manipulation made easy in PHP
https://csv.thephpleague.com
MIT License
3.34k stars 336 forks source link

TabularDataReader::getRecords()->current() can return NULL #514

Closed josefsabl closed 9 months ago

josefsabl commented 9 months ago

Bug Report

Information Description
Version 9.12.0
PHP version 8.3.1
OS Platform Alpine Linux

Summary

In 9.12.0 the \League\Csv\Statement::create()->process('...')->getRecords()->current() started to return NULL even if the records is not empty.

Iterating over iterator works as expected.

Standalone code, or other way to reproduce the problem

var_dump(
    $records = \League\Csv\Statement::create()
        ->process(
            \League\Csv\Reader::createFromString(
                'hello;world'
            )
                ->setDelimiter(';')
        )
        ->getRecords()
        ->current()
);

Expected result

Version <9.11.0 prints:

array(2) {
  [0]=>
  string(5) "hello"
  [1]=>
  string(5) "world"
}

Actual result

Version >=9.12.0 (including master) prints:

NULL

Checks before submitting

nyamsprod commented 9 months ago

@josefsabl thanks for using the library.

To me what you are experiencing/describing is not a bug but rather an undocumented and unsupported way of using the package. The method is expected to be used either with a foreach construct or via the iterator_to_array function.

To me the documented way of doing what you were expecting is as follow:

<?php

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

var_dump(
    $records = Statement::create()
        ->process(
            Reader::createFromString('hello;world')
                ->setDelimiter(';')
        )
-        ->getRecords()
-        ->current()
+        ->first()
);

You may use nth instead of first if you want another specific record see the documentation page for more information on those methods.

Using TabularDataReader::first or TabularDataReader::nth, the return value is always consistent regardless of the version you are using. Hence why those methods are present and in the public API

nyamsprod commented 9 months ago

@josefsabl after consideration a quick fix was added to correct the behaviour. What you are experiencing is due to how PHP handles iterator. The changes made during 9.12.0 development did not broke the Iterator contract but you are currently getting an un-rewinded Iterator. To fix your current code you can either wait for 9.15.0 to be release or do the following:

<?php

$records = Statement::create()
    ->process(
        Reader::createFromString('hello;world')
            ->setDelimiter(';')
    )
    ->getRecords();
+ $records->rewind();
$records->current();

When used via foreach or iterator_to_array PHP automatically does the rewind for you otherwise you are responsable for making sure the rewind was performed. Hence why I suggested that the correct way to use getRecords is via those two method/structures.

nyamsprod commented 9 months ago

@josefsabl After much consideration I've decided to go along the POLA principle aka Principle Of Least Astonishment.

This means that I will not consider the current ticket as it being a bug but rather the expected behaviour in PHP language. So there's no need to wait for the next minor version for fixing this behaviour.

Currently to align with PHP behaviour you only have 2 possibilities. Those I gave you in response to this ticket: