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

[v4] PHP 8.4 support #6811

Draft
wants to merge 7 commits into
base: develop-minor
Choose a base branch
from
Draft

[v4] PHP 8.4 support #6811

wants to merge 7 commits into from

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Nov 23, 2024

  • Stops using PHP 8.1 for CI backend tests

Changelog

Enhancements

  • Support for PHP 8.4

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add changes & docs to release notes draft in Notion

@distantnative distantnative self-assigned this Nov 23, 2024
Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

I think most of the CS fixes will create merge conflicts with v5. E.g. in v5 we have replaced the something|null $param = null params with something|null $param and made some fixes to docblocks where CS fixer now again wreaks havoc.

If we merge this as is to ensure PHP 8.4 compatibility in v4, we should probably not continue to merge back v4 changes into the v5 branch, otherwise it will be a merge conflict hell. Alternative could be to backport the CS changes manually from v5.

@distantnative
Copy link
Member Author

distantnative commented Nov 24, 2024

@lukasbestle We made nullable parameters non-optional?

This got created as Basti mentioned the user demand for 8.4 support in v4 as well. I think we'll need to tackle the merge conflict once. I think it'll be not too bad (as we've merged regularly, merging into v5/develop after merging this PR we can go with the v5/develop version in pretty much all conflicts). But while we probably won't have a lot more to merge from v4 to v5 I wouldn't want to rule it out in general.

@lukasbestle
Copy link
Member

lukasbestle commented Nov 24, 2024

Not in general, but in some Str methods (breaking change) and also in those cases where the parameter is not in the last position, which are those lines changed by this PR. Those parameters are never optional, which is the reason why PHP 8.4 deprecates the type $param = null syntax in favor or type|null. We had discussed that in #6398 and implemented it with #6401.

Looking back, it would have been better to make those changes in v4 and not v5. But now it's like it is.

@distantnative
Copy link
Member Author

If we merge develop-minor into v5/develop right before and right after merging this PR and then go with the v5/develop version for any merge conflict, we should be fine, in terms of merge conflicts.

In terms of making those changes for v4 as well of course not. But maybe that's ok that v4 doesn't get those enhancements but just most basic 8.4 support.

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.

3 participants