Allow installation on PHP 8. #46

ADmad · 2020-08-14T18:32:08Z

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	no
RFC	yes
QA	no

Description

composer.json

.travis.yml

ADmad · 2020-08-15T14:31:18Z

Related PRs for dependencies:

laminas/laminas-zendframework-bridge#69
php-http/psr7-integration-tests#45
http-interop/http-factory-tests#44

ADmad · 2020-08-19T07:09:54Z

@Ocramius I have bumped PHP constraint to 7.3+ as per your feedback. Temporarily using my fork to run the test suite until php-http/psr7-integration-tests has a new release.

The two test failures aren't directly related to the PHP version bump and probably need to be addressed separately.

The docblock of StreamFactoryInterface::createStreamFromFile() says @throws \RuntimeException If the file cannot be opened. and the test case correctly tests for that. But Laminas\Diactoros\StreamFactory::createStreamFromFile() always throws an instance of Laminas\Diactoros\Exception\InvalidArgumentException (which extends \InvalidArgumentException if it fails to create a stream.

ADmad · 2020-08-21T06:37:30Z

Regarding the failing tests this commit message has a quoted response for the PHP-FIG member: http-interop/http-factory-tests@c29f9c5.

It's seems throwing a RuntimeException instead of InvalidArgumentException exception when failing to create a stream is recommend. So maybe the exception thrown by diactoros could be changed for next minor release?

Another option would be to see if http-interop is willing to just widen the asserted exception to Exception instead of RuntimeException as done for other tests in the commit linked above.

ADmad · 2020-09-30T07:37:40Z

Can someone get the maintainers of php-http/psr7-integration-tests to merge or at least respond to php-http/psr7-integration-tests#45 so that we can make progress on this PR?

weierophinney · 2020-10-16T16:43:09Z

Can someone get the maintainers of php-http/psr7-integration-tests to merge

I've bumped via a comment; crossing fingers for now.

weierophinney · 2020-10-16T16:44:49Z

Regarding the failing tests this commit message has a quoted response for the PHP-FIG member: http-interop/http-factory-tests@c29f9c5.

It's seems throwing a RuntimeException instead of InvalidArgumentException exception when failing to create a stream is recommend. So maybe the exception thrown by diactoros could be changed for next minor release?

Another option would be to see if http-interop is willing to just widen the asserted exception to Exception instead of RuntimeException as done for other tests in the commit linked above.

Third option: we override that test case in our own test suite, to address the behavior.

ADmad · 2020-10-16T17:44:06Z

Third option: we override that test case in our own test suite, to address the behavior.

Not quite sure how to do that since http-factory-tests's test suite is directly run

laminas-diactoros/phpunit.xml.dist

Line 7 in e609ac2

<directory>./vendor/http-interop/http-factory-tests/test</directory>

ADmad · 2020-10-16T17:47:19Z

BTW should this PR now target 2.5.x branch? Given the min. PHP version bump I believe a new minor release would be warranted.

ADmad · 2020-10-16T17:48:45Z

BTW should this PR now target 2.5.x branch?

I took the liberty of doing so since the milestone for the PR was already changed to 2.5.0.

ADmad · 2020-10-16T17:51:04Z

I don't know your backward compatibility policy is but since these changes are going into a new minor release perhaps we can just update Stream::setStream() to throw RuntimeException instead of InvalidArgument exception to fix the testsuite failure.

ADmad · 2020-10-17T14:31:26Z

I updated to use the new php-http/psr7-integration-tests release. Tweaked Stream::isValidStreamResourceType() to account for GD changes in PHP 8.

andypost · 2020-10-17T14:36:30Z

Handy guide about changes https://php.watch/versions/8.0

ADmad · 2020-10-17T15:03:07Z

http-interop/http-factory-tests@c29f9c5

It's seems throwing a RuntimeException instead of InvalidArgumentException exception when failing to create a stream is recommend. So maybe the exception thrown by diactoros could be changed for next minor release?

My recommendation would be to do the above to fix the test failures. I can make required changes if I get confirmation.

Signed-off-by: ADmad <admad.coder@gmail.com>

GD extension uses \GdImage class objects instead of resource in PHP 8. Signed-off-by: ADmad <admad.coder@gmail.com>

See http-interop/http-factory-tests@c29f9c5 Signed-off-by: ADmad <admad.coder@gmail.com>

ADmad · 2020-11-02T09:05:34Z

Everyone is welcome to help!

@froschdesign I had been awaiting the Laminas team's response regarding how to deal with the RuntimeException vs InvalidArgumentException issue. With none forth coming I finally just went ahead and updated the code to throw RuntimeException.

andypost · 2020-11-02T15:01:29Z

Both PRs for dependencies are accepted but no releases yet

Bump up http-interop/http-factory-tests to v0.8. Signed-off-by: ADmad <admad.coder@gmail.com>

Signed-off-by: ADmad <admad.coder@gmail.com>

boesing · 2020-11-02T18:58:38Z

composer.json

@@ -28,7 +28,7 @@
        }
    },
    "require": {
-        "php": "^7.1",
+        "php": "^7.3 || ^8.0",


Please use ~8.0.0 as we internally decided to go that way for PHP 8.0.

@boesing is there a record of the discussion about using ~8.0.0? That means when 8.1 comes out any of your dependencies will require a new release of their laminas dependency for them to be installable even if there are no code changes required to the laminas library. This feels a step backwards. I think the approach recently taken by Symfony to change their constraints to >= is the way to go but I can understand libraries choosing to go with ^8.0 - but ~8.0.0 feels very restrictive.

That means when 8.1 comes out any of your dependencies will require a new release of their laminas dependency for them to be installable even if there are no code changes required to the laminas library.

This is intentional: PHP is too unstable to pin to a major release, and indeed, restricting is intentional. As for symfony, I can only say that jumping off the bridge because somebody told you to do it is not a wise approach to decision-making.

If all is good, next year, it is just a matter of bumping dependency ranges: until then, there is no guarantee that 8.1 will be compatible :-)

