WordPress.org

Making WordPress.org

Opened 3 months ago

Closed 7 weeks ago

#3506 closed defect (fixed)

Core SVN commit blocked by a pre-commit hook

Reported by: SergeyBiryukov Owned by:
Milestone: Priority: normal
Component: Version Control Keywords:
Cc:

Description

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.

Change History (6)

#1 follow-up: @Otto42
3 months ago

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.

Last edited 3 months ago by Otto42 (previous) (diff)

#2 @dd32
3 months ago

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.

Last edited 3 months ago by dd32 (previous) (diff)

#3 in reply to: ↑ 1 ; follow-up: @SergeyBiryukov
3 months ago

Replying to Otto42:

Is this new syntax to 7.1?

No, it appeared in 7.0.

#5 in reply to: ↑ 3 @markjaquith
8 weeks ago

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. :-)

#6 @dd32
7 weeks ago

  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.