PHP 8 support #3015

edudobay · 2020-11-01T17:01:38Z

This PR allows Slim to be installed in PHP 8 and passes all the tests and checks.

History: I've raised some points in #2977 (comment) and since that time I've worked around other repositories to make this possible. This is also my first open-source contribution to a major project :)

Test fixes:

Added a test for an uncovered branch of MiddlewareDispatcher->addDeferred. PHPUnit 8 was miscalculating coverage – see https://coveralls.io/builds/33553304/source?filename=Slim/MiddlewareDispatcher.php.

My questions:

I implemented disableXmlEntityLoader as a private static method. Is this okay? I thought about making it protected or extracting it to a class, but I'm unsure about creating new "contracts".
Coverage – conditional call to libxml_disable_entity_loader. Is it okay to just @codeCoverageIgnore some part that will only run in PHP 7 or PHP 8? How should situations like this be handled?

Dependencies: I have gone through all dependencies that are blocking the PHP 8 upgrade and temporarily linked to development branches where the PHP 8 issues are solved. Surely this must be addressed before this can be merged (this is isolated in a single commit that can be removed). Those are the open PRs, some by me and some by others:

coveralls · 2020-11-01T17:03:41Z

Coverage remained the same at 100.0% when pulling ffd1196 on edudobay:php8 into 40c1fff on slimphp:4.x.

l0gicgate · 2020-11-10T22:46:34Z

@edudobay thank you for this PR

I implemented disableXmlEntityLoader as a private static method. Is this okay? I thought about making it protected or extracting it to a class, but I'm unsure about creating new "contracts".

I think that we could just make that method protected but does not need to be static unless I'm missing something

Coverage – conditional call to libxml_disable_entity_loader. Is it okay to just @codeCoverageIgnore some part that will only run in PHP 7 or PHP 8? How should situations like this be handled?

I don't think there's any way around this one in this particular case. We would have to overload the LIBXML_VERSION constant somehow to be able to stub the behavior.

Dependencies: I have gone through all dependencies that are blocking the PHP 8 upgrade and temporarily linked to development branches where the PHP 8 issues are solved. Surely this must be addressed before this can be merged (this is isolated in a single commit that can be removed). Those are the open PRs, some by me and some by others:

This particular issue bothers me a bit because we're going to have to do this work all over again when all the other libraries have official releases to support PHP 8. I would rather just do all the work at once than do it twice. We can just leave this PR open for now until the other libraries release official support for PHP 8.

edudobay · 2020-11-11T01:19:32Z

@l0gicgate Sure, I'll make it protected then. But an instance method would not work inside the static anonymous function. I suppose that function is static for performance reasons. When I was implementing these changes, I benchmarked several kinds of function calls and I got differences up to 10x in memory usage — even though I don't think this is performance-critical code, I would be careful not to introduce a 10x factor in resource usage. I'll redo these benchmarks (I think I threw it away at the time), but I'll be happy to discuss other solutions too.

Regarding dependencies, I'll just watch them for releases and ping a comment when they're all ready.

edudobay · 2020-11-27T01:10:34Z

Hi! I've updated the source branch and fixed some PHPStan issues that surfaced recently. I'm not sure if my solution was the way to go — the type hints that could go wrong are the ones for Closure::bindTo(), which can fail if called on a closure with static scope.

edudobay · 2020-11-27T10:52:37Z

Hi @l0gicgate. All set for external projects! When slimphp/Slim-Psr7#169 is merged and release tags are ready for it and for slim/http, we can remove the "TEMP" commit to go back to tagged release constraints.

I guess we will need to wait a few days for an official PHP 8.0 tag to be available in Travis CI, so we can fix that too, right? I'm not sure if we should otherwise remove "allowed to fail" from the nightly build.

l0gicgate · 2020-11-28T06:31:18Z

@edudobay

I guess we will need to wait a few days for an official PHP 8.0 tag to be available in Travis CI, so we can fix that too, right? I'm not sure if we should otherwise remove "allowed to fail" from the nightly build.

Once they release the official PHP 8.0 tag we can add it to the CI builds and leave the nightly build as "allowed to fail" as we will need it to examine the behavior of future versions on CI e.g: PHP 9 (long time from now)

I will try and release the other repos this weekend!

edudobay · 2020-11-28T12:46:47Z

@l0gicgate Great, let me know if I can help with anything!

l0gicgate · 2020-11-30T00:35:51Z

@edudobay see the releases below:

Slim-Psr7 1.3.0 Release
Slim-Http 1.2.0 Release

We should be good to go for this PR after!

edudobay · 2020-11-30T02:16:31Z

@l0gicgate I've updated composer.json to require these latest slim/http and slim/psr7 if that's ok.

l0gicgate · 2020-11-30T02:40:19Z

Slim/CallableResolver.php

            $callable = $callable->bindTo($this->container);
+            /** @var Closure $callable */


can we put the comment above the variable?

sure! Sorry I missed these little things :)

l0gicgate · 2020-11-30T02:41:12Z

Slim/Middleware/BodyParsingMiddleware.php

