Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:116129
MIME-Version: 1.0
References: <CAJp_myX3J8gUb3885KAdEz+xokpZ_mz1Gs_DrN2Tp+JYfewcEw@mail.gmail.com>
In-Reply-To: <CAJp_myX3J8gUb3885KAdEz+xokpZ_mz1Gs_DrN2Tp+JYfewcEw@mail.gmail.com>
Date: Wed, 22 Sep 2021 15:01:21 +0100
Message-ID: <CAFPFaMJsLmDTbg+iQiTRNFup1tW=GY9MxLjUFayZbhbqc6qgZQ@mail.gmail.com>
To: "Matthew Weier O'Phinney" <mweierophinney@gmail.com>
Cc: PHP Developers Mailing List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000096645905cc95f392"
Subject: Re: [PHP-DEV] BC breaking changes in PHP 8.1
From: george.banyard@gmail.com ("G. P. B.")

--00000000000096645905cc95f392
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, 22 Sept 2021 at 14:30, Matthew Weier O'Phinney <
mweierophinney@gmail.com> wrote:

> Yesterday, I opened an issue regarding a change in the pgsql extension (
> https://bugs.php.net/bug.php?id=3D81464).
>
> PHP 8.0 introduced the concept of "resource objects". Where previously we
> would have resources, and use `get_resource_type()` when we needed to
> differentiate various resources, resource objects give us immutable objec=
ts
> instead that allow us to type hint. Personally, I think this is wonderful=
!
>
> The rollout for 8.0 was incomplete, however, and only touched on somethin=
g
> like 4-6 different resource types. Still, a good start.
>
> With PHP 8.1, we're seeing the addition of more of these, and it was
> encountering one of those changes that prompted the bug I previously link=
ed
> to.
>
> Here's the issue: while overall, I like the move to resource objects,
> introducing them in a MINOR release is hugely problematic.
>
> Previously, you would do constructs such as the following:
>
>     if (! is_resource($resource) || get_resource_type($resource) !=3D=3D
> $someSpecificType) {
>         // skip a test or raise an exception
>     }
>
> Resource objects, however:
>
> - Return `false` for `is_resource()` checks.
> - Raise a warning for `get_resource_type()` checks, and/or report the
> resource object class name =E2=80=94 which differs from the previous reso=
urce names
> in all cases.
>
> This means conditionals like the above BREAK. As a concrete example, I di=
d
> PHP 8.1 updates for laminas-db last week, and assumed our postgres
> integration tests were running when we finally had all tests passing
> successfully. However, what was really happening was that our test suite
> was testing with `is_resource()` and skipping tests if resources were not
> present. We shipped with broken pgsql support as a result, and it wasn't
> until test suites in other components started failing that we were able t=
o
> identify the issue.
>
> Further, the "fix" so that the code would work on both 8.1 AND versions
> prior to 8.1 meant complicating the conditional, adding a `! $resource
> instanceof \PgSql\Connection` into the mix. The code gets unwieldy very
> quickly, and having to do this to support a new minor version was
> irritating.
>
> When I opened the aforementioned bug report, it was immediately closed as
> "not an issue" with the explanation that it was "documented in UPGRADING"=
.
>
> This is not an acceptable explanation.
>
> - There was no RFC related to 8.1 indicating these changes were happening=
.
> (In fact, there was no RFC for resource objects in the first place =E2=80=
=94 which
> is concerning considering the BC implications!)
> - In semantic versioning, existing APIs MUST NOT change in a new minor
> version, only in new major versions.
>
> Reading the UPGRADING guide, there's a HUGE section of backwards
> incompatible changes for 8.1 =E2=80=94 THIRTY-FOUR of them. Nested in the=
se are
> notes of around a half-dozen extensions that once produced resources now
> producing resource objects.
>
> The pace of change in PHP is already breathtaking when you consider large
> projects (both OSS and in userland); keeping up with new features is in a=
nd
> of itself quite a challenge. Introducing BC breaks in minor versions make=
s
> things harder for everyone, as now you have to figure out not only if
> there's new features you want to adopt, but whether or not there are
> changes that will actively break your existing code. I strongly feel that
> anything in the backwards incompatible section of the UPGRADING guide
> should be deferred to 9.0, when people actually expect things to change.
>
> --
> Matthew Weier O'Phinney
> mweierophinney@gmail.com
> https://mwop.net/
> he/him
>

Resource to object conversions have precedent for not being in a major
version:
GMP in PHP 5.6
Hash in PHP 7.2

The fix to how to check these conditions is the same as last year and it is
to check against false, not is_resource nor an instance of the class.
It is true that if you ensure that the resource type is correct it gets
slightly unwieldy but:

if ($resource !=3D=3D false && ((is_resource($resource) &&
get_resource_type($resource) =3D=3D=3D $someSpecificType) || $resource inst=
anceof
ClassName) ) {
        // skip a test or raise an exception
}

Should be identical to the changes made to support PHP 5.6, 7.2, and 8.0 in
regards to the other conversions, and I don't see why it is now an issue.
We also *explicitly* hold off from pouring more of our time in resources to
object conversions during the Beta cycle of PHP 8.0 as we knew we could
defer this to a later minor version, compared to promotions from E_WARNING
to Exceptions which took a huge amount of last summer.

PHP tries to broadly follow semantic versioning but it *never* has adhered
to it strictly, and this is not uncommon within programming languages
nomenclature (e.g. Python).

Moreover, many changes to php-src are based, and a result of this
refactoring from resources to objects, so it isn't just a simple "revert
commit", especially as we are in the RC phase.

Albeit this is only my opinion, this is still not a bug, and I frankly
don't think any of it should be reverted as we, the core team, are never
going to be able to convert all of the resource to objects in time for a
major release, except if we decide that each year PHP is getting a major
release and we throw minor versions out of the windows.

Best regards,

George P. Banyard

--00000000000096645905cc95f392--