Simply007
commented
2 years ago Hello @jankonas, thanks for your interest.
This is one of the best issue descriptions I have seen for a long time 🚀
Old PHP versions dropping
Our practice for this repo (and most of the other OS projects) is to support only the latest version of the project.
I definitely agree to upgrade to PHP 8 since PHP 7.4 is in the security period (similarly we have done in #13. #79)! So you are more than welcome to a pull request. I agree to migrate to v 8 first before taking a step further. I would go the "PHP 8.0 + PHPUnit 9" way + dropping also 7.3 from tests and running them on PHP 8.0 for now (and possibly adding 8.1 in separate PR).
I know we had some issues with HTML parser library (used for rich text resolution) (#60, https://github.com/Kub-AT/php-simple-html-dom-parser/issues/5, #79) -> this might be something we can then adjust based on the upgrade. -> something to test on review.
According to the dev dependencies
codeclimate/php-test-reporter
This is a leftover from migration to codecov so this one could be definitely removed.
phpdocumentor/phpdocumentor
We had an automatic step for generating API references when a new version was published. Currently, the API reference could be generated on demand so this dev dependency is also deprecated since the wiki page suggests installing this package globally. According to the upgrade - I hope this upgrade does not break this possibility, but this is something to be done on review.
Motivation
PHP 7.4 (the last version in PHP 7 family) is no longer actively supported and it's security support ends this November so it would be nice if this SDK supported newer PHP versions.
Proposed solution
From running tests it looks like the SDK already supports PHP 8.0 and with some deprecation warnings even 8.1. So it should be pretty straightforward to add PHP 8 to
composer.jsonrequirements. I've taken some steps already, so I'll share the findings:PHP 7.4 tests + PHPUnit 8
PHP 7.4 can be added to tests without any problems:
If we want PHP 8 support we need to upgrade PHPUnit to at least 8.5.12 which means we need to drop PHP < 7.2 from tests. But no changes are required in code (except in tests) so the SDK will still work in these version (but not tested anymore):
PHP 8.0 + PHPUnit 9
On PHP 8.0 there are conflicts with development requirements so I dropped them for now. No other changes were needed. One more issue arised - PHPUnit 8 does not support code coverage on PHP 8:
That issue is easily fixed by upgrading PHPUnit to 9.5, but that means we are also dropping PHP 7.2 from tests (which is fine by me since it is not supported anymore, also the SDK still works on that version, just is not tested against it):
PHP 8.1
Although the tests looks good on Github, with PHP 8.1 there are some deprecation warnings thrown that will need code updates. I did not look into that yet as I want to focus on PHP 8.0 support first.
Next steps?
I will be more than happy to submit a PR with PHP 8 support, I just need some guidance here:
Old PHP versions dropping
I am not sure what is your policy on dropping old versions. As I wrote before, no code changes were made in
srcdirectory so nothing could be broken but if it were my SDK, I would not state PHP 7.0 support if that version is not tested against anymore :-)codeclimate/php-test-reporter
I am not sure why there was
codeclimate/php-test-reporterdev dependency since the Github Actions works without it. Can it be dropped or is there some use case I don't see?phpdocumentor/phpdocumentor
I tried to replace the
phpdocumentor/phpdocumentordependency with https://github.com/phpDocumentor/shim and it seems to work. But the new version has different template and requires PHP 7.4+. Again, I'm not sure why that dependency is there and how is it used.