support PHPUnit 10, PHP 8.1+, cleanup #48
Conversation
codemasher
force-pushed
the
php-8.1-cleanup
branch
from
3ab32fc
to
73ee1fd
Compare
test/RequestFactoryTest.php
Outdated
| public function setUp():void{ | ||
|
|
||
| if(!defined('REQUEST_FACTORY') || !class_exists(REQUEST_FACTORY)){ | ||
| static::markTestSkipped('REQUEST_FACTORY class name not provided'); | ||
| } | ||
|
|
||
| if(!defined('URI_FACTORY') || !class_exists(URI_FACTORY)){ | ||
| static::markTestSkipped('URI_FACTORY class name not provided'); | ||
| } | ||
|
|
||
| $this->requestFactory = new (REQUEST_FACTORY); | ||
| $this->uriFactory = new (URI_FACTORY); |
There was a problem hiding this comment.
This change is out of scope with supporting PHPUnit 10. Please revert it.
There was a problem hiding this comment.
The scope of this change is given in the OP.
There was a problem hiding this comment.
random abstract classes and traits
These are not "random", they are design decisions that have downstream impacts. Changing this package's API has consequences.
There was a problem hiding this comment.
Ok, so I checked the most popular dependents for this library and all except one are using it as a stand-alone test suite without any changes. The Slim framework is the only one that extends the test classes and because of the "design decision" it has to unnecessarily re-implement several methods that are only found in the finalized classes here. Since you want to increment the major version you should take the opportunity to amend bad design decisions. What i can do is to rename the classes back to "*TestCase" so that libraries that extend them won't break - so even with out changes after an upgrade, their tests will still run and the old methods are ignored.
There was a problem hiding this comment.
There are two separate concerns that need to be separated:
- Support PHPUnit 10 #47 wasn't a complete switch to PHPUnit 10. I merged it and reduced the scope to be PHP 8.0+ and PHPUnit 10.0+ and released it as v2.0.0. Obviously that was not quite sufficient, given that PHPUnit 10 requires PHP 8.1. So one change should be just finalizing the PHPUnit 10.x upgrade, with as few code changes as possible, so that we can release v2.0.1.
- This PR shows that we could make some code/API changes that would improve the test cases, and I am absolutely for that. But such changes would be represent version 3.x, because this package follows semantic versioning. Those changes need to be a separate PR that downstream users can review and have input on.
I am happy to review and merge fixes for both (1) and (2) but they need to be separate PRs.
test/ResponseFactoryTest.php
Outdated
| if(!defined('RESPONSE_FACTORY') || !class_exists(RESPONSE_FACTORY)){ | ||
| static::markTestSkipped('RESPONSE_FACTORY class name not provided'); | ||
| } | ||
|
|
||
| $this->responseFactory = new (RESPONSE_FACTORY); |
There was a problem hiding this comment.
This change is out of scope. Please revert it. Same goes for the rest of this PR.
The only changes should be to composer.json and anything that is necessary for PHPUnit to not give warnings about deprecations.
codemasher
force-pushed
the
php-8.1-cleanup
branch
from
73ee1fd
to
9f4701b
Compare
|
Ok, I've changed the PR to the requested changes that should have been discussed and made in #47:
If you don't mind I'll add another commit with the changes that were supposed to be in this PR, aka, API changes. |
As a separate PR please. |
|
Good. Better tag it right now because there are already people waiting for it. |
|
A small oversight on my end, i forgot a |
fixed some oversights from #48
So that's the cleanup PR mentioned in #47. It removes the random abstract classes and traits, updates to some modern PHPUnit features (e.g. data provider calls via attributes) and generally some "modern" PHP features such as typed properties.