Page MenuHomePhabricator

ParenthesesAroundKeywordSniff can partially apply, resulting in corrupted code
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • Install packages: composer require squizlabs/php_codesniffer mediawiki/mediawiki-codesniffer dealerdirect/phpcodesniffer-composer-installer
  • Create files:
.phpcs.xml.dist
<?xml version="1.0"?>
<ruleset>
    <rule ref="PEAR.Functions.FunctionCallSignature">
        <properties>
            <property name="requiredSpacesAfterOpen" value="1" />
            <property name="requiredSpacesBeforeClose" value="1" />
        </properties>
    </rule>
    <rule ref="MediaWiki.ExtraCharacters.ParenthesesAroundKeyword" />
</ruleset>
file.php
<?php
require_once('bar.php');
  • Run phpcbf on the file.

What happens?:

File is now

<?php
require_once("bar.php";

What should have happened instead?:

File is now

<?php
require_once "bar.php";

Software version (skip for WMF-hosted wikis like Wikipedia): phpcs 3.7.2, mediawiki/mediawiki-codesniffer 41.0.0

Other information (browser name/version, screenshots, etc.):

Running phpcbf with -vvv produces the following output (in part):

	=> Fixing file: 3/3 violations remaining
---START FILE CONTENT---
1|<?php
2|require_once("bar.php");
3|
--- END FILE CONTENT ---
	PEAR.Functions.FunctionCallSignature:247 replaced token 2 (T_OPEN_PARENTHESIS on line 2) "(" => "(·"
	PEAR.Functions.FunctionCallSignature:280 replaced token 4 (T_CLOSE_PARENTHESIS on line 2) ")" => "·)"
	MediaWiki.ExtraCharacters.ParenthesesAroundKeyword:66 replaced token 1 (T_REQUIRE_ONCE on line 2) "require_once" => "require_once·"
	* token 2 has already been modified, skipping *
	* token 4 has already been modified, skipping *
        => Fixing file: 3/3 violations remaining [made 1 pass]...               
	* fixed 3 violations, starting loop 2 *
---START FILE CONTENT---
1|<?php
2|require_once ( "bar.php" );
3|
--- END FILE CONTENT ---
	=> Changeset started by PEAR.Functions.FunctionCallSignature:128
		Q: PEAR.Functions.FunctionCallSignature:130 replaced token 2 (T_WHITESPACE on line 2) "·(" => "("
		Q: PEAR.Functions.FunctionCallSignature:135 replaced token 3 (T_OPEN_PARENTHESIS on line 2) "(" => "("
		A: PEAR.Functions.FunctionCallSignature:136 replaced token 2 (T_WHITESPACE on line 2) "·(" => "("
		A: PEAR.Functions.FunctionCallSignature:136 replaced token 3 (T_OPEN_PARENTHESIS on line 2) "(" => "("
	=> Changeset ended: 2 changes applied
	* token 3 has already been modified, skipping *
	MediaWiki.ExtraCharacters.ParenthesesAroundKeyword:72 replaced token 4 (T_WHITESPACE on line 2) "·"bar.php"" => ""bar.php""
	MediaWiki.ExtraCharacters.ParenthesesAroundKeyword:76 replaced token 7 (T_CLOSE_PARENTHESIS on line 2) ")" => ""
	MediaWiki.ExtraCharacters.ParenthesesAroundKeyword:79 replaced token 6 (T_WHITESPACE on line 2) "·" => ""
        => Fixing file: 5/3 violations remaining [made 2 passes]...             
	* fixed 5 violations, starting loop 3 *
---START FILE CONTENT---
1|<?php
2|require_once("bar.php";
3|
--- END FILE CONTENT ---
        => Fixing file: 0/3 violations remaining [made 3 passes]...             
DONE in 1ms

The critical part seems to be that PEAR.Functions.FunctionCallSignature applied some changes to the code, then MediaWiki.ExtraCharacters.ParenthesesAroundKeyword tried to apply more changes but some of those conflicted with changes from PEAR.Functions.FunctionCallSignature.

If I edit ParenthesesAroundKeywordSniff to call $phpcsFile->fixer->beginChangeset() before its set of changes and $phpcsFile->fixer->endChangeset() after, the corruption no longer occurs because the conflicted replacement causes the entire changeset to fail to apply (and then get applied in a subsequent pass).

Event Timeline

Change 893529 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/tools/codesniffer@master] ParenthesesAroundKeyword: Use fixer->beginChangeset/endChangeset

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

Change 893529 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] ParenthesesAroundKeyword: Use fixer->beginChangeset/endChangeset

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