Add support for Laravel 9.x #20
Conversation
composer.json
Outdated
| "orchestra/testbench": "^6.0.0" | ||
| "orchestra/testbench": "^6.0.0|^7.0.0" | ||
| }, |
There was a problem hiding this comment.
- "orchestra/testbench": "^6.0.0|^7.0.0"
+ "orchestra/testbench": "^7.0.0"Do this and it works.
There was a problem hiding this comment.
I'm against this change.
v7.x of testbench requires PHP ^8.0, so if we make that change this package would not support PHP < 8 anymore.
@gauravmak thoughts?
There was a problem hiding this comment.
It is a require-dev dependency. We will need PHP 8 during development to run tests. The package can still support older versions of PHP.
There was a problem hiding this comment.
I'm pretty sure the Github workflow will fail for PHP < 8 if we removed that. Should we also update the test to drop checks for PHP < 8?
There was a problem hiding this comment.
- "orchestra/testbench": "^6.15.1",
+ "orchestra/testbench": "^6.23|^7.0.0",You need to do this. Check it's working.
There was a problem hiding this comment.
^6.15.1 is working fine and all tests run successfully with --prefer-lowest flag.
Composer will decide which version of orchestra/testbench to install.
If --prefer-lowest flag is set it will install v6.0.0.
Otherwise if --prefer-stable is set (which is by default) it will install v6.24.1 (whatever the latest 6.x version is).
The change you are requesting doesn't have any impact at all, so I fail to understand the reason behind it.
I'm not being pushy, I just want to get some clarification on why we need to change it.
There was a problem hiding this comment.
As per your note on description Side note: I tried to get PHP 8.1 support as well, but the action is failing to run the test using --prefer-lowest option. The package is working as intended on 8.1, but the tests cases are failing. I think the tests need to be re-written to support PHPUnit ^9.5.
I tried and it's throwing an error so I suggest you do this.
There was a problem hiding this comment.
Oh, I see. That's more complicated than that though.
PHPUnit ^9.5 is needed for PHP 8.1, but the tests are not compatible with PHPUnit ^9.5. We'd have to re-write the tests to get support for 8.1 (and be backward compatible with 7.3). I don't have the capability to handle that right now.
There was a problem hiding this comment.
there is no issue with the PHPUnit versions because as per the https://github.com/sebastianbergmann/phpunit/blob/master/composer.json#L25 they did not need more than or equal PHP 8.1.
There is an issue is with the orchestra/testbench because one of the dependencies of this package is not updated so we need to update this package to the 6.23 version. after this, all the tests are passed.
There was a problem hiding this comment.
@hirenkeraliya you were right, setting testbench to at least 6.23 fixed the tests for PHP 8.1. Appreciate your input.
@gauravmak tests are passing on my branch. Can you start tests on this PR and merge it (if everything looks good)?
composer.json
Outdated
| "orchestra/testbench": "^6.0.0" | ||
| "orchestra/testbench": "^6.0.0|^7.0.0" | ||
| }, |
There was a problem hiding this comment.
there is no issue with the PHPUnit versions because as per the https://github.com/sebastianbergmann/phpunit/blob/master/composer.json#L25 they did not need more than or equal PHP 8.1.
There is an issue is with the orchestra/testbench because one of the dependencies of this package is not updated so we need to update this package to the 6.23 version. after this, all the tests are passed.
|
Done. Thank you for your contribution, Eduard. 🙂 |
@gauravmak I've made the changes to support the newest major version of Laravel. While running tests locally I was prompted to update
phpunit.xmlto the new standard format, so I did that too.Also updated the Action to include testing for PHP 8.1