Bug #81551 [Com]: SplFileObject::fgets( does not advance line pointer after SplFileObject::seek()

Edit report at https://bugs.php.net/bug.php?id=81551&edit=1 ID: 81551 Comment by: lucasfbustamante at gmail dot com Reported by: lucasfbustamante at gmail dot com Summary: SplFileObject::fgets( does not advance line pointer after SplFileObject::seek() Status: Feedback 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: 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. Previous Comments: ------------------------------------------------------------------------ [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 ------------------------------------------------------------------------ [2021-10-21 21:45:34] lucasfbustamante at gmail dot com I understand what's happening now, and the reasoning behind the change. However, this makes it very difficult for distributed libraries to have a comprehensive PHP version constraint. If you use the SPLFileObject and depend on seek() and key(), you either have to pick PHP 8 or PHP 5~7, because they have the same API but operates differently. Taking the WordPress environment for instance (I am a WordPress plugin developer, my distributed library is a WordPress plugin), PHP 8.0 is used by only 1.5% of the websites to this date: https://wordpress.org/about/stats/ I dislike the fact that PHP broke backwards compatibility for a function that exists and returns this same result since PHP 5.3, especially considering that this was introduced in a minor patch version bump, so PHP 8.0.0 will operate in a substantially different way than, say, PHP 8.0.1 or 8.1. ------------------------------------------------------------------------ [2021-10-21 20:37:41] requinix@php.net Looks correct to me. Change the lines to get a better picture of what's happening. https://3v4l.org/33lsI key() returns the current line number, which should be the same at the "beginning" of the line (eg, after a seek or rewind) as it is at the "end" of the line (after an fgets). The line number increments according to iterator semantics, which say that the key/current doesn't change until a call to next() - or, in this case, if you try to continue reading beyond the current location using another call to fgets. Before the bug fix: - Rewinding returns key=0, fgets=line 0, key=0 - Seeking to 50 would return key=50, fgets=line 51, key=51 After the bug fix: - Rewinding returns key=0, fgets=line 0, key=0 - Seeking to 50 returns key=50, fgets=line 50, key=50 ------------------------------------------------------------------------ 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