Edit report at https://bugs.php.net/bug.php?id=81551&edit=1
ID: 81551
Updated by: dharman@php.net
Reported by: lucasfbustamante at gmail dot com
Summary: SplFileObject::fgets( does not advance line pointer
after SplFileObject::seek()
-Status: Feedback
+Status: Not a bug
Type: Bug
Package: SPL related
Operating System: Unix
PHP Version: 8.0.12
Assigned To: cmb
Block user comment: N
Private report: N
New Comment:
The current behaviour looks correct to me. I have read all the comments but I don't see any bug
that would need further fixing. We are not going to revert a bug fix just because it breaks existing
applications that relied on the buggy behaviour. Relevant https://xkcd.com/1172/
If we consider a proper example of fgets it works correctly after the fix. I tried a few variations
but I can't see any issues. https://3v4l.org/KPXcR
Considering your example, I believe the expectation is wrong.
---------------
$file = new \SplTempFileObject();
for ($i = 0; $i < 100; $i++) {
$file->fwrite("Foo\n");
}
// Puts the internal pointer at the start of line index 50 (0-based indexing)
$file->seek(50);
// Read the internal pointer value = 50
var_dump($file->key());
// Read the next available line (which is line at index 50)
var_dump($file->fgets());
// Read the internal pointer value = 50 (end of the line now)
var_dump($file->key());
// Read the next available line (which is line at index 51 as there's nothing more on line 50)
var_dump($file->fgets());
// Read the internal pointer value = 51 (end of the line)
var_dump($file->key());
---------------
The line pointer is incremented consistently since PHP 8. https://3v4l.org/m5Dhn
As such I am closing this bug report.
Previous Comments:
------------------------------------------------------------------------
[2021-10-23 13:09:13] rene at wp-staging dot com
Despite the fact that PHP 8.0.1 has been released already in January 2021 and is out for several
months I highly encourage you to consider to roll back this change. This will have a huge negative
effect on many projects if more hosting providers start to add PHP 8.X. Rolling back will affect
only a minority. This is a modification which is not backward compatible at all and needs huge
efforts for keeping projects cross compatible between 5.x/7.x and 8.0.1
On top of that, the doc has not been updated and still says that fgets() get the NEXT line while
this is not true any longer: https://www.php.net/manual/en/splfileobject.fgets.php
Such a substantial change needs to be better documented. Such a change can break application nearly
invisible. If it's not unit tested if input does not equals the expected output, users will not
notice the failure in their applications.
Chances are high that many affected projects even did not realized yet that their applications are
not working properly anymore with >= PHP 8.0.1
If this will be rolled back, we would have time to work out a more consistent solution which covers
every aspect of the SplFileObject and is either backward compatible or we at least can prepare
better documentation with more samples that covers upgrading aspects to create cross compatible
code.
Our team will gladly assist with that and we'd like to spent some of our resources on this.
------------------------------------------------------------------------
[2021-10-23 00:08:20] lucasfbustamante at gmail dot com
Indeed, the code snippets make much more sense in a foreach loop. I have re-written them in this
format to make it clearer.
The whole picture starts to make sense. I will recap some of the obvious parts for someone from the
outside that might be reading this.
If the iteration is starting from the beginning, all scenarios works consistently: https://3v4l.org/9BYb8
If the iteration is starting after a seek, PHP 8.0.1 fixed an issue where fgets() would
start from the next line, but seems have introduced an inconsistent behavior if next() is called
before current(): https://3v4l.org/UpN1G
In my personal opinion, I think it wasn't a good idea to fix the fgets bug, as it breaks many
applications. I am here because I run a database importing script that uses seek() and fgets(), and
I was getting a duplicated primary key error because 8.0.1 fetches one row above what 8.0.0 would.
I personally think it would have been best to document the behavior of seek + fgets returning the
next line, and leave the behavior unchanged, considering it part of the behavior of SplFileObject, I
am not sure if we are past the point of rolling back this fix.
Either way, if the behavior of next() then current() was not expected, I believe we should aim to
make it consistent with the previous PHP versions, as to minimize impact on applications.
------------------------------------------------------------------------
[2021-10-22 14:47:34] requinix@php.net
I don't believe that is correct.
It's a bit tricky because key/current/next/etc. are almost always called through iteration with
foreach and never manually, and when calling them manually you're expected to follow a standard
pattern:
for ($object->rewind(); $object->valid(); $object->next()) {
$key = $object->key();
$value = $object->current();
// ...
}
or in other words,
1. rewind
2. valid
3. key/current
4. next
5. goto 2
However it should not matter if you call key/current before the first next because those are
supposed to be read-only operations, yet here it does matter:
https://3v4l.org/BVMsV vs https://3v4l.org/s1G2E
Note that it is incorrect to call methods like next in the same statement as key/current because PHP
does not guarantee that it will evaluate all operands in a statement in a particular order (though
it virtually always does go left to right). That means with a line like
array('triggerNext' => $file->next(), 'line' => $file->key(),
'contents' => trim($file->current())),
you can't actually be sure what methods will be called in what order, and having next before or
after key/current makes a difference. The same problem exists with expressions like ($i++) + (++$i).
https://en.wikipedia.org/wiki/Sequence_point
------------------------------------------------------------------------
[2021-10-22 12:33:42] lucasfbustamante at gmail dot com
Is this expected? https://3v4l.org/s4ZKA
<?php
$file = new SplTempFileObject();
for ($i = 0; $i < 100; $i++) {
$file->fwrite("Foo $i\n");
}
$file->seek(50);
echo json_encode(array(
array('triggerNext' => $file->next(), 'line' => $file->key(),
'contents' => trim($file->current())),
array('triggerNext' => $file->next(), 'line' => $file->key(),
'contents' => trim($file->current())),
array('triggerNext' => $file->next(), 'line' => $file->key(),
'contents' => trim($file->current())),
), JSON_PRETTY_PRINT);
?>
Results:
PHP 8.0.1+
[
{
"line": 51,
"contents": "Foo 50"
},
{
"line": 52,
"contents": "Foo 51"
},
{
"line": 53,
"contents": "Foo 52"
}
]
PHP 5.1 - 8.0.0:
[
{
"line": 51,
"contents": "Foo 51"
},
{
"line": 52,
"contents": "Foo 52"
},
{
"line": 53,
"contents": "Foo 53"
}
]
------------------------------------------------------------------------
[2021-10-21 22:21:08] lucasfbustamante at gmail dot com
I see the consistency this bugfix aims to provide between rewind (or starting from
zero) and seek: https://3v4l.org/HBTeF
It still escapes me why "Line" is 0 at "Foo 1", but either way, it's
consistent with the old implementation so it won't cause bugs in my application.
I can replace all my usage of fgets() with current() and
next() instead, so that key() will remain consistent after
rewind() or seek() with all PHP versions: https://3v4l.org/WYrig
------------------------------------------------------------------------
The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at
https://bugs.php.net/bug.php?id=81551
--
Edit this bug report at https://bugs.php.net/bug.php?id=81551&edit=1