Improve PHP 8.4+ support by avoiding implicitly nullable type declarations #260
Conversation
Fixes all issues that emits a deprecation notice on PHP 8.4. See: - [RFC](https://wiki.php.net/rfc/deprecate-implicitly-nullable-types) - [PHP 8.4: Implicitly nullable parameter declarations deprecated](https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated)
SimonFrings
changed the title
SimonFrings
changed the title
There was a problem hiding this comment.
@Ayesh Thank you for filing the original PR and thanks to @SimonFrings for picking this up again. Changes LGTM, so let's get this shipped in time for PHP 8.4! 
-
I can confirm this avoids any deprecation warnings with PHP 8.4+ regarding implicitly nullable type declarations with Promise v3. I agree that we should verify this as part of our test suite once PHP 8.4 reaches feature freeze as this may require additional changes unrelated to this PR.
-
I've also looked into how this affects the older Promise v2 and v1 releases and can confirm that they will report deprecation warnings on PHP 8.4+ regarding implicitly nullable type declarations. Porting these changes to the older Promise versions would incur a BC break to change the
PromiseInterface(Enforce return type hints on all functions and require PHP 7.1+ as a consequence聽#149), so it looks like there's not much we can do for these legacy versions. Promise v3 would be the way to go in either case. -
Additionally, I've checked how similar changes will be required for our other components. I can confirm this affects Stream, DNS, Socket, HTTP, Async v4, ChildProcess, PromiseTimer, and have prepared matching PRs already. We're currently in the process of working on ReactPHP v3, so we will very likely work on the v3 branches first, upgrade the PHP requirements and then apply any PHP 8.4+ changes like suggested in this PR. Once this is done, we will likely backport some of these changes to our v1 components to ease upgrading.
As you can see, this will still involve quite some work across all our components. Thank you for sparking this discussion and filing these PRs to kill this off! I'll make sure to follow up on this and will link any PRs here. If you want to help us work on this, consider supporting this project, for example by becoming a sponsor 鉂わ笍
There was a problem hiding this comment.
Good to see this land, in all honestly we could/should have reopened the original PR and merge that instead. But given this is literally the same commit credits still go to @Ayesh on that level.
@WyriHaximus Tried that, but couldn't reopen the original one because the fork of this project had already been removed. |
@SimonFrings Check, thank you for the clarification 馃憤 |
|
@WyriHaximus no stress here but just as a reminder because this repo is otherwise fairly stable, please don't forget to tag a release ;) |
@Seldaek Give it a week or two to check this across our other packages, but don't worry, we're definitely planning to release this way ahead of time for PHP 8.4. |
This changeset improves PHP 8.4+ support by avoiding implicitly nullable types as discussed in reactphp/promise#260. I'm planning to add native types to the public API and introduce PHPStan in follow-up PRs. Once merged, we should apply similar changes to all our upcoming v3 components. On top of this, we should backport similar changes to the v1 branch. Builds on top of reactphp#182, reactphp#222 and reactphp/promise#260
This changeset backports reactphp#223 from `3.x` to `1.x` to improve PHP 8.4+ support by avoiding implicitly nullable types as discussed in reactphp/promise#260. The same idea applies, but v1 requires manual type checks to support legacy PHP versions as the nullable type syntax requires PHP 7.1+ otherwise. Builds on top of reactphp#223, reactphp#204 and reactphp#218
This changeset backports reactphp#223 from `3.x` to `1.x` to improve PHP 8.4+ support by avoiding implicitly nullable types as discussed in reactphp/promise#260. The same idea applies, but v1 requires manual type checks to support legacy PHP versions as the nullable type syntax requires PHP 7.1+ otherwise. Builds on top of reactphp#223, reactphp#204 and reactphp#218
|
@Seldaek We just released the new v3.2.0 of our Promise component containing the improved PHP 8.4 support 馃憤 |
|
Cool thanks, updated the composer snapshot to use it now 馃憤馃徎 One step closer to getting rid of all 8.4 warnings :) |
This changeset backports reactphp#223 from `3.x` to `1.x` to improve PHP 8.4+ support by avoiding implicitly nullable types as discussed in reactphp/promise#260. The same idea applies, but v1 requires manual type checks to support legacy PHP versions as the nullable type syntax requires PHP 7.1+ otherwise. Builds on top of reactphp#223, reactphp#204 and reactphp#218
This changeset improves PHP 8.4+ support by avoiding implicitly nullable types as discussed in reactphp/promise#260. I'm planning to add native types to the public API and introduce PHPStan in follow-up PRs. Once merged, we should apply similar changes to all our upcoming v3 components. On top of this, we should backport similar changes to the v1 branch. Builds on top of reactphp#182, reactphp#222 and reactphp/promise#260
This pull request is a continuation of #258 & #259, all credit goes @Ayesh for originally bringing this up and filing a fix for this in #258 馃憤
In short, PHP 8.4 deprecates implicitly nullable parameter types, which this PR fixes and thus avoids the deprecation notices.
This has originally been brought up by @Ayesh in #258 and I argued that we should also add PHP 8.4 to our test matrix, which then led to #259 being brought up. I now have a different view on this matter: PHP 8.4 is still in development and if we start running our tests on this version, there's a good chance a new PHP 8.4 feature release will break something and make our complete test suite fail. There's actually a recent case where this happened, because the stack trace changed in PHP 8.4, which led to failures in some of our
.phpttest files.To move forward I suggest to go the original way that @Ayesh proposed and start supporting PHP 8.4 features if we have a good reason to do so (see #258 (comment)). Once PHP 8.4 has its fixed set of changes, we can start adding this to our test matrix and ensure we're compatible with every new addition of this PHP version.
Thanks for everyone who contributed to the discussion in #258 & #259, I think this way makes the most sense for now 馃憤
Builds on top of #258, #259 and others
Closes #259