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

Update the documentation and testing matrix for current PHP releases #445

Merged
merged 16 commits into from
Dec 31, 2019
Merged

Update the documentation and testing matrix for current PHP releases #445

merged 16 commits into from
Dec 31, 2019

Conversation

stevegrunwell
Copy link

@stevegrunwell stevegrunwell commented Dec 5, 2019

This pull request raises the minimum PHP version to 7.2, per the library's "Supported Versions" documentation.

Additionally, since no currently-supported HHVM releases support PHP 7, HHVM has also been removed.

Changes to the .travis.yml file include:

  • Limit testing matrix to supported PHP versions and the nightly snapshot, but permit the nightly build to fail
  • Prevent interactive prompts and prefer distribution versions of libraries when running composer install
  • Until such a time that code coverage is being generated, disable XDebug in the CI environment (performance improvement)

With older PHP versions out of the way, I've also upgraded PHPUnit to version 8 (which requires PHP 7.2+) and made the necessary compatibility fixes to the test suite, which fixes #431.

PHPUnit 8.x requires at least PHP 7.2, which is the lowest version of PHP that has yet to reach EOL.

Fixes #431.
PHPUnit 8 has deprecated `assertInternalType()` in favor of methods like `assertIsArray()`.
Per the README file, this package only supports versions of PHP that are currently supported by the project. As of a few days ago, that's PHP 7.2 and above.

There was also mention of HHVM, but only the LTS releases that still support PHP; unfortunately, that appears to have ended some time ago.
- Include the nightly build of PHP, but permit it to fail
- Include `--no-interaction` and `--prefer-dist` when running `composer install`
- Remove XDebug before running PHPUnit, which improves performance
@stevegrunwell stevegrunwell changed the title Drop support for older PHP versions + HHVM Update the documentation and testing matrix for current PHP releases Dec 10, 2019
@drewish
Copy link

drewish commented Dec 10, 2019

@bhelx what are your thoughts on backwards compatibility with old PHP versions? we've tended to keep testing even on unsupported versions for a bit.

@bhelx
Copy link
Contributor

bhelx commented Dec 10, 2019

Although technically these versions are not supported, we still have a lot of people who cannot upgrade from 5.6, quite a few in 5.5, and a handful of people using 5.4, 5.3, 5.2.
We also have a lot of people using 7.0 and 7.1. We like to try to keep them working for people.

At the least, I'd like to keep 7.1, 7.0, and 5.6. I think we're okay to remove the HHVM versions though as I can't find the actual use of these versions in our logs and I don't think we need to support it moving forward.

@stevegrunwell what would be the consequences of keeping an older version like 5.6 around?

@stevegrunwell
Copy link
Author

First, I 💯 realize this gets into more of a business decision rather than something that I — as a customer + OSS contributor — get to weigh in on, but if an outside perspective helps:

  • It requires some work, but it's possible to change the version of PHPUnit used in a per-environment basis (I'm happy to do that in this PR)
  • Bumping the minimum version of PHP will prevent developers using Composer and older versions of PHP from getting the update; if Composer sees the environment is running PHP 7.0 but the new version of the package requires php: ^7.2, it will keep them where they are.
    • For example, the customers you mentioned who are still using PHP < 5.6 must either be manually installing the updated versions of the library or (more likely) still running older, unpatched versions of Recurly (changes since 15a227423, for reference)
    • However, this means that future bugfixes wouldn't be backported, and I'm guessing your team doesn't want the maintenance burden of manually backporting them.

Maybe a good middle-ground solution would be:

  • Revert the bump in the composer.json file, taking it back to php: >=5.6
  • Lower PHPUnit to version 7, removing the need for the : void return type hints on test fixtures (since return type declarations aren't available in PHP < 7.0).
  • Introduce a simple compatibility layer for running tests written for PHPUnit 7 on earlier versions (essentially, if PHPUnit\Framework\TestCase doesn't exist but PHPUnit_Framework_TestCase does, define a new PHPUnit\Framework\TestCase class that extends PHPUnit_Framework_TestCase
  • Restore PHP 5.6, 7.0, and 7.1 to the Travis testing matrix, but maybe mark them as allowed to fail since they're not officially supported?
  • For versions of PHP that can't run PHPUnit 7 (e.g. PHP < 7.1), explicitly load the PHAR file in the test environment during the installation step.

As an aside, the fact that people are running Recurly — a billing service — on un-supported/unpatched versions of PHP seems like an opportunity for Recurly to help instill consumer trust:

  • Using the logs referenced in this thread, identify customers running older versions of PHP.
  • Notify those customers that are running outdated/insecure versions of PHP

@bhelx
Copy link
Contributor

bhelx commented Dec 30, 2019

@stevegrunwell We greatly appreciate your pragmatism. I'd support the middle ground solution you outlined. I'd like to continue to run the PHP 5 tests even though we aren't explicitly supporting it.

As an aside, the fact that people are running Recurly — a billing service — on un-supported/unpatched versions of PHP seems like an opportunity for Recurly to help instill consumer trust.

Hey, we completely agree. We do run through our logs and capture stats on these things across all our customers. We try our best to get them to upgrade their systems, but for many of these businesses, they just don't have the resources or ability to upgrade. We've found it very hard to make these kind of arguments. Now that we have a team around Developer Experience though, I think we will be doing more of this in the future.

On another note, I wanted to point out that @douglasmiller has made some good progress on the v3 PHP client and it should be ready to test soon. Would you be interested in taking a look at it when it's ready? Since no one is using it yet, we can make sweeping changes to it and we're going to be looking for opinions from our customers using modern PHPs.

@stevegrunwell
Copy link
Author

@bhelx PR has been updated to cover the middle-ground solution discussed.

Now that we have a team around Developer Experience though, I think we will be doing more of this in the future.

I wish them all the luck in the world 😄

On another note, I wanted to point out that @douglasmiller has made some good progress on the v3 PHP client and it should be ready to test soon. Would you be interested in taking a look at it when it's ready?

Absolutely, happy to help however I can!

@bhelx
Copy link
Contributor

bhelx commented Dec 31, 2019

Thanks @stevegrunwell ! That was a quick turn around. Apologies for the late response on my end. Either me or Doug will shoot you an email when the code is ready. We will likely send out some comms to our other PHP customers.

Copy link
Contributor

@bhelx bhelx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bhelx bhelx merged commit f2bc6ef into recurly:master Dec 31, 2019
@joannasese joannasese mentioned this pull request Dec 31, 2019
@bhelx bhelx added the V2 V2 Client label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V2 V2 Client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider upgrading PHPUnit version
3 participants