fix: updates for PHP8 compatibility #27
Conversation
|
PHPUnit 8.5.13 supports PHP 8.0 now, I think it does not require to upgrade PHPUnit to 9.x version. After testing in local, I think it only needs to change "require": {
- "php": "^7.1"
+ "php": "^7.1|^8.0"
},
"require-dev": {
"phpunit/phpunit": "^8.5",
- "phpbench/phpbench": "^0.17",
+ "phpbench/phpbench": "^1.0.0-alpha3",
"symfony/phpunit-bridge": "^5.1"
} |
| @@ -22,11 +22,13 @@ | |||
| } | |||
| }, | |||
| "require": { | |||
| "php": "^7.1" | |||
| "php": ">=7.1" | |||
There was a problem hiding this comment.
This version is too loosely constrained. Can you make it "^7.2|^8.0" to make sure php 9.0 throws an error.
| }, | ||
| "require-dev": { | ||
| "phpunit/phpunit": "^8.5", |
There was a problem hiding this comment.
PHPUnit 8.5 currently supports both 7.2 and 8.0 so no need to update.
| } | ||
| }, | ||
| "minimum-stability": "dev", | ||
| "prefer-stable": true |
There was a problem hiding this comment.
Please remove both of these lines
| @@ -1,23 +1,16 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Please revert any changes to this file
| @@ -24,15 +24,15 @@ public function testGeneratesUppercaseIdentifierByDefault(): void | |||
| { | |||
| $ulid = Ulid::generate(); | |||
|
|
|||
| $this->assertRegExp('/[0-9][A-Z]/', (string) $ulid); | |||
There was a problem hiding this comment.
As we still use PHPUnit 8.5, these changes can be reverted
|
Thank you @andycowan for this pull request! And thanks @bepsvpt for doing an initial review by running it locally, this saves me a lot of time! @andycowan there are some minor changes though. If you could apply them, this PR is good to merge. |
|
Superseded by #28 |
Hi Robin
I've made some adjustments for PHP8 compatibility, and fixed some deprecations that arise from upgrading phpunit.
Some things, particularly what I've done with versions in the composer.json might not be the way you want to do it, so I suggest you have a look and see what you think.
Also, my changes aren't compatible with 7.1 and 7.2 - I haven't removed them from the version constraint - it depends whether you want to carry on supporting them or not...
A.