Page MenuHomePhabricator
Create Task
Maniphest T209756

IPSet tests fail on PHP 7.3
Closed, ResolvedPublic
Actions

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.

Details

SubjectRepoBranchLines +/-
Upgrade wikimedia/ip-set to 2.0.0mediawiki/coremaster+4 -1
Upgrade wikimedia/ip-set to 1.3.0mediawiki/coreREL1_32+2 -1
Upgrade wikimedia/ip-set to 1.3.0mediawiki/coreREL1_31+2 -1
Modify the test logic to make sure compatibility with PHP 7.3IPSet1.x+31 -19
Modify the test logic to make sure compatibility with PHP 7.3IPSetmaster+31 -19
Customize query in gerrit

Related Objects
Search...

StatusSubtypeAssignedTask
 ResolvedReedyT204884 Run MediaWiki tests on PHP 7.3
 ResolvedRazeSoldierT209756 IPSet tests fail on PHP 7.3

Event Timeline

Legoktm triaged this task as Medium 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
Legoktm added a subscriber: Smalyshev.Nov 17 2018, 6:56 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
RazeSoldier subscribed.Nov 18 2018, 5:29 AM
This comment was removed by RazeSoldier.
RazeSoldier added a comment.Nov 18 2018, 5:29 AM
This comment was removed by RazeSoldier.
RazeSoldier added a comment.Nov 18 2018, 5:42 AM

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.

gerritbot subscribed.Nov 18 2018, 8:55 AM

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

gerritbot added a project: Patch-For-Review.Nov 18 2018, 8:55 AM
RazeSoldier claimed this task.Nov 18 2018, 8:55 AM
Smalyshev added a comment.Nov 18 2018, 8:47 PM

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.

RazeSoldier added a comment.Nov 19 2018, 12:03 AM

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

gerritbot added a comment.Dec 18 2018, 3:23 AM

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

gerritbot added a comment.Dec 18 2018, 3:47 AM

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

gerritbot added a comment.Dec 18 2018, 3:54 AM

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

gerritbot added a comment.Dec 18 2018, 4:01 AM

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

gerritbot added a comment.Dec 18 2018, 4:06 AM

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

gerritbot added a comment.Dec 18 2018, 4:15 AM

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

gerritbot added a comment.Dec 18 2018, 5:45 AM

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

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

gerritbot added a comment.Dec 18 2018, 5:47 AM

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

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

ReleaseTaggerBot added projects: MW-1.32-notes, MW-1.31-release-notes.Dec 18 2018, 6:00 AM
gerritbot added a comment.Dec 18 2018, 8:01 AM

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
ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.9; 2018-12-18).Dec 18 2018, 9:01 AM
RazeSoldier mentioned this in rMLIPd430abed0f2c: IPSet: Modify the test logic to make sure compatibility with PHP 7.3.Sep 24 2023, 3:31 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL