ext/spl: Fix default argument handling for named parameters in SplFileObject::fgetcsv()

[arshidkv12]

#22156

[devnexen]

Hi @arshidkv12 thanks for looking into but I m afraid it's more a band aid than a fix (and even it is incomplete). e.g. ; is not the only valid csv separator.

[arshidkv12]

You are correct. I think this should be changed in the stub.php file. Then manually assign the default value if it is null..

public function fgetcsv(string $separator, string $enclosure, string $escape): array|false {}

[devnexen]

A few suggested tests to add so the brittleness of the current approach is visible in CI.

1. Explicit , after a non-comma setCsvControl (the false-negative the guard creates).**

$f->setCsvControl(';', "\n", "\\");
$f->fwrite("a,b;c\n");
$f->seek(0);
var_dump($f->fgetcsv(',', '"', '\\'));   // user explicitly wants ,
// expected: ["a","b;c"]   patched: ["a,b","c"]   ← regression introduced by this PR

2. Enclosure-only via named arg (the bug is unfixed on enclosure).
$f->setCsvControl(',', "'", "\\");
$f->fwrite("a,'b,c'\n");
$f->seek(0);
var_dump($f->fgetcsv(escape: '\\'));     // named-skip fills enclosure with "
// expected: ["a","b,c"]   patched: ["a","'b","c'"]

3. Escape via named arg with a non-default enclosure (also exercises the is_escape_default deprecation path).
$f->setCsvControl(',', "'");
$f->fwrite("a,'b\\'c'\n");
$f->seek(0);
var_dump($f->fgetcsv(escape: '\\'));     // enclosure silently becomes "

4. Same bug shape on fputcsv() (the PR does not touch it).
$f->setCsvControl(';', "\n", "\\");
$f->fputcsv(['a','b'], escape: '\\');
$f->seek(0);
echo $f->fgets();                         // expected "a;b\n"   patched: "a,b\n"

5. Reflection round-trip to lock the API contract.
$rm = new ReflectionMethod(SplFileObject::class, 'fgetcsv');
foreach ($rm->getParameters() as $p) {
    var_dump($p->getName(), $p->allowsNull(), $p->getDefaultValue());
}

6. is_escape_default invariant. After setCsvControl(',', '"') (two args), fgetcsv() (no args) should still emit the escape-default deprecation. Confirms the rewrite
does not accidentally suppress the existing notice through the nullability change.

Two meta-points:
- The PR's own test calls setCsvControl(';', "\n", "\\") (three args), which flips is_escape_default to false and hides cases 3 and 6. Dropping the third arg from
setCsvControl already changes what is exercised, so both shapes are worth covering.
- Worth a quick run against the standalone fgetcsv() / fputcsv() in ext/standard as well, just to confirm those are unaffected (no instance state to clobber there),
so the fix scope is correctly limited to the SPL methods.

Note: suggested test cases with help of LLM.

[devnexen]

For final review, might be best to wait Gina's opinion not enough familiar with this part of the code :)

[arshidkv12]

I can fix fputcsv after completing this PR.

[Girgias]

I don't agree with the premise of this change. Making the type nullable is not an improvement.

Nor does it seem to fix the issue of providing the default value explicitly having a different behaviour? Moreover, this would need to be applied to all relevant functions (in SPL and ext/standard)

Finally, this is not a bugfix that is backportable.

[arshidkv12]

I don't agree with the premise of this change. Making the type nullable is not an improvement.

Nor does it seem to fix the issue of providing the default value explicitly having a different behaviour? Moreover, this would need to be applied to all relevant functions (in SPL and ext/standard)

Finally, this is not a bugfix that is backportable.

Thank you. Let me fix the bug.

[auxmoney-tbarcala]

@Girgias The suggestion to change the parameters to nullable was to make the API clearer. The default values defined in the stubs don't tell the real story, because they depend on the state of the SplFileObject. If $file->setCsvControl(...); is never called it works as described, but if any of the controls change, the defaults don't make sense anymore.

I mean, if I encounter the code below in the codebase:

$row = $file->fgetcsv();

... my IDE (as well as PHP documentation) tells me that the default values for the arguments are ,, " and \, so I'd expect passing these exact same values would not change the oucome:

$row = $file->fgetcsv(',', '"', '\\');

Without making the arguments nullable, what should be the expected outcome of the code below?

// $file contents is 'a,b;c'
$file->setCsvControl(';', "\n", "\\");
$row = $file->fgetcsv(',', '"', '\\');

I'd expect $row to be equal to ['a', 'b;c'], but with the current state of this PR it would be ['a,b', 'c'].

I agree with you that making the parameters nullable is not ideal, but I'd say it is preferrable in comparison to the current implementation. A better option would be, IMHO, to deprecate/remove either the method setCsvControl, or the arguments for fgetcsv.