Skip to content
New issue

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

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

Already on GitHub? # to your account

[PHP 8.4] Fixes for implicit nullability deprecation #16

Closed
wants to merge 1 commit into from

Conversation

Ayesh
Copy link

@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)
Comment on lines +31 to +32
?string $clientFilename = null,
?string $clientMediaType = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This syntax is supported as of PHP 7.1.0, and predates generalized union types support.

As this package supports PHP 7.0, we cannot use nullable types without other changes.

Additionally, such changes MUST be targeted to the PHP-FIG specification before being targeted to the implementation. cc @mbniebergall @Jean85 @weierophinney

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick review. You are right, we can't make these changes without bumping the minimum PHP version to 7.1+.

As far as specification goes, I think this is merely an implementation detail. Both string $clientFilename = null and ?string $clientFilename = null are effectively identical for LSP and Reflection APIs, so changing this should have no impact on PSR implementations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shadowhand int $paramName = null is semantically equivalent to ?int $paramName, and it's been that way since parameter type hints were introduced; PHP just didn't formalize the concept of nullable types until 7.1.

I think at this point, it's fine to bump to 7.1. When you look at Packagist stats, across all packages, 7.0 only has a 1.3% usage... and when you look at the http-factory stats... it has 0% usage. I think we can safely bump.

Copy link
Contributor

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my note, I think it's safe to do this, but we also need to bump the minimum supported PHP version to 7.1 as part of this request, and it should target a new minor.

I also don't think we need to change the spec, as the change doesn't introduce any change in behavior, nor, when we target 7.1, does it change the signature in any way that is backwards incompatible.

Copy link
Member

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @MWOP considerations, and our evolution bylaw does exactly these considerations and requirements. The same bylaw, though, requires the equivalent of an errata vote (and a spec+meta adaptation) to approve this new minor version; I suggest that we proceed with that, even if we can expect an easy, unanimous approval.

@nicolas-grekas
Copy link

nicolas-grekas commented Apr 11, 2024

@Ayesh let's change the branch-alias in composer.json and bump to PHP >= 7.1
What's the status of this PR, FIG-wise? This is making the Symfony CI fail on PHP 8.4 so it'd be great fixing ASAP.

@Jean85
Copy link
Member

Jean85 commented Apr 11, 2024

What's the status of this PR, FIG-wise?

We need to follow the PSR evolution bylaw, which means:

  • fixing this PR:
    • bumping to PHP 7.1+ is required (to make it work correctly without BCs)
    • it's also allowed, since we did the same i.e. with PSR-7 from 1.0 to 1.1
  • we need also a PR on the PSR meta document to note the changes, as required in the bylaw
  • when that's ready, the editor @shadowhand can call for a vote to approve the two PRs
  • when that's done, we can merge both PRs, tag & release

@shadowhand
Copy link
Contributor

Superseded by #17

@shadowhand shadowhand closed this Apr 15, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants