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

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

Closed
6 tasks done
josefsabl opened this issue Jan 16, 2024 · 3 comments
Closed
6 tasks done

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

josefsabl opened this issue Jan 16, 2024 · 3 comments
Assignees
Labels

Comments

@josefsabl
Copy link

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

  • Be sure that there isn't already an issue about this. See: Issues list
  • Be sure that there isn't already a pull request about this. See: Pull requests
  • I have added every step to reproduce the bug.
  • If possible I added relevant code examples.
  • This issue is about 1 bug and nothing more.
  • The issue has a descriptive title. For example: "JSON rendering failed on Windows for filenames with space".
@nyamsprod
Copy link
Member

nyamsprod commented Jan 16, 2024

@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 nyamsprod self-assigned this Jan 16, 2024
@nyamsprod nyamsprod changed the title Bug (or at least BC break) in 9.12.0. \League\Csv\Statement::create()->process('...')->getRecords()->current() returns NULL even if it should not Bug (or at least BC break) in 9.12.0. getRecords()->current() returns NULL even if it should not Jan 16, 2024
@nyamsprod nyamsprod changed the title Bug (or at least BC break) in 9.12.0. getRecords()->current() returns NULL even if it should not TabularDataReader::getRecords()->current() can returns NULL Jan 16, 2024
@nyamsprod nyamsprod changed the title TabularDataReader::getRecords()->current() can returns NULL TabularDataReader::getRecords()->current() can return NULL Jan 16, 2024
@nyamsprod nyamsprod removed the wontfix label Jan 16, 2024
@nyamsprod
Copy link
Member

@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
Copy link
Member

nyamsprod commented Jan 19, 2024

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

  • Explicitly call the rewind method on the returned Iterator;
  • Use the TabularDataReader::nth or TabularDataReader::first methods from the public API.

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

No branches or pull requests

2 participants