Proposal: drop support for PHP 5.3 and PHPCS < 2.6.0 when PHP 7.4 is released #835
Comments
|
Hi @jrfnl, Thank you for raising this question. As a project that aims to support people who need to develop for and support old versions of PHP, it feels that it is problematic to drop support for earlier versions of PHP. However, perhaps that is a misleading way of looking at things. Another way of looking at it would be that there are three types of PHP developer - those who are not doing any automated testing, those who use a third-party service to provide their automated testing infrastructure (e.g. Travis) and those that deploy PHPCompatibility themselves. The first group don't use PHPCompatibility, so are irrelevant. For the second group, the third-party companies will be running a recent PHP version (probably in parallel with older versions) and therefore there is no issue with raising the minimum requirements. In the case of the final group, they will either be using a recent version of PHP for all their development (and using PHPCompatibility as a basic compatibility smoke test), or else will be running multiple parallel PHP versions to allow for proper cross-version testing and are therefore also have a later version of PHP to run PHPCompatibility on. I find it hard to believe that there are developers out there who do their development solely on PHP < 5.3 who are also interested in compatibility with other PHP versions. Therefore, contrary to my usual opinion on these matters, it feels like the impact of raising the minimum PHP version as you describe won't have a significant impact. I would caveat that instead of "drop PHP 5.3 support once PHP 7.4 comes out" it should probably be "drop PHP 5.3 support once PHP 7.4 comes out or 3 months have passed since a PHPCompatibility release that warns about the loss of support (whichever is later)", just to sanity-check that our assumptions are correct. Note that the above is caveated on the presumption that there is a commitment for PHPCompatibility to continue to provide test coverage for all versions of PHP, as is currently the case, regardless of which versions of PHP it is able to run on. (Also, I should note that my answer might well be different if you were talking about dropping PHP 5.4 support.) With regards PHPCS requirements, I don't have an opinion on this, so long as there is some evidence to back up any usage assumptions that the decision is based on (for example, as discussed here: #241 (comment)). |
Sounds like a good plan. Also, see PR #838 which pulls the sniff to warn about the intended update of the minimum PHP requirements.
If by that you mean that it is the intention of PHPCompatibility to continue to provide cross-version compatibility checks for all PHP versions, then yes: this proposal does not change the scope of the project. This proposal is only about dropping support for certain PHP and PHPCS versions on which the scans can be run, not about what will be detected. As for test coverage: PHPCompatibility itself will only be tested to run correctly on supported versions. When support for older PHP/PHPCS versions is dropped, we will no longer test that PHPCompatibility will run correctly on - then - unsupported PHP/PHPCS versions. (but I don't think that's what you meant, is it?)
There is currently no intention to drop support for PHP 5.4.
Both PEAR as well as Packagist allow for stats to be shown for downloads per PHPCS version. The PEAR statistics are a little misleading (low) as PEAR users generally don't download packages, nor update them, nearly as often as Composer users, but still. I don't know if Packagist supports showing a combined download graph for several versions, if so, I haven't found out about it yet, however, you can click on the tagged releases to the right of the graph at the bottom to get some idea of number of downloads. For PEAR, here are the stats:
|
|
FYI - I've been working on (yet) another PHP 7.4 sniff which I cannot get to work on PHP 5.3 with PHPCS < 2.6.0 due to a Tokenizer problem (Undefined index notice) in PHPCS < 2.6.0. Just wanted to mention it as it reinforces why the above proposals are valid and necessary. |
|
@wimg and me discussed this ticket during a call today and have decided that:
The new recommended minimum versions once the drop is in place will be:
|
|
Hi @jrfnl, I meant to respond to this thread ages ago, but somehow it slipped through the net. Apologies for such a tardy reply!
All correct - I meant the former.
Very impressed with your research, and very happy with the conclusions drawn from those stats. Good work! :-)
Bit late to the party, but that all seems sensible. :-) |
Using a normal `composer install` means that while the build was testing against various PHP versions, it would only ever test against the latest PHPCS 3.x version (`3.5.3` at the time of writing) for PHP 5.4-7.4 and PHPCS 2.9.2 for PHP 5.3.
This PR changes the build matrix to test against a variety of PHPCS versions to ensure compatibility with all versions this plugin claims to support.
Some explanation is needed about the choices made in this changeset:
### Adding an `install` section
The `install` section is intended for installing prerequisites for test build tests.
AFAICS, this wasn't previously used to avoid the PHP `lint` command running over the `vendor` directory, but that can be solved by just removing the `vendor` directory from the list PHP `lint` is run over.
Having the install instructions in the `install` section makes it clearer that the installation is not part of the tests.
### Changes to the PHP `lint` command
As mentioned above, the command now excludes the `vendor` directory.
Also, as most PHP versions now have a second build, adding a `LINT` environment variable so the PHP linting is only done on one build per PHP version.
### Changes to the PHPCS run test command
The PSR12 ruleset which is part of the `phpcs.xml.dist` ruleset was only added in PHPCS 3.3.0.
As testing is now done against a wider range of PHPCS versions, this `PSR12` ruleset won't always be available, so I've added a `PHPCS` environment variable to only run the PHPCS check against the repo specific ruleset _once_ per build.
I've added this variable to a build against the latest PHPCS version. While I'd like to use `dev-master` for this, that may be prone to intermittent bugs in PHPCS, so using a stable PHPCS version instead makes the build more sane.
On all the other builds, only a check against `PHPCompatibility` is run which then tests whether the setting of the `installed_paths` for external standards worked correctly.
Now there are two caveats to this:
* PHPCompatibility 9.0.0 dropped support for PHPCS < 2.3.0.
For testing against PHPCS 2.2.0 - 2.3.0 we need to explicitly install PHPCompatibility 8.x.
For testing against PHPCS < 2.2.0 we need to explicitly install PHPCompatibility 7.x, however PHPCompatibility 7.x wasn't compatible with Composer installs yet, so in that case we also need to rename the directory within `vendor` to make things work.
Note/advance notice: PHPCompatibility 10.0.0, which is expected over the next few months, will drop support for PHPCS < 2.6.0 and PHP < 5.4.
Also see: PHPCompatibility/PHPCompatibility#835
* The PHPCS `--exclude` option previously used to exclude the `PHPCompatibility.Upgrade.LowPHPCS` sniff is only available since PHPCS 2.6.2, so cannot be used to exclude that sniff on older PHPCS versions.
With that in mind, switching to using the `--sniffs=` command line option to limit the PHPCS run to just one sniff.
This option has been available since PHPCS 1.x (or even before), so can be used without problem.
Additionally, limiting the test run to just one sniff will make it faster as well.
I've just selected one of the sniffs from PHPCompatibility which has been around for a while for this. The actual sniff doesn't matter after all, as this test is just about PHPCS recognizing the external standard correctly.
### Custom PHP 5.3 script
As the script for the PHPCS run has changed now, there is no need for the custom `script` section for the PHP 5.3 build anymore. This can now revert back to using the normal test `script` again, so to that end, I've remove the custom script.
## Future improvements
This is a first iteration to improve the build script.
In a further iteration, I'd like to suggest exploring the following additional variants:
* Having some builds with a global install of PHPCS + global install of the external standard used for testing.
* Having some builds with a mix of global/local installs of external standards(s).
* Having some builds using Windows as OS instead of Linux.
Using a normal `composer install` means that while the build was testing against various PHP versions, it would only ever test against the latest PHPCS 3.x version (`3.5.3` at the time of writing) for PHP 5.4-7.4 and PHPCS 2.9.2 for PHP 5.3.
This PR changes the build matrix to test against a variety of PHPCS versions to try to ensure compatibility with all versions this plugin claims to support.
Some explanation is needed about the choices made in this changeset:
### Adding an `install` section
The `install` section is intended for installing prerequisites for test build tests.
AFAICS, this wasn't previously used to avoid the PHP `lint` command running over the `vendor` directory, but that can be solved by just removing the `vendor` directory from the list PHP `lint` is run over.
Having the install instructions in the `install` section makes it clearer that the installation is not part of the tests.
### Changes to the PHP `lint` command
As mentioned above, the command now excludes the `vendor` directory.
Also, as most PHP versions now have a second build, adding a `LINT` environment variable so the PHP linting is only done on one build per PHP version.
### Changes to the PHPCS run test command
The PSR12 ruleset which is part of the `phpcs.xml.dist` ruleset was only added in PHPCS 3.3.0.
As testing is now done against a wider range of PHPCS versions, this `PSR12` ruleset won't always be available, so I've added a `PHPCS` environment variable to only run the PHPCS check against the repo specific ruleset _once_ per build.
I've added this variable to a build against the latest PHPCS version. While I'd like to use `dev-master` for this, that may be prone to intermittent bugs in PHPCS, so using a stable PHPCS version instead makes the build more sane.
On all the other builds, only a check against `PHPCompatibility` is run which then tests whether the setting of the `installed_paths` for external standards worked correctly.
Now there are two caveats to this:
* PHPCompatibility 9.0.0 dropped support for PHPCS < 2.3.0.
For testing against PHPCS 2.2.0 - 2.3.0 we need to explicitly install PHPCompatibility 8.x.
For testing against PHPCS < 2.2.0 we need to explicitly install PHPCompatibility 7.x, however PHPCompatibility 7.x wasn't compatible with Composer installs yet, so in that case we also need to rename the directory within `vendor` to make things work.
Note/advance notice: PHPCompatibility 10.0.0, which is expected over the next few months, will drop support for PHPCS < 2.6.0 and PHP < 5.4.
Also see: PHPCompatibility/PHPCompatibility#835
* The PHPCS `--exclude` option previously used to exclude the `PHPCompatibility.Upgrade.LowPHPCS` sniff is only available since PHPCS 2.6.2, so cannot be used to exclude that sniff on older PHPCS versions.
With that in mind, switching to using the `--sniffs=` command line option to limit the PHPCS run to just one sniff.
This option has been available since PHPCS 1.x (or even before), so can be used without problem.
Additionally, limiting the test run to just one sniff will make it faster as well.
I've just selected one of the sniffs from PHPCompatibility which has been around for a while for this. The actual sniff doesn't matter after all, as this test is just about PHPCS recognizing the external standard correctly.
### Custom PHP 5.3 script
As the script for the PHPCS run has changed now, there is no need for the custom `script` section for the PHP 5.3 build anymore. This can now revert back to using the normal test `script` again, so to that end, I've remove the custom script.
## Future improvements
This is a first iteration to improve the build script.
In a further iteration, I'd like to suggest exploring the following additional variants:
* Having some builds with a global install of PHPCS + global install of the external standard used for testing.
* Having some builds with a mix of global/local installs of external standards(s).
* Having some builds using Windows as OS instead of Linux.
Proposal 1:
There are certain tokenizer quirks - especially around the changes in integers, like binary integers (5.4) and numeric literals with underscores (7.4) - which are making supporting PHP 5.3 more and more complicated.
I'd like to propose for PHPCompatibility to drop PHP 5.3 support once PHP 7.4 comes out.
The new minimum PHP version will then be PHP 5.4 which is still ancient.
PHP 5.4 has already been the minimum recommended PHP version since June 2017 (619cb41).
In anticipation of this, I'd like to suggest adding a
PHPCompatibility.Upgrade.LowPHPsniff to warn people who are still running PHPCompatibility on PHP 5.3 about the impending change in the minimum requirements.Proposal 2:
Along the same lines, I'd like to propose we drop support for PHP_CodeSniffer < 2.6.0 at the same time.
PHP_CodeSniffer 2.6.0 has already been the minimum recommended PHPCS version for PHPCompatibility since August 2016 (5598f60).
Additionally, the
LowPHPCSupgrade warning sniff has been in place since July 2018 and has not generated any support requestsOpen questions:
If this proposal is accepted, it would be a good idea to set new (higher) recommended PHP and PHPCS versions when the version drop is implemented.
I'd like to suggest the following for those:
The text was updated successfully, but these errors were encountered: