-
Notifications
You must be signed in to change notification settings - Fork 5
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.1 Support #32
PHP 8.1 Support #32
Conversation
b0dde25
to
1c59623
Compare
@bkintanar under /.github/workflows/run_tests.yml can you add php 8.1 to the github actions test? so we can ensure everything checks out. Change this: To this: |
Signed-off-by: Bertrand Kintanar <bertrand.kintanar@gmail.com>
@foresight-developers done. |
@foresight-developers i see warnings. but the pipeline is failing. I'm not sure why. |
@bkintanar we have it set where even warnings cause it to fail. We need to look into why we are getting those warnings. Depending on that answer we may add a comment to have the test ignore them. I think you are still fine to do the migration of the xml file to the new format, as that isnt the issue I believe. |
a02e3d8
to
84f9c38
Compare
@foresight-developers ok. I've restored the updated phpunit.xml |
@bkintanar we are looking into this issue over the next few days, unfortunately we cant dedicate someone to this full time currently, but we will let you know if we figure anything out. Again many thanks for supporting this project! |
@bkintanar we have yet to allocate much time to this however it is on our development road map within the next several months |
@bkintanar we will be looking at this in the coming weeks but if you want to try the following: "You can rename the trait from NodeModelExtensionsTest to NodeModelExtensionsTestTrait. Starting with PHPUnit 10, files in your configured test directory that end with a configured suffix (see testsuites>testsuite>directory) in phpunit.xml must be classes extending PHPUnit\Framework\TestCase. I couldn't find an explanation for why it was changed unfortunately. A related issue on GitHub for some context: sebastianbergmann/phpunit#4979." |
@bkintanar looks like that has done the trick, the warning is not an issue so we will have to put something in place to ignore it, most likely in the github actions |
@bkintanar in the phpunit.xml if we remove the lines that auto generate the report coverage.txt then the warning most likely will go away, we dont really need to run a coverage report everytime we run phpunit anyways. |
@bkintanar do you need assistance with that phpunit.xml edit? thanks |
@foresight-developers updated. sorry to take so long. can this be merged now? |
@bkintanar we are reviewing the code right now, if everything checks out we can merge. |
Thanks! Can we tag a release version too please? |
@bkintanar I need to add to the readme the newly supported php version etc, this also supports laravel 10 right? so #31 can be closed? |
@bkintanar Thanks for all the help here it is: https://github.com/TopOnePercent/baum/releases/tag/3.1.0 We're based in the U.K, but I will be back online 6 hours if you need anything else. |
Fixes: #25