-
-
Notifications
You must be signed in to change notification settings - Fork 69
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.
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
base: master
Are you sure you want to change the base?
Ruleset: hard deprecate use of the old property array format #890
Conversation
This format was (soft) deprecated in PHPCS 3.3.0 (June 2018) and it is expected that most rulesets will have been updated. With the new "error handling" functionality in the Ruleset class in place (PR 857), the old format can now be hard deprecated to make users who haven't updated their rulesets yet, aware of the upcoming removal of support for the old array format. Refs: * https://github.com/PHPCSStandards/PHP_CodeSniffer/releases/tag/3.3.0 * squizlabs/PHP_CodeSniffer 1665 Closes 693
$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$`'; |
There was a problem hiding this comment.
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); | |
} | |
} |
There was a problem hiding this comment.
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:
- 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.
- If the sniff referenced by file doesn't follow the PHPCS naming conventions, the sniff code may not be descriptive at all.
Description
This format was (soft) deprecated in PHPCS 3.3.0 (June 2018) and it is expected that most rulesets will have been updated.
With the new "error handling" functionality in the Ruleset class in place (PR #857), the old format can now be hard deprecated to make users who haven't updated their rulesets yet, aware of the upcoming removal of support for the old array format.
Refs:
Suggested changelog entry
Added deprecation notices (hard deprecation) for:
Related issues/external references
Closes #693