As for the meeting where this was decided, I think it was this one: https://github.com/laminas/technical-steering-committee/blob/main/meetings/minutes/2020-09-07-TSC-Minutes.md#hacktoberfest

Can't find the precise point in any meeting notes though - would need to check the #tsc-meeting channel on https://laminas.slack.com/

@Ocramius I'm not jumping off a bridge because somebody told me too. I'm being stopped sending out scouts across a bridge because someone is unnecessarily putting a stop sign in the way.

There are a number of reasons why I think constraining the maximum version of PHP a library can run on is a not healthy for the ecosystem. Here are a some:

Increases friction to early adopters from testing your code and creating PRs to fix it

Increases friction to dependencies testing against new PHP versions. They have to jump through hoops and if nothing is broken they then require a release from Laminas in order to run tests. It makes the ecosystem form a long chain and, if you are on the end of the chain, you have a lot waiting to do.

Something hand wavy about software freedoms, gophp5, and Postel's law. It just feels wrong.

Composer's PHP constraints don't really work this way anyway... if there's something like "platform": { "php": 7.4.99 } in the root composer.json then, even though I'm running PHP 8, composer will still get laminas dependencies and not inform me a project is incompatible.

I can understand limiting to major versions ie something like ^7.0 (although I disagree with it) but ~8.0.0 increases the friction for laminas's dependencies due to a vague "PHP is too unstable to pin to a major release". I've worked on making Drupal 7.0, 7.1, 7.2, 7.4 and 8.0 compatible. It's a very large codebase and has many dependencies. PHP is generally the least of the problems. The biggest problem in maintaining compatibility against multiple PHP versions at the moment is PHPUnit.

For example, any dependency of laminas-feed that wants to be compatible with PHP 8.0 is currently waiting on laminas/laminas-feed#28 - which is only about code quality - and the PHP 8.0 compatibility PR only stopped PHP from issuing a deprecation message (a good thing to be sure) BUT now in order to be PHP 8 compatible your dependencies have to upgrade from 2.12.x to 2.13.0 - when there is no new functionality. There's a similar story for laminas-escaper and laminas-diactoros.

The biggest problem in maintaining compatibility against multiple PHP versions at the moment is PHPUnit.

This! For e.g. the only reason PHP version contraint for this package needed to be updated to ^7.3 is because of PHPUnit.

I don't know in what utopia Sebastian lives where the whole PHP ecosystem gets updated as soon as active support for a PHP version ends.

@alexpott

For example, any dependency of laminas-feed that wants to be compatible with PHP 8.0 is currently waiting on laminas/laminas-feed#28 - which is only about code quality

Unfortunately this is not entirely correct. This pull request is completed but the commits needs a correct signed off by the creator of the pull request.
But I hope that we can solve the problem on laminas-feed quickly.

I've just found php-http/psr7-integration-tests#45 and I'm more than a bit bemused. But to find the laminas project asking for PHP 8 compatibility on project because it is holding back the ecosystem is a bit much. The points made are exactly the points I'm making about needing compatibility before a release in order to be able to test before. I quote...

bump

RC2 just dropped, so PHP 8 is reaching relative stability. A number of PSR-7 providers are trying to ensure they are ready (e.g., Diactoros, Slim), but we are all currently blocked until this is merged.

Diactoros has been sitting on approved merge requests for days, and similarly for Feeds (who's PHP 8 compatibility release is blocked on a new integration with Psalm...). This means that unless Drupal makes exceptions to its release management policies it'll be June 2021 before we have an officially PHP 8 compatible release.

Also given the time its taken to get 8.0.0 compatibility done setting your constraint to ~8.0.0 does not fill your dependencies with confidence that you'll 8.1.0 compatible releases in place in time for your dependencies to be 8.1.0 compatible.

These are all valid view points. However, there's an aspect that those consuming the library are missing from those maintaining the library: Using ^ constraints makes maintaining the library far harder.

How?

Because every time we get a new minor or major PHP version, we have to increase our test matrix. And every time we increase the test matrix, we generally have to revisit the PHPUnit versions we use and the syntax each uses to ensure that tests will actually run. This makes even simple contributions often more complex because new failures arise that were not present before; additionally, if the contributor does not have every version of PHP currently supported by the library on hand, they may not even know there are test failures until CI/CD runs... which, when the matrix gets large, take longer and longer to complete. And we all know that when we have to wait for results, we tend to get bored and move on.

Yes, in an ideal world, there would be no BC breaks or deprecations within a given minor release series, whether that's PHP or libraries you use. But in point of fact, these arise all the time. That's the origin of the "PHP is too unstable to pin to a major release'" statement. Every single minor release that comes out, we MUST do a cycle to test our libraries against it, because inevitably some new deprecation or change does lead to breakage, whether that's simply a test failing because of a new deprecation notice, or, worse, users identifying issues. When a user jumps to a new version and suddenly the library does not work, that immediately becomes our problem, but now there's urgency to it.

On top of that, there's the question of how long we support a given minor or major release of a library. If the library has versions 1.0, 1.1, 1.2, 2.0, 2.1, 3.0, and 3.1, and all of them have a minimum supported version of ^7.0, does that mean we have to provide bugfixes and security fixes to all of these branches? or just the latest minor releases of each major branch (e.g., 1.2, 2.1, and 3.1)? or current major and previous major?

By narrowing the window of PHP versions we support (via the ~ constraint, or bumping the minimum minor we support), we make the maintenance story far easier:

We provide security patches to branches where the minimum supported PHP version is still supported by community PHP.

We can have a smaller number of PHPUnit versions we have to be compatible with.

There's a smaller test matrix we need to fulfill.

When a new minor or major version of PHP is released, we create a new minor release that changes the PHP constraints, to signal that we have actually tested it on that version, versus assumed it would remain compatible.

we generally have to revisit the PHPUnit versions we use and the syntax each uses to ensure that tests will actually run.

Fortunately the world has changed... https://twitter.com/s_bergmann/status/1328197689029386240

Finally, Hallelujah!

boesing · 2020-11-02T20:58:00Z

src/Stream.php

@@ -347,7 +347,7 @@ private function setStream($stream, string $mode = 'r') : void
        }

        if ($error) {
-            throw new Exception\InvalidArgumentException('Invalid stream reference provided');
+            throw new Exception\RuntimeException('Invalid stream reference provided');


As this is a BC break, I would prefer having some kind of "feature toggle" which allows upstream consumers to "enable" the RuntimeException

We could mark that feature toggle method as deprecated aswell as stating that with the next major, we will completely switch to RuntimeException.

Thoughts @weierophinney?

I'd consider this a bugfix, but call it out in the "Changed" section of the changelog, and note the why of the change ("to comply with PSR-7 integration test suites")

src/Stream.php

Laminas team's opinion is: "PHP is too unstable to pin to a major release, and indeed, restricting is intentional". Signed-off-by: ADmad <admad.coder@gmail.com>

src/Stream.php

dereuromark · 2020-11-18T11:06:18Z

Projects and libraries need this PR to get merged in order to avoid any --ignore-platform-req hacks when finalizing PHP 8 compatibility.
Is there anything else needed to get this done at this point?

weierophinney · 2020-11-18T18:12:49Z

Projects and libraries need this PR to get merged in order to avoid any --ignore-platform-req hacks when finalizing PHP 8 compatibility.
Is there anything else needed to get this done at this point?

Reviewing today, and will likely get a release out.

weierophinney

🚢

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

GrahamCampbell · 2021-03-07T22:14:11Z

The upstream change is wrong. You need to catch the ValueError from fopen on PHP 8.

froschdesign · 2021-03-07T22:17:38Z

@GrahamCampbell
Please open a new issue report because this pull request is already closed. Thanks in advance! 👍

GrahamCampbell · 2021-03-07T22:20:04Z

A fix will be made to the upstream package, which will break this package's tests when it is tagged.

ADmad force-pushed the php-8 branch from adcf8ab to eb652fd Compare August 14, 2020 18:36

Ocramius reviewed Aug 14, 2020

View reviewed changes

composer.json Outdated Show resolved Hide resolved

Ocramius requested changes Aug 14, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

ADmad marked this pull request as draft August 18, 2020 19:43

ADmad force-pushed the php-8 branch from eb652fd to 524878d Compare August 18, 2020 19:43

michalbundyra added the Enhancement label Aug 25, 2020

michalbundyra added this to the 2.4.0 milestone Aug 25, 2020

weierophinney changed the base branch from master to 2.4.x September 2, 2020 13:46

weierophinney modified the milestones: 2.4.0, 2.5.0 Sep 2, 2020

ADmad changed the base branch from 2.4.x to 2.5.x October 16, 2020 17:48

ADmad force-pushed the php-8 branch 2 times, most recently from b1d8b60 to 244502c Compare October 17, 2020 13:59

edudobay mentioned this pull request Oct 18, 2020

PHP 8.0 support #54

Closed

6 tasks

boesing linked an issue Oct 20, 2020 that may be closed by this pull request

PHP 8.0 support #54

Closed

6 tasks

ADmad force-pushed the php-8 branch from e4d67e0 to 7656c15 Compare November 2, 2020 08:40

ADmad added 3 commits November 2, 2020 14:31

Bump up PHP version constraint.

219d9fe

Signed-off-by: ADmad <admad.coder@gmail.com>

Fix failure on PHP 8.

e104115

GD extension uses \GdImage class objects instead of resource in PHP 8. Signed-off-by: ADmad <admad.coder@gmail.com>

Stream now throws a RuntimeException.

b0b23c7

See http-interop/http-factory-tests@c29f9c5 Signed-off-by: ADmad <admad.coder@gmail.com>

ADmad force-pushed the php-8 branch from 3e508cf to 7b96a55 Compare November 2, 2020 09:01

ADmad marked this pull request as ready for review November 2, 2020 09:05

ADmad requested a review from Ocramius November 2, 2020 09:06

ADmad added 2 commits November 2, 2020 23:47

Fix test failure on PHP 8.

7b2e5f3

Bump up http-interop/http-factory-tests to v0.8. Signed-off-by: ADmad <admad.coder@gmail.com>

Fix coding standards issues.

4b21e54

Signed-off-by: ADmad <admad.coder@gmail.com>

ADmad force-pushed the php-8 branch from 7b96a55 to 4b21e54 Compare November 2, 2020 18:18

boesing requested changes Nov 2, 2020

View reviewed changes

Update PHP 8 version constraint.

0278d1c

Laminas team's opinion is: "PHP is too unstable to pin to a major release, and indeed, restricting is intentional". Signed-off-by: ADmad <admad.coder@gmail.com>

ADmad force-pushed the php-8 branch from 1d0fbf0 to 0278d1c Compare November 3, 2020 15:02

snapshotpl mentioned this pull request Nov 4, 2020

Allow install php8 php-middleware/request-id#14

Merged

boesing approved these changes Nov 5, 2020

View reviewed changes

src/Stream.php Show resolved Hide resolved

dragonmantank mentioned this pull request Nov 16, 2020

PHP 8 Support Nexmo/nexmo-laravel#51

Merged

boesing mentioned this pull request Nov 17, 2020

Stream is not fully compatible with GD resource #57

Closed

weierophinney approved these changes Nov 18, 2020

View reviewed changes

weierophinney added a commit that referenced this pull request Nov 18, 2020

docs: adds CHANGELOG entry for #46

a38f845

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

weierophinney merged commit 0278d1c into laminas:2.5.x Nov 18, 2020

ADmad deleted the php-8 branch November 18, 2020 18:37

jrbasso mentioned this pull request Mar 16, 2021

2.4.x - Allow installation on PHP 8 #60

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow installation on PHP 8. #46

Allow installation on PHP 8. #46

ADmad commented Aug 14, 2020

ADmad commented Aug 15, 2020

ADmad commented Aug 19, 2020 •

edited

Loading

ADmad commented Aug 21, 2020 •

edited

Loading

ADmad commented Sep 30, 2020

weierophinney commented Oct 16, 2020

weierophinney commented Oct 16, 2020

ADmad commented Oct 16, 2020

ADmad commented Oct 16, 2020

ADmad commented Oct 16, 2020

ADmad commented Oct 16, 2020

ADmad commented Oct 17, 2020

andypost commented Oct 17, 2020

ADmad commented Oct 17, 2020 •

edited

Loading

ADmad commented Nov 2, 2020 •

edited

Loading

andypost commented Nov 2, 2020

boesing Nov 2, 2020

alexpott Nov 3, 2020 •

edited

Loading

Ocramius Nov 3, 2020

alexpott Nov 4, 2020

ADmad Nov 4, 2020 •

edited

Loading

froschdesign Nov 5, 2020

alexpott Nov 18, 2020

weierophinney Nov 18, 2020

alexpott Nov 18, 2020

ADmad Nov 19, 2020 •

edited

Loading

boesing Nov 2, 2020

weierophinney Nov 18, 2020

dereuromark commented Nov 18, 2020

weierophinney commented Nov 18, 2020

weierophinney left a comment

GrahamCampbell commented Mar 7, 2021

froschdesign commented Mar 7, 2021

GrahamCampbell commented Mar 7, 2021

Allow installation on PHP 8. #46

Allow installation on PHP 8. #46

Conversation

ADmad commented Aug 14, 2020

Description

ADmad commented Aug 15, 2020

ADmad commented Aug 19, 2020 • edited Loading

ADmad commented Aug 21, 2020 • edited Loading

ADmad commented Sep 30, 2020

weierophinney commented Oct 16, 2020

weierophinney commented Oct 16, 2020

ADmad commented Oct 16, 2020

ADmad commented Oct 16, 2020

ADmad commented Oct 16, 2020

ADmad commented Oct 16, 2020

ADmad commented Oct 17, 2020

andypost commented Oct 17, 2020

ADmad commented Oct 17, 2020 • edited Loading

ADmad commented Nov 2, 2020 • edited Loading

andypost commented Nov 2, 2020

Choose a reason for hiding this comment

alexpott Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADmad Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADmad Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark commented Nov 18, 2020

weierophinney commented Nov 18, 2020

weierophinney left a comment

Choose a reason for hiding this comment

GrahamCampbell commented Mar 7, 2021

froschdesign commented Mar 7, 2021

GrahamCampbell commented Mar 7, 2021

ADmad commented Aug 19, 2020 •

edited

Loading

ADmad commented Aug 21, 2020 •

edited

Loading

ADmad commented Oct 17, 2020 •

edited

Loading

ADmad commented Nov 2, 2020 •

edited

Loading

alexpott Nov 3, 2020 •

edited

Loading

ADmad Nov 4, 2020 •

edited

Loading

ADmad Nov 19, 2020 •

edited

Loading