Page MenuHomePhabricator

Incorrect fix for MediaWiki.PHPUnit.SpecificAssertions
Closed, ResolvedPublicBUG REPORT

Description

PHPUnit's assertStringContainsString() and assertStringNotContainsString() take the needle first, then the haystack, which is the opposite of PHP's strpos(). This results in an incorrect fix.

Steps to Reproduce:

mkdir /tmp/test
cd /tmp/test
cat <<'EOF' > test.php
<?php
class FooTest {
    function test() {
        $this->assertFalse( strpos( $haystack, $needle ) );
        $this->assertNotFalse( strpos( $haystack, $needle ) );
        $this->assertIsInt( strpos( $haystack, $needle ) );
    }
}
EOF
composer require mediawiki/mediawiki-codesniffer
vendor/bin/phpcbf --standard=vendor/mediawiki/mediawiki-codesniffer/MediaWiki/ruleset.xml test.php

Actual Results:

test.php contains

<?php
class FooTest {
	function test() {
		$this->assertStringNotContainsString( $haystack, $needle );
		$this->assertStringContainsString( $haystack, $needle );
		$this->assertStringContainsString( $haystack, $needle );
	}
}

Expected Results:

test.php contains

<?php
class FooTest {
	function test() {
		$this->assertStringNotContainsString( $needle, $haystack );
		$this->assertStringContainsString( $needle, $haystack );
		$this->assertStringContainsString( $needle, $haystack );
	}
}

Event Timeline

DannyS712 moved this task from Untriaged to Accepted rule changes on the MediaWiki-Codesniffer board.

Change 661247 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/tools/codesniffer@master] SpecificAssertionsSniff: fix order of assertStringContainsString parameters

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

Once this is fixed, is it worth it to do a quick release 35.1.0 to include this fix and the one from T273264?

It looks like the damage was limited: any replacements with assertStringContainsString should have caused the tests to fail, and having reviewed places where assertStringNotContainsString is seen in codesearch I only found two places where the autofix is to blame, and sent fixes for both

we just need to watch out for other places in future autofix patches

Change 661247 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] SpecificAssertionsSniff: fix order of assertStringContainsString parameters

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