Fix SplFileObject::fgets() RETURN_THROWS() assertion failed

[twose]

@Girgias

[twose]

It frustrates me that SplFileObject cannot write code like this:

while (!feof($fp)) {
    $content .= fread($fp, 8192);
}

SplFileObject way throws RuntimeException after 6f87a5c .
Because we can't know where the end of the real file is in advance (but SplTempFileObject can, I guess that's also the reason for the difference.)

And the commit causes a behavior change, in #8563, reporter expect that key will be 5 and 5, but now it's 4 and 4. I don't know which one is right...
@Girgias Could you help me to clear my confusion?

[Girgias]

That fix looks incorrect, the whole point of this branch is that when EOF is hit at the end of a loop it doesn't return an empty string which doesn't exist.

Can you point out which RETURN_THROWS() assertion is triggered?

[twose]

php-src/ext/spl/spl_directory.c

Lines 2213 to 2215 in eb5b12c

	if (spl_filesystem_file_read_ex(intern, /* silent */ false, /* line_add */ 1) == FAILURE) {
	RETURN_THROWS();
	}

The test which I provided in this PR can reproduce this assertion failure...
It looks like we have no test coverage for this case.
For real file, we will get NULL from php_stream_get_line(), and php_stream_eof() returns true only in the next call.

[Girgias]

It frustrates me that SplFileObject cannot write code like this:

while (!feof($fp)) {
    $content .= fread($fp, 8192);
}

SplFileObject way throws RuntimeException after 6f87a5c . Because we can't know where the end of the real file is in advance (but SplTempFileObject can, I guess that's also the reason for the difference.)

And the commit causes a behavior change, in #8563, reporter expect that key will be 5 and 5, but now it's 4 and 4. I don't know which one is right... @Girgias Could you help me to clear my confusion?

The thing about the index is that the first line is index 0, so it's an off by one error, as if there are 5 total lines then the 5th line is index 4.

About the code that you cannot write, was it possible to write this prior to my fix? Or has this limitation always existed?

(for the record SPL is hell to understand, debug, and fix properly IMHO and I'm kind hating that I started trying to fix it as I think the only long term sane thing to do is create a replacement (be that when streams get moved to objects or otherwise) so that we can nuke SPL into Siberia)

[twose]

The thing about the index is that the first line is index 0, so it's an off by one error, as if there are 5 total lines then the 5th line is index 4.

I see.

About the code that you cannot write, was it possible to write this prior to my fix? Or has this limitation always existed?

Have a try with this:

<?php
$file = new SplFileObject(__FILE__);
while (!$file->eof()) {
    var_dump($file->fgets());
}

Before 6f87a5c, it works because the final fgets() will return an empty string instead of returning failure and in the next loop php_stream_eof() returns true.

(for the record SPL is hell to understand, debug, and fix properly IMHO and I'm kind hating that I started trying to fix it as I think the only long term sane thing to do is create a replacement (be that when streams get moved to objects or otherwise) so that we can nuke SPL into Siberia)

I have to say I agree with you.

[twose]

https://3v4l.org/MfAeT

[cmb69]

This is a pretty serious issue. Code which formerly worked, would now segfault, since intern->u.file.current_line == NULL:

php-src/ext/spl/spl_directory.c

Line 2216 in 9f29e2d

RETURN_STRINGL(intern->u.file.current_line, intern->u.file.current_line_len);

PS: Nonsense. Please disregard.

[twose]

As the commit has been reverted via 79a2832, and @Girgias said that the only long-term sane thing to do is create a replacement, I recommend that do not make fixes for inconsistent behavior to ensure backward compatibility (or consider modifying the behavior of SplTempFileObject, 🧐 is also another hell...).

This patch has been unavailable due to the revert, so, closed.