SplFileObject: key() and current() unsinchronized after call to fgets()

[gabrielpl]

Description

The following code:

<?php

declare(strict_types=1);

$file = new SplTempFileObject();

// write to file
for ($i = 0; $i < 5; $i++) {
    $file->fwrite("line {$i}" . PHP_EOL);
}

$file->rewind();

// read first line
$file->fgets();

// where am I?
echo $file->key(), ': ', $file->current();

Resulted in this output:

1: line 0

But I expected this output instead:

1: line 1

This bug was introduced after fixing #8273

PHP Version

PHP 8.1.6

Operating System

Windows 11

[Girgias]

I'll look into this when I can after the 26th of May if no one beats me to it.

[Girgias]

I think the fix for #8273 is wrong. Frankly the first call to any method which requires a line have been read to be meaningful not throw when none has been read is a mistake, as it is meaningless. But fixing that now would be way too painful.

[rene-hermenau]

I want to emphasize that we are messing around with a part of php that is essential for every software that process large rows of data. No software developer can rely on the consistency of the result of SplFileObject() API; if its behavior changes with every release.

When we don't bring consistency into this and make sure that it doesn't change again with every release it will bring us all into real troubles! This is serious.

Just a quick test of the code above:

7.x - 8.0.19 Broken but at least consistent in all releases
8.0.20 - 8.0.23 Working (Why such a fundamental change that is not backward compatible in a PATCH? Semver exists for a reason)
8.1.0 - 8.1.5 Broken
8.1.6 - 8.1.10 Working

It reminds me on this bug that is somehow similar https://bugs.php.net/bug.php?id=81551 where we "fixed" something in a PATCH that shouldn't be fixed there.

Please add a test to make sure this behavior will stay consistent in the future so that we can rely on it and do not need to be concernend it will break our applications again.

If we make changes to an API, the changes should not be done in a MINOR nor PATCH when they are not backward compatible.

@Luc45 Watch out. The SplFileObject has been touched again a while ago and leads to further inconsistencies.

[iluuu1994]

If we make changes to an API, the changes should not be done in a MINOR nor PATCH when they are not backward compatible.

In other words, let's create a new extension for every bug fix 👻

Most bug fixes are technically breaking. I can see you got bitten by this and that sucks but waiting 5 years for a simple bug fix is simply not realistic.

[rene-hermenau]

for a simple bug fix is

This breaks ABI and is far away from a simple bug.
This changes the behavior of the standard PHP libray and if it is easy it would have been fixed already without poping up again.

Complex issues, like this one here will be "fixed" and will break again because they are tight coupled with other dependencies or change well documented behavior.

Real example: We cannot use the SplFileObject consistent anymore across PHP version because this class behaves in multiple PHP versions completely differently. Imagine you do heavy complex processing on huge millions of rows in a file. It needs to be safe that line x will always be line x and not the result of x-1 suddenly due to a minor update.

If you have an application that runs on 150.000 hosts where you can not control the PHP version it is even much more challenging and we are nearing a point where we consider to drop the SPL and build our own for the sake of loosing all the performance gains SPL* brings to the table.

Hosting providers update their systems to a new minor version faster and more often or have internal requirement to do so immediately when there is a new minor version because a minor version usually does not break fundamentals.

If it is a simple fix, fix it, but if a fix breaks the behavior of a standard library there should be a very good reason to do so and when you do, stick to the rules and don't do cowboy coding:

• MAJOR version when you make incompatible API changes
• MINOR version when you add functionality in a backwards compatible manner
• PATCH version when you make backwards compatible bug fixes

If the behavior of a class is changed, it's also recommended to document it properly.

These are simple rules that should be always applied and only ignored for a good reason.

Sometimes it is necessary to remind ourselves of the basics.

In other words, let's create a new extension for every bug fix 👻

If this isn't such a serious problem that gets worse with every touch of SPL, I would laugh.

[Luc45]

Yeah, the fundamental problem that Rene brings to the table is that we, as WordPress plugin developers, ship code on our client's servers, which is infrastructure we don't control, so it has to run on multiple PHP versions. We write code that is compatible with PHP 5.6 to 8.2. When the same function produces different results depending on the PHP version, that makes our job really hard.

It would be ideal if these bug fixes that change behavior were to be introduced in a new API. Or at least, as part of a major update.

[iluuu1994]

but if a fix breaks the behavior of a standard library there should be a very good reason to do so

Hyrum's Law:

With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody.

In principle I agree with everything you say. Unfortunately, SPL is a mess and it's almost impossible to fix anything without unwanted side-effects. I haven't looked at the issue here in detail but I'd just like to say that we're trying to do our best to keep everyone happy. Closing bugs as WONTFIX will piss people off, so will fixing them while slightly altering behavior. It might very well be the case that this particular bug fix did more harm than good but that must be discussed on a case-by-case basis without discouraging any and all changes. And it actually looks like this is a new bug introduced by the bug fix and the changes were not intentional (@Girgias correct me if I'm wrong). That happens, we're human.

It would be ideal if these bug fixes that change behavior were to be introduced in a new API. Or at least, as part of a major update.

We postpone a lot of our bug fixes to the next minor version if we think the impact might be too big. We don't have a branch for the next major version as that release can be many years in the future and keeping such a branch up-to-date will mean a lot of additional work. Creating a new API per change for SPL would be wild, given the amount of bugs and dubious behaviors.

@cmb69 I think it could make sense to be a bit more strict with bug fixes in patches. E.g. if the behavior has been broken for some number of years (as was the case in GH-8273) it might be better to postpone the fix at least to the next minor version. WDYT?