Closed
Conversation
Once PHP 7.3-beta came out, the `nightly` build on Travis became PHP 7.4-dev and build haven't been tested against PHP 7.3 for months now. Luckily, Travis has *finally* deemed it appropriate to set up a PHP 7.3 alias now RC3 is out, so I've added PHP 7.3 to the matrix. It would have been nice to move the coverage builds onto 7.3 at this time as that should raise the coverage a little as the PHP 7.3 specific code in the `NewFlexibleHeredocNowdoc` sniff should then show as covered. However, that is not yet possible at this time as there is _no code coverage driver available_ for PHP 7.3 at this time. See https://travis-ci.org/PHPCompatibility/PHPCompatibility/jobs/441316823
Ok, so this one will need some explaining and is actually about allowing the unit tests to pass on PHP 7.3. First off, the underlying problem is that PHPCS isn't fully compliant with PHP 7.3 until version 3.3.1 - see upstream PR 2086. PHPCS prior to version 3.3.1, has a couple of `continue` statements within `switch` control structures causing PHP 7.3 to throw a warning. One of these is in the code for the `Tokenizer` class which is used to enhance the token array for every file being analysed and therefore unavoidable. In PHPCS 2.x, this doesn't cause too much of a problem as PHPCS handled warnings encountered during the tokenizer runs differently. In PHPCS 3.x, however, it becomes a problem as when such a warning will get thrown during the unit test run, PHPUnit will mark the first test which throws a warning as `Error` and fail the build. Skipping select tests for the PHP 7.3 / PHPCS 3.0 < 3.3.1 combi will not help as that just moves the error to the next unit test with the same net effect. Now, PHPUnit has a `convertWarningsToExceptions` setting which defaults to `true`. Changing that setting to `false` will allow the unit test run to pass as in that case, the first test which sees the warning will be marked as `Risky` for having output, but will not fail the build. The next problem we then run into, is that PHPCompatibility itself throws a warning in the `getTestVersion()` method. With the `convertWarningsToExceptions` setting set to `false`, those warnings will be thrown and the unit tests for that method which relied on the warnings being converted to exceptions will now fail. That, however, is fixable. So, I've added a new `PHPCompatibility\Util\TestVersionException` class and turned the warnings which were being thrown from the `getTestVersion()` method into Exceptions. This Exception unfortunately can't extend the PHPCS native `RuntimeException` as PHPCS native exceptions weren't added until PHPCS 3. Having said that, the current implementation allows the `convertWarningsToExceptions` setting to be set to `false` and our unit tests to still pass. For PHP 7.3 in combination with PHPCS 3.0 < 3.3.1, this means that the very first test **will** be marked as risky, but other than that, the builds should pass. **Note**: there is one additional caveat. PHPCS 2.x has a bug in the error handling and will throw an `Undefined offset: 0` error now whenever we throw the new Exception in PHPCS 2.x. That warning will _only_ be thrown when someone sets an invalid `testVersion` though, so I'm not too concerned about that. N.B.: The new `PHPCompatibility\Util\TestVersionException` class uses the new-style file and class docblock documentation as proposed in 734. For that reason, some additional CS exceptions have been set in the `phpcs.xml.dist` file. Those can and will be removed when the big PR to fix 734 for all files is pulled.
1f34db2 to
f4a1f6f
Compare
Member
Author
|
I'm withdrawing this PR for the moment, may re-open at a later point. After thinking it over some more, this is not the right solution. The problem with this solution is as follows:
Alternatively, we could take a hard line and say that PHPCompatibility is only supported on PHP 7.3 in combination with PHPCS 3.3.1+ and not test against PHP 7.3 with lower PHPCS combinations. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Once PHP 7.3-beta came out, the
nightlybuild on Travis became PHP 7.4-dev and builds haven't been tested against PHP 7.3 for months now.Luckily, Travis has finally deemed it appropriate to set up a PHP 7.3 alias now RC3 is out, so I've added PHP 7.3 to the matrix.
It would have been nice to move the coverage builds onto 7.3 at this time as that should raise the coverage a little as the PHP 7.3 specific code in the
NewFlexibleHeredocNowdocsniff should then show as covered.However, that is not yet possible at this time as there is no code coverage driver available for PHP 7.3 at this time. See https://travis-ci.org/PHPCompatibility/PHPCompatibility/jobs/441316823
Now, once I'd pushed the branch for this PR, the PHP 7.3 / PHPCS 3.0.2 build turned out to be failing. To fix this I've added a second commit with a small, but significant change.
I've also annotated in the
ReadmeRequirements section that PHP 7.3 is only fully supported in combination with PHPCS 3.3.1 or higher.As it took quite some figuring out and for future reference, this is the reasoning behind the additional changes (as written in the commit message):
Sniff:getTestVersion(): throw Exception instead of PHP native WarningOk, so this one will need some explaining and is actually about allowing the unit tests to pass on PHP 7.3.
First off, the underlying problem is that PHPCS isn't fully compliant with PHP 7.3 until version 3.3.1 - see upstream PR squizlabs/PHP_CodeSniffer#2086.
PHPCS prior to version 3.3.1, has a couple of
continuestatements withinswitchcontrol structures causing PHP 7.3 to throw a warning. One of these is in the code for theTokenizerclass which is used to enhance the token array for every file being analysed and therefore unavoidable.In PHPCS 2.x, this doesn't cause too much of a problem as PHPCS handled warnings encountered during the tokenizer runs differently.
In PHPCS 3.x, however, it becomes a problem as when such a warning will get thrown during the unit test run, PHPUnit will mark the first test which throws a warning as
Errorand fail the build.Skipping select tests for the PHP 7.3 / PHPCS 3.0 < 3.3.1 combi will not help as that just moves the error to the next unit test with the same net effect.
Now, PHPUnit has a
convertWarningsToExceptionssetting which defaults totrue. Changing that setting tofalsewill allow the unit test run to pass as in that case, the first test which sees the warning will be marked asRiskyfor having output, but will not fail the build.The next problem we then run into, is that PHPCompatibility itself throws a warning in the
getTestVersion()method.With the
convertWarningsToExceptionssetting set tofalse, those warnings will be thrown and the unit tests for that method which relied on the warnings being converted to exceptions will now fail.That, however, is fixable.
So, I've added a new
PHPCompatibility\Util\TestVersionExceptionclass and turned the warnings which were being thrown from thegetTestVersion()method into Exceptions.This Exception unfortunately can't extend the PHPCS native
RuntimeExceptionas PHPCS native exceptions weren't added until PHPCS 3.Having said that, the current implementation allows the
convertWarningsToExceptionssetting to be set tofalseand our unit tests to still pass.For PHP 7.3 in combination with PHPCS 3.0 < 3.3.1, this means that the very first test will be marked as risky, but other than that, the builds should pass.
Note: there is one additional caveat. PHPCS 2.x has a bug in the error handling and will throw an
Undefined offset: 0error now whenever we throw the new Exception in PHPCS 2.x.That warning will only be thrown when someone sets an invalid
testVersionthough, so I'm not too concerned about that.N.B.: The new
PHPCompatibility\Util\TestVersionExceptionclass uses the new-style file and class docblock documentation as proposed in #734.For that reason, some additional CS exceptions have been set in the
phpcs.xml.distfile. Those can and will be removed when the big PR to fix #734 for all files is pulled.