Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PHP 8.4] Fixes for implicit nullability deprecation #258

Closed
wants to merge 1 commit into from
Closed

[PHP 8.4] Fixes for implicit nullability deprecation #258

wants to merge 1 commit into from

Conversation

Ayesh
Copy link
Contributor

@Ayesh Ayesh commented Mar 15, 2024

Fixes all issues that emits a deprecation notice on PHP 8.4.

See:

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
Copy link
Member

SimonFrings commented Mar 18, 2024

Hey @Ayesh, thanks for bringing this up, always happy about contributions 👍

I really like your changes and would love to see ReactPHP being compatible with PHP 8.4 before it comes out! The release of PHP 8.4 is set for November 2024, which is over half a year in the future. Thus the deprecation for implicit nullability might not be the only thing that will affect our projects when it comes to PHP 8.4 compatibility, so maybe it makes sense to wait before the PHP developer team finalizes their plans.

This won't be a problem for ReactPHP v3, as we support PHP >= 7.1, but for v1 (or v2 here in promise) this is going to be interesting. We support PHP >= 5.4 and since PHP 7.1 added support for nullable types, we can't use the ?TYPE syntax for older PHP versions, see https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated for more information. Not sure yet how we can achieve compatibility with PHP 8.4 in v1.

(Edit: I just noticed that you're probably the author of this PHP.Watch article, so you'll most definitely know what it's about 😄)

All in all I really appreciate the time and effort looking into this, but in my opinion we should wait until we have the first PHP 8.4 development version ready to run our test suite against and confirm our changes working. I would close this for now, but let's revisit this once we know what PHP 8.4 is going to look like.

@Ayesh
Copy link
Contributor Author

Ayesh commented Mar 18, 2024

This makes sense, thank you for your time reviewing this @SimonFrings. Hopefully we can revisit this and other potential changes in reactphp/* packages once it reaches its Feature Freeze.

@Seldaek
Copy link

Seldaek commented Mar 18, 2024

@SimonFrings I think it'd be nice if you can patch v3 still before 8.4 is further because composer bundles react/promise and that means every 8.4 build prints a bunch of deprecation warnings whenever a composer command runs. This is quite noisy and because of that I'm in the process of cleaning up all these deprecation warnings in composer itself.

It's not urgent by any means but IMO harmless to do it in v3, and most likely as you said you can't (and I'd argue you shouldn't bother as it's a mere deprecation) fix it in v1/v2.

@SimonFrings
Copy link
Member

@Seldaek Fair point, also talked with @clue about this and I can see that this should be handled upstream in our component. There already is a new pull request for this in #259 which builds on top of this one and adds PHP 8.4 to our test matrix, so looks good to me. Also Composer is a really impressive project (I use it everyday) and we're more than happy to help easing your development 👍

We'll probably release this officially in a v3.2.0, so until then you can install the development version (or the specific merge commit) to use this in Composer.

Still have to look what this means for Promise v1 & v2, but I think this is a topic for another day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants