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

[10.x] Explicit nullable parameter declarations to fix PHP 8.4 deprecation #50921

Closed
wants to merge 2 commits into from

Conversation

Jubeki
Copy link
Contributor

@Jubeki Jubeki commented Apr 4, 2024

Closes #50803

Draft State

Let's run tests and get some opinions first, if this should be applied to Laravel 11 or Laravel 10. (I opted here for Laravel 10, not sure what this would imply when merging changes upstream)

Pint will be removed before the merge.

Description Taken from the provided issue:

As of PHP 8.4, implicitly nullable parameter declarations will be deprecated. See also https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated and https://wiki.php.net/rfc/deprecate-implicitly-nullable-types.

Currently, a large number of functions within this codebase still use this pattern; see also a quick example I made with rector. This should probably be adjusted.

Implementation suggestions:

  • for maintenance reasons, it might be most fruitful to target any update at 10.x, to also prevent lots of deprecations in the few months where v10 and PHP 8.4 are both supported;
  • StyleCI has a nullable_type_declarations fixer that can automatically fix and enforce this, which is probably the easiest way to automatically fix this;
  • maybe for other people using StyleCI, adding this to the laravel preset might be a good way forward?

Copy link

github-actions bot commented Apr 4, 2024

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@GrahamCampbell
Copy link
Member

Laravel 10 does not and will never support PHP 8.4, as far as I'm aware. This should target 11.x.

@GrahamCampbell
Copy link
Member

Moreover, pint should not be installed here. The framework uses StyleCI to enforce code style.

@Jubeki
Copy link
Contributor Author

Jubeki commented Apr 4, 2024

Pint will be uninstalled after reviewing the changes, I left it there for now.

I just wanted to tag you @GrahamCampbell to discuss these changes, because you maintain StyleCI I think.

And I will quote @driesvints from the issue: #50803 (comment)

It's highly unlikely we'll support PHP 8.4 on Laravel v10. But if it's not too much work and there aren't any breaking changes we can give it a go. I'll try to get a CI job up soon.

@crynobone
Copy link
Member

By the time PHP 8.4 is released, Laravel 10 will be under security releases only.

@driesvints
Copy link
Member

@Jubeki @crynobone makes a good remark about the security releases only support. It might be better in the end to send this to 11.x instead. I appreciate the effort! 👍

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

Successfully merging this pull request may close these issues.

4 participants