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

str_getcsv() and friends should not explicitly tell about "optional" parameters #4175

Open
8ctopus opened this issue Nov 25, 2024 · 6 comments · May be fixed by #4177
Open

str_getcsv() and friends should not explicitly tell about "optional" parameters #4175

8ctopus opened this issue Nov 25, 2024 · 6 comments · May be fixed by #4177

Comments

@8ctopus
Copy link
Contributor

8ctopus commented Nov 25, 2024

Description

The following code:

<?php

$csv = <<<CSV
1,30,2,29
CSV;

$columns = str_getcsv($csv, ',', '', '\\');

var_dump($columns);

Resulted in this output in php 8.3.10:

array(4) {
  [0] =>
  string(1) "1"
  [1] =>
  string(2) "30"
  [2] =>
  string(1) "2"
  [3] =>
  string(2) "29"
}

Now in php 8.4.1, I get this:

Fatal error: Uncaught ValueError: str_getcsv(): Argument php/php-src#3 ($enclosure) must be a single character in K:\dev\github\php-nano-csv\test
.php on line 7

ValueError: str_getcsv(): Argument php/php-src#3 ($enclosure) must be a single character in K:\dev\github\php-nano-csv\test.php on line 7

Call Stack:
    0.4199     403080   1. {main}() K:\dev\github\php-nano-csv\test.php:0
    0.4200     403112   2. str_getcsv($string = '1,30,2,29', $separator = ',', $enclosure = '', $escape = '\\') K:\dev\github\php-nano
-csv\test.php:7

I saw that the behavior of the function was changed, not to permit an empty enclosure, however who are we meant to deal with unenclosed csv?

PHP Version

PHP 8.4.1

Operating System

Windows 10

@8ctopus 8ctopus added bug Documentation contains incorrect information Status: Needs Triage labels Nov 25, 2024
@Girgias
Copy link
Member

Girgias commented Nov 25, 2024

If the CSV is not enclosed because it doesn't have any " then you can just leave the default value.

$columns = str_getcsv($csv, ',', '"', '\\');

Will give you the same output as with an empty string.
We aligned the behaviour of str_getcsv() to be identical to fgetcsv(), fputcsv() and co.

@8ctopus
Copy link
Contributor Author

8ctopus commented Nov 25, 2024

@Girgias Thank you for the quickly reply. I confirm that it works not only with the default enclosure character, but with any enclosure character.

Should the documentation be updated to say that if the CSV does not use enclosure characters leaving the default character works? I feel the way it's currently explained is confusing.

@cmb69
Copy link
Member

cmb69 commented Nov 25, 2024

In my opinion, the documentation is okay. After all, generally there needs to be an enclosure character, and in those cases where the CSV can do without, using the default is fine. (I find https://3v4l.org/Lf0SD a bit fishy, though.)

And frankly, I don't think str_getcsv() makes much sense at all. How are you supposed to get a suitable string when reading general CSV? You can't split by line, because the line break may be enclosed.

@Girgias
Copy link
Member

Girgias commented Nov 26, 2024

I suppose we could remove the word "optional" from the descriptions?

@cmb69
Copy link
Member

cmb69 commented Nov 26, 2024

Oh, right; users can figure that from the signature.

@cmb69
Copy link
Member

cmb69 commented Nov 27, 2024

Okay, transferring to doc-en then.

@cmb69 cmb69 transferred this issue from php/php-src Nov 27, 2024
@cmb69 cmb69 removed bug Documentation contains incorrect information Status: Needs Triage labels Nov 27, 2024
@cmb69 cmb69 changed the title str_getcsv str_getcsv() and friends should not explicitly tell about "optional" parameters Nov 27, 2024
Girgias added a commit to Girgias/doc-en that referenced this issue Nov 27, 2024
@Girgias Girgias linked a pull request Nov 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants