-
Notifications
You must be signed in to change notification settings - Fork 109
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
add php 8.4 support #110
base: master
Are you sure you want to change the base?
add php 8.4 support #110
Conversation
cc: @denisdulici |
Co-authored-by: Kehet <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a maintainer, but I am wondering if the changes that are in this PR is the minimal amount of changes needed to get the PHP 8.4 support we need/want.
Any chance we could get a maintainer to point this PR in the right direction so we can move this forward? 👍 Code looks solid to me.
<!-- <coverage>--> | ||
<!-- <include>--> | ||
<!-- <directory suffix=".php">src</directory>--> | ||
<!-- </include>--> | ||
<!-- <report>--> | ||
<!-- <html outputDirectory="build/coverage"/>--> | ||
<!-- <text outputFile="build/coverage.txt"/>--> | ||
<!-- <clover outputFile="build/logs/clover.xml"/>--> | ||
<!-- </report>--> | ||
<!-- </coverage>--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: What's the thought behind this coverage comment? Is this necessary for adding PHP 8.4 support? Why would this work on the other PHP versions 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the project is not using coverage in the workflows and throws some weird errors so I decided to disable these.
If we want to fix this, it will require extra work and maybe changing the minimum requirements for the project dependencies.
Yes, it is. |
hope it can be merged! |
No description provided.