Core SVN commit blocked by a pre-commit hook

[SergeyBiryukov]

Stumbled upon a pre-commit hook on core SVN blocking a commit to unit tests (adding support for PHPUnit 7.x) due to PHP7 return type declarations: ​https://core.trac.wordpress.org/ticket/43218#comment:16

Commit failed (details follow):
Commit blocked by pre-commit hook (exit code 1) with output:

***********************************
PHP error in: trunk/tests/phpunit/includes/phpunit7/speed-trap-listener.php:

Parse error: syntax error, unexpected ':', expecting ';' or '{' in
 trunk/tests/phpunit/includes/phpunit7/speed-trap-listener.php on line 57
Errors parsing trunk/tests/phpunit/includes/phpunit7/speed-trap-listener.php

The line in question is:

public function addError( PHPUnit\Framework\Test $test, Throwable $t, float $time ): void {

PHPUnit 7.x requires PHP 7.1+, but the file with the PHP7 syntax is included conditionally, and the patch is tested on PHP 5.2.17 with PHPUnit 3.6.12 as well.

Seems like the pre-commit hook should be adjusted to allow PHP7 syntax. If someone unintentionally makes a PHP7-specific commit to core, Travis failures on older PHP versions would let us know.

[Otto42]

The precommit hook does lint against PHP 7 currently. The error is legitimate.

Is this new syntax to 7.1? I'm not sure which specific version is linted against.

[dd32]

I believe meta.svn & plugins.svn are the only repos which currently uses PHP7 linting, develop.svn should therefor be linting against PHP 5.6 or lower.

Seems like the pre-commit hook should be adjusted to allow PHP7 syntax. If someone unintentionally makes a PHP7-specific commit to core, Travis failures on older PHP versions would let us know.

I'm inclined to agree here. Travis catches all sorts of PHP 5.2-related things at least once every few months - usually it's by using a function that doesn't exist which the linting doesn't catch.
Using PHP7 linting shouldn't be an issue here, it'll catch malformed commits before they happen, won't prevent PHP7 syntaxes being used incorrectly, but as long as Tests are covering that part of code it should be fine.

[SergeyBiryukov]

Replying to Otto42:

Is this new syntax to 7.1?

No, ​it appeared in 7.0.

[dd32]

I've made a systems request here: ​https://make.wordpress.org/systems/2018/03/12/change-all-svn-php-linting-to-php7/

[markjaquith]

Replying to SergeyBiryukov:

Replying to Otto42:

Is this new syntax to 7.1?

No, ​it appeared in 7.0.

Return type declarations were introduced in 7.0, but the void return type declaration was introduced in PHP 7.1

​http://php.net/manual/en/migration71.new-features.php

Though that linting error on the : does seem to suggest that the : is the problem it is having, not the void after it. :-)

[dd32]

This should've been fixed with the SVN host migration recently - pre-commit linting should be now PHP7