Skip to content
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

Upgrade unit tests to work with php 8 #168

Merged
merged 4 commits into from Feb 5, 2023

Conversation

michaelw85
Copy link
Contributor

@michaelw85 michaelw85 commented Jan 30, 2023

This upgrade works with PHP 7.4, 8.0, 8.1 and 8.2

PHP unit versions + supported PHP version: https://phpunit.de/supported-versions.html

What has been changed:
\PHPUnit_Framework_TestCase Needs to be replaced with namespaced TestCase.
assertArraySubset has been removed see ticket, I've added a package re-adding it via trait.
prophesize is not included in phpunit anymore, as of phpunit 9 you will have to install it yourself and use a trait, which I did but this library supports PHP 7.3 as a minimum.
assertInternalType Is deprecated
expectedException Should be written with an assertion

@michaelw85
Copy link
Contributor Author

Scrutinizer fails on composer install since it's running PHP 7.2, for this branch it should at least run 7.4.
I would like to update the setting but I don't think the configuration is part of the project or is it @stevep ?

@michaelw85 michaelw85 mentioned this pull request Jan 30, 2023
@stevep
Copy link
Member

stevep commented Feb 5, 2023

@michaelw85 Thank you for your work on this. This is huge. I've added a scrutinizer config file to the repo, so that works now.

Trying to decide if the library should just require 7.4 going forward or not. And if so, it probably warrants a 2.0 version number since it's breaking backwards compatibility. I have similar thoughts on the #167 PR, with NamedBuilder being removed. I don't know why a user's or lib's code would be dependent on it directly, but I can't be sure that they aren't.

That being said I'll move this to master, as well as #167 to master, and think about the release number. Any thoughts on the matter?

Thanks again. (and sorry for the delay)

@stevep stevep merged commit ad15ebb into StoutLogic:master Feb 5, 2023
@michaelw85 michaelw85 deleted the php_8_unit_tests branch February 5, 2023 09:07
@michaelw85
Copy link
Contributor Author

@stevep Thank you for merging! I wasn't sure you would be happy with such a big change 😅
For PHP version support I would even consider dropping 7.4, officially it's been end of life since November. Even 8.0 only has 9 months left. I think PHP is pushing harder on updates. Also seeing the support on PHP Unit I think it's good to move in the same direction.

On the versioning, I think it makes sense to bump the major version number when dropping support for older PHP versions.
If you still want to support the older PHP versions you can branch, patch, and release version 1.x.y. I don't think you should but you can (better to have the ability than to get stuck in my opinion).

Technically I think a minor release would be enough, I don't think composer would update the package if your PHP version does not meet the new constraints. I think it always gets the latest possible version unless you start forcing or when it's a dependency of another package.

Hope this helps.

P.S I added some packages for unit testing to add support for things that were dropped. Do you want to keep it running with these packages or would you rather refactor the tests and remove the added packages? I'm referring to assertArraySubset and prophesize. For me, these packages feel like temporary bandaids.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants