Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ruleset: hard deprecate use of the old property array format #890

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ruleset: hard deprecate use of the old property array format #890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/Ruleset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1219,7 +1219,14 @@ private function processRule($rule, $newSniffs, $depth=0)
}

$printValue = rtrim($printValue, ',');
} else {
} else if (isset($prop['value']) === true) {
$message = 'Passing an array of values to a property using a comma-separated string'.PHP_EOL;
$message .= 'was deprecated in PHP_CodeSniffer 3.3.0. Support will be removed in PHPCS 4.0.0.'.PHP_EOL;
$message .= "The deprecated syntax was used for property \"$name\"".PHP_EOL;
$message .= "for sniff \"$code\".".PHP_EOL;
$message .= 'Pass array values via <element [key="..." ]value="..."> nodes instead.';
$this->msgCache->add($message, MessageCollector::DEPRECATED);

$value = (string) $prop['value'];
$printValue = $value;
if ($value !== '') {
Expand Down
34 changes: 33 additions & 1 deletion tests/Core/Ruleset/PropertyTypeHandlingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,35 @@ final class PropertyTypeHandlingTest extends TestCase
const SNIFF_CLASS = 'Fixtures\\TestStandard\\Sniffs\\SetProperty\\PropertyTypeHandlingSniff';


/**
* Verify a deprecation notice is shown when an array property is set from the ruleset using a comma-separated string.
*
* Support for this format was (soft) deprecated in PHPCS 3.3.0.
*
* @return void
*/
public function testUsingOldSchoolArrayFormatShowsDeprecationNotice()
{
$regex = '`^(';
$regex .= 'DEPRECATED: Passing an array of values to a property using a comma-separated string\R';
$regex .= 'was deprecated in PHP_CodeSniffer 3\.3\.0\. Support will be removed in PHPCS 4\.0\.0\.\R';
$regex .= 'The deprecated syntax was used for property "expectsOldSchool(?:EmptyArray|ArrayWith(?:Extended|Only)?(?:KeysAnd)?Values)"\R';
$regex .= 'for sniff "';
$regex .= '(?:\./tests/Core/Ruleset/Fixtures/TestStandard/Sniffs/SetProperty/PropertyTypeHandlingSniff\.php|TestStandard\.SetProperty\.PropertyTypeHandling)';
$regex .= '"\.\R';
$regex .= 'Pass array values via <element \[key="\.\.\." \]value="\.\.\."> nodes instead\.\R';
$regex .= '){14}\R$`';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that the deprecation notice for each property is displayed twice? There are seven properties using the old array format in PropertyTypeHandlingTest.xml. So, I expected this regex group to repeat seven times, not fourteen.

On a quick check, it seems this happens only when a sniff path is passed to <rule ref=''>. It doesn't happen when a sniff code is passed.

When a sniff path is passed, $todo ends up with two entries for the same sniff (one with the path and another with the code), and then the code that adds the deprecation notice for a given property is executed twice:

PHP_CodeSniffer/src/Ruleset.php

Lines 1078 to 1100 in f2274bd

$todo = [$ref];
$parts = explode('.', $ref);
$partsCount = count($parts);
if ($partsCount <= 2
|| $partsCount > count(array_filter($parts))
|| in_array($ref, $newSniffs) === true
) {
// We are processing a standard, a category of sniffs or a relative path inclusion.
foreach ($newSniffs as $sniffFile) {
$parts = explode(DIRECTORY_SEPARATOR, $sniffFile);
if (count($parts) === 1 && DIRECTORY_SEPARATOR === '\\') {
// Path using forward slashes while running on Windows.
$parts = explode('/', $sniffFile);
}
$sniffName = array_pop($parts);
$sniffCategory = array_pop($parts);
array_pop($parts);
$sniffStandard = array_pop($parts);
$todo[] = $sniffStandard.'.'.$sniffCategory.'.'.substr($sniffName, 0, -9);
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodrigoprimo You're 100% correct and I agree, it's not pretty to have those messages twice. I did consider filtering on sniff code, but ended up deciding against it for the following reasons:

  1. If people include by file name, they may not be aware of the sniff code, so filtering out the file name messages would make it harder for them to identify what to fix in their ruleset.
  2. If the sniff referenced by file doesn't follow the PHPCS naming conventions, the sniff code may not be descriptive at all.


$this->expectOutputRegex($regex);

// Set up the ruleset.
$standard = __DIR__.'/PropertyTypeHandlingTest.xml';
$config = new ConfigDouble(["--standard=$standard"]);
new Ruleset($config);

}//end testUsingOldSchoolArrayFormatShowsDeprecationNotice()


/**
* Test the value type handling for properties set via a ruleset.
*
Expand Down Expand Up @@ -215,6 +244,9 @@ public static function dataTypeHandling()
/**
* Test Helper.
*
* Note: the deprecations for using comma-separated string to pass an array, are silenced in this helper
* as that's not what's being tested here.
*
* @see self::testTypeHandlingWhenSetViaRuleset()
*
* @return \PHP_CodeSniffer\Sniffs\Sniff
Expand All @@ -226,7 +258,7 @@ private function getSniffObjectForRuleset()
if (isset($sniffObject) === false) {
// Set up the ruleset.
$standard = __DIR__."/PropertyTypeHandlingTest.xml";
$config = new ConfigDouble(["--standard=$standard"]);
$config = new ConfigDouble(["--standard=$standard", '-q']);
$ruleset = new Ruleset($config);

// Verify that our target sniff has been registered.
Expand Down