Skip to content

Build/Travis: test builds against PHP 7.3#754

Closed
jrfnl wants to merge 2 commits intomasterfrom
feature/travis-add-php-7.3
Closed

Build/Travis: test builds against PHP 7.3#754
jrfnl wants to merge 2 commits intomasterfrom
feature/travis-add-php-7.3

Conversation

@jrfnl
Copy link
Member

@jrfnl jrfnl commented Oct 14, 2018

Once PHP 7.3-beta came out, the nightly build 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 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


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 Readme Requirements 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 Warning

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 squizlabs/PHP_CodeSniffer#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.

jrfnl added 2 commits October 14, 2018 18:38
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.
@jrfnl jrfnl added Type: chores/QA PR: quick merge PR only contains relatively simple changes PR: ready for review labels Oct 14, 2018
@jrfnl jrfnl added this to the 9.x Next milestone Oct 14, 2018
@jrfnl jrfnl requested a review from wimg October 14, 2018 16:54
@jrfnl jrfnl force-pushed the feature/travis-add-php-7.3 branch from 1f34db2 to f4a1f6f Compare October 14, 2018 16:54
@jrfnl
Copy link
Member Author

jrfnl commented Oct 14, 2018

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:

  • Where previously the warning would be thrown and sniff execution would continue, now execution will stop - at least for PHPCS 2.x.
  • In PHPCS 3.x, execution would stop for the first file in which the error is encountered already. This is unintended behaviour due to the changed error handling in PHPCS 3.x and was not discovered previously (as the method was introduced well before PHPCS 3.x).
  • Possibly catching the exception in the supportsAbove()/supportsBelow() methods, using the defaults if an exception was caught, and re-throwing the exception later could get us a step closer, but this needs to be investigated.

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.

@jrfnl jrfnl closed this Oct 14, 2018
@jrfnl jrfnl deleted the feature/travis-add-php-7.3 branch December 13, 2018 21:30
@jrfnl jrfnl modified the milestone: 9.1.0 Dec 14, 2018
@jrfnl jrfnl removed the PR: quick merge PR only contains relatively simple changes label Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant