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: rene at wp-staging 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: 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. Previous Comments: ------------------------------------------------------------------------ [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 ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ 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