Page MenuHomePhabricator

IPSet tests fail on PHP 7.3
Closed, ResolvedPublic

Description

IPSet tests fail on PHP 7.3:

22:32:18 1) Wikimedia\IPSet\Test\IPSetTest::testAddCidrWarning with data set "inet fail" ('0af.0af')
22:32:18 Failed asserting that exception of type "PHPUnit\Framework\Error\Warning" is thrown.
22:32:18
22:32:18 2) Wikimedia\IPSet\Test\IPSetTest::testMatchWarning with data set "inet fail" ('0af.0af', false)
22:32:18 Failed asserting that exception of type "PHPUnit\Framework\Error\Warning" is thrown.

https://integration.wikimedia.org/ci/job/composer-package-php73-docker/13/console

cc @BBlack as the primary author.

Related Objects

Event Timeline

Legoktm triaged this task as Normal priority.Nov 17 2018, 6:45 AM
Legoktm created this task.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 17 2018, 6:45 AM

So inet_pton('0af.0af'); used to emit a warning in PHP 7.2 and earlier, but no longer does so in PHP 7.3, despite returning false: https://3v4l.org/RhM46

Digging a bit, it seems like this was caused by https://github.com/php/php-src/pull/3200. And we were relying on that functionality, given the inline comment:

		$raw = inet_pton( $ip );
		if ( $raw === false ) {
			return false; // inet_pton() sends an E_WARNING for us
		}

cc @Smalyshev for his opinion as well.

RazeSoldier moved this task from Backlog to Other on the PHP 7.3 support board.Nov 18 2018, 4:56 AM
This comment was removed by RazeSoldier.
This comment was removed by RazeSoldier.

For my test, the warning is not only triggered once. trigger_error() (https://github.com/wikimedia/IPSet/blob/master/src/IPSet.php#L126) and inet_pton() will trigger a warning. This looks very messy. We may need to improve the test logic before fixing this compatibility issue.

Change 474522 had a related patch set uploaded (by 星耀晨曦; owner: 星耀晨曦):
[IPSet@master] Modify the test logic to make sure compatibility with PHP 7.3

https://gerrit.wikimedia.org/r/474522

IMHO relying on the warnings is not a great idea. We should just ensure it returns false when appropriate, but we should not be checking the warnings, it's a recipe for BC problems.

So, checks the method return value instead of check the warning triggered would be better?

Change 474522 merged by jenkins-bot:
[IPSet@master] Modify the test logic to make sure compatibility with PHP 7.3

https://gerrit.wikimedia.org/r/474522

Change 480408 had a related patch set uploaded (by Legoktm; owner: 星耀晨曦):
[IPSet@1.x] Modify the test logic to make sure compatibility with PHP 7.3

https://gerrit.wikimedia.org/r/480408

Change 480408 merged by jenkins-bot:
[IPSet@1.x] Modify the test logic to make sure compatibility with PHP 7.3

https://gerrit.wikimedia.org/r/480408

Change 480412 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] Upgrade wikimedia/ip-set to 2.0.0

https://gerrit.wikimedia.org/r/480412

Change 480414 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@REL1_32] Upgrade wikimedia/ip-set to 1.3.0

https://gerrit.wikimedia.org/r/480414

Change 480418 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@REL1_31] Upgrade wikimedia/ip-set to 1.3.0

https://gerrit.wikimedia.org/r/480418

Change 480418 merged by jenkins-bot:
[mediawiki/core@REL1_31] Upgrade wikimedia/ip-set to 1.3.0

https://gerrit.wikimedia.org/r/480418

Change 480414 merged by jenkins-bot:
[mediawiki/core@REL1_32] Upgrade wikimedia/ip-set to 1.3.0

https://gerrit.wikimedia.org/r/480414

Change 480412 merged by jenkins-bot:
[mediawiki/core@master] Upgrade wikimedia/ip-set to 2.0.0

https://gerrit.wikimedia.org/r/480412

Legoktm closed this task as Resolved.Dec 18 2018, 8:05 AM