+
+    protected static function disableXmlEntityLoader(bool $disable): bool
+    {
+        if (\LIBXML_VERSION >= 20900) {


Let's add this to the imports. We don't do \ in the codebase

use LIBXML_VERSION;

l0gicgate · 2020-11-30T02:41:30Z

Slim/MiddlewareDispatcher.php

            $middleware = $middleware->bindTo($this->container);
+            /** @var Closure $middleware */


Let's put the comment above the variable

l0gicgate

Just a few little things then I'll merge

Changes: * Make the call to libxml_disable_entity_loader conditional. This function has been deprecated in PHP 8.0 and the new default is equivalent to calling that function with `true`. https://php.watch/versions/8.0/libxml_disable_entity_loader-deprecation * Add PHPUnit 9.3 dependency — that's the first 9.x release to support PHP 8. PHPUnit 8.x will not be upgraded to support PHP 8. TODO: * Some dependencies do not yet declare compatibility with PHP 8; this package can currently only be installed on PHP 8 using the Composer `--ignore-platform-reqs` flag. Apart from Slim dependencies, these are the packages which do not yet support a "php:^8.0" requirement: fig/http-message-util laminas/laminas-diactoros nyholm/psr7-server

The external package weirdan/prophecy-shim is required to support PHPUnit 8 and 9 simultaneously — we can't fully upgrade to PHPUnit 9 because it drops support for PHP 7.2, whereas PHPUnit 8 won't support PHP 8.

edudobay · 2020-11-30T03:07:48Z

@l0gicgate I've fixed the issues you mentioned!

l0gicgate

Thank you for this @edudobay!

edudobay force-pushed the php8 branch from b92654f to 99fc1e5 Compare November 1, 2020 17:10

edudobay mentioned this pull request Nov 1, 2020

PHP 8 support slimphp/Slim-Http#142

Merged

1 task

odan mentioned this pull request Nov 25, 2020

Declare compatibility with PHP 8.x #3025

Closed

edudobay force-pushed the php8 branch from 869349c to fee6a52 Compare November 27, 2020 01:03

edudobay force-pushed the php8 branch from fee6a52 to 054ae9d Compare November 27, 2020 10:44

edudobay changed the title ~~Draft: PHP 8 support~~ PHP 8 support Nov 27, 2020

edudobay force-pushed the php8 branch from 054ae9d to 9036a93 Compare November 28, 2020 12:49

l0gicgate added this to the 4.7.0 milestone Nov 30, 2020

l0gicgate added the Slim 4 label Nov 30, 2020

edudobay force-pushed the php8 branch from 9036a93 to 988917a Compare November 30, 2020 02:14

l0gicgate reviewed Nov 30, 2020

View reviewed changes

edudobay added 5 commits November 30, 2020 00:03

Shim/migrate PHPUnit deprecations

d971acb

The external package weirdan/prophecy-shim is required to support PHPUnit 8 and 9 simultaneously — we can't fully upgrade to PHPUnit 9 because it drops support for PHP 7.2, whereas PHPUnit 8 won't support PHP 8.

Add test for uncovered lines in MiddlewareDispatcher::addDeferred

919a3ce

Fix recent PHPStan issues for PHP 8

dfab0b0

Update slim/http and slim/psr7 requirements

ffd1196

edudobay force-pushed the php8 branch from 988917a to ffd1196 Compare November 30, 2020 03:05

l0gicgate approved these changes Nov 30, 2020

View reviewed changes

l0gicgate merged commit 8dbe684 into slimphp:4.x Nov 30, 2020

edudobay deleted the php8 branch November 30, 2020 11:34

l0gicgate mentioned this pull request Dec 1, 2020

4.7 - Release #3028

Merged

j0k3r mentioned this pull request Dec 1, 2020

weirdan/prophecy-shim must be a dev requirement #3029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 8 support #3015

PHP 8 support #3015

edudobay commented Nov 1, 2020 •

edited

Loading

coveralls commented Nov 1, 2020 •

edited

Loading

l0gicgate commented Nov 10, 2020

edudobay commented Nov 11, 2020

edudobay commented Nov 27, 2020

edudobay commented Nov 27, 2020 •

edited

Loading

l0gicgate commented Nov 28, 2020 •

edited

Loading

edudobay commented Nov 28, 2020

l0gicgate commented Nov 30, 2020

edudobay commented Nov 30, 2020

l0gicgate Nov 30, 2020

edudobay Nov 30, 2020

l0gicgate Nov 30, 2020

l0gicgate Nov 30, 2020

l0gicgate left a comment

edudobay commented Nov 30, 2020

l0gicgate left a comment

		$callable = $callable->bindTo($this->container);
		/** @var Closure $callable */

		$middleware = $middleware->bindTo($this->container);
		/** @var Closure $middleware */

PHP 8 support #3015

PHP 8 support #3015

Conversation

edudobay commented Nov 1, 2020 • edited Loading

coveralls commented Nov 1, 2020 • edited Loading

l0gicgate commented Nov 10, 2020

edudobay commented Nov 11, 2020

edudobay commented Nov 27, 2020

edudobay commented Nov 27, 2020 • edited Loading

l0gicgate commented Nov 28, 2020 • edited Loading

edudobay commented Nov 28, 2020

l0gicgate commented Nov 30, 2020

edudobay commented Nov 30, 2020

l0gicgate Nov 30, 2020

Choose a reason for hiding this comment

edudobay Nov 30, 2020

Choose a reason for hiding this comment

l0gicgate Nov 30, 2020

Choose a reason for hiding this comment

l0gicgate Nov 30, 2020

Choose a reason for hiding this comment

l0gicgate left a comment

Choose a reason for hiding this comment

edudobay commented Nov 30, 2020

l0gicgate left a comment

Choose a reason for hiding this comment

edudobay commented Nov 1, 2020 •

edited

Loading

coveralls commented Nov 1, 2020 •

edited

Loading

edudobay commented Nov 27, 2020 •

edited

Loading

l0gicgate commented Nov 28, 2020 •

edited

Loading