Avoid incompatibility between Generic and PSR2 standards for function call indentation
#38
Conversation
fredden
force-pushed
the
whitespace-incompatibility
branch
from
4663def
to
44a32a0
Compare
fredden
force-pushed
the
whitespace-incompatibility
branch
from
44a32a0
to
6ff2dd3
Compare
| @@ -135,7 +129,6 @@ public function getErrorList($testFile='FunctionCallSignatureUnitTest.inc') | |||
| 567 => 1, | |||
| 568 => 1, | |||
| 573 => 1, | |||
| 574 => 1, | |||
There was a problem hiding this comment.
All these errors no longer being thrown, gives me the impression you are breaking the PEAR sniff behaviour in favour of fixing something which affects only PSR2. That doesn't seem right....
What is the justification for this ?
There was a problem hiding this comment.
I've gone back through these changes and re-assessed each in turn. Below are my findings, grouped together by class / reasoning.
Lines 106 & 355
There were two errors being flagged for this line:
106 | ERROR | [x] Multi-line function call not indented correctly; expected 0 spaces but found 4 (PEAR.Functions.FunctionCallSignature.Indent)
106 | ERROR | [x] Closing parenthesis of a multi-line function call must be on a line by itself (PEAR.Functions.FunctionCallSignature.CloseBracketLine)
The first error is actually not correct (or is misleading). The parameter is correctly indented by 4 spaces. The closing parenthesis should be indented by zero spaces, but not on this line. Fixing the second error (which is still reported after the changes in this pull request) by moving the closing parenthesis to its own line makes this clear as the indents for the parameter and closing parenthesis are then checked/reported independently.
The error message when the line is incorrectly indented (eg with 5 spaces) is misleading as it reports "expected 0 spaces but found 5" whereas it should probably report "expected 4 spaces but found 5" for the parameter (and leave the closing parenthesis to report its own indentation anomalies when it's on its own line.). I'm not trying to fix that problem here; this is an improvement that could be made in the future.
Lines 378-380 & 386-388 (& 394-396)
This code snippet shows these errors:
377 | ERROR | [x] Opening statement of multi-line function call not indented correctly; expected 4 spaces but found 5 (PEAR.Functions.FunctionCallSignature.OpeningIndent)
378 | ERROR | [x] Multi-line function call not indented correctly; expected 9 spaces but found 8 (PEAR.Functions.FunctionCallSignature.Indent)
379 | ERROR | [x] Multi-line function call not indented correctly; expected 9 spaces but found 8 (PEAR.Functions.FunctionCallSignature.Indent)
380 | ERROR | [x] Multi-line function call not indented correctly; expected 5 spaces but found 4 (PEAR.Functions.FunctionCallSignature.Indent)
There is ambiguity in the sniff here in that it will accept any multiple of 4 spaces (including zero) on line 377. When line 377 is no longer being flagged as problematic, the following three lines are either correct (when 377 has four spaces) or off by some multiple of four (eg, when 377 has 12 leading spaces, the following three lines are each indented by 8 too few spaces).
The coding standard does say that "Parameters need to be indented 4 spaces compared to the level of the function call", so I can see why the following three lines are being reported as "wrong" by this sniff. However the same coding standard says to "Use an indent of 4 spaces", which makes me think that "expected 9 spaces" and "expected 5 spaces" are in violation of the coding standard.
Lines 394-396 don't show up in the delta for this pull request an error message is still shown for these lines, but the message is changing in this pull request. Previously the sniff would suggest an indent of 13/9 spaces, now the sniff suggests an indent of 12/8 spaces.
Line 574
This one is harder to justify. The sniff (now) accepts both 4 & 6 spaces indentation here as "correct." Should we be accepting only one of these numbers? I can't see any examples of "inline HTML" in the coding standard at https://pear.php.net/manual/en/standards.php
|
As suggested by @jrfnl, I have run all three sniffs mentioned / changed in this pull request on both the Joomla and WordPress codebases. This is to determine if any of the changes here produce different behaviour. For the Joomla codebase, all three combinations produced identical results. For the WordPress codebase, Differences when running `PEAR.Functions.FunctionCallSignature`--- pr-38.wordpress.master.PEAR.Functions.FunctionCallSignature.diff 2024-01-16 16:40:25.423993745 +0000
+++ pr-38.wordpress.whitespace-incompatibility.PEAR.Functions.FunctionCallSignature.diff 2024-01-16 16:55:55.348267329 +0000
@@ -85752,7 +85752,7 @@
}
diff --git a/wp-admin/includes/class-pclzip.php b/wp-admin/includes/class-pclzip.php
-index 963f31178c..dce5bc1d03 100644
+index 963f31178c..15bbc36f5b 100644
--- a/wp-admin/includes/class-pclzip.php
+++ b/wp-admin/includes/class-pclzip.php
@@ -27,7 +27,7 @@
@@ -86224,7 +86224,7 @@
- strlen($p_header['stored_filename']),
- $p_header['extra_len']);
+ strlen($p_header['stored_filename']),
-+ $p_header['extra_len']
++ $p_header['extra_len']
+ );
// ----- Write the first 148 bytes of the header in the archive
@@ -86249,7 +86249,7 @@
- $p_header['disk'], $p_header['internal'],
- $p_header['external'], $p_header['offset']);
+ $p_header['disk'], $p_header['internal'],
-+ $p_header['external'], $p_header['offset']
++ $p_header['external'], $p_header['offset']
+ );
// ----- Write the 42 bytes of the header in the zip file
@@ -86264,7 +86264,7 @@
+ $v_binary_data = pack(
+ "VvvvvVVv", 0x06054b50, 0, 0, $p_nb_entries,
+ $p_nb_entries, $p_size,
-+ $p_offset, strlen($p_comment)
++ $p_offset, strlen($p_comment)
+ );
// ----- Write the 22 bytes of the header in the zip file
@@ -86280,7 +86280,7 @@
+ "Filename '".$v_header['stored_filename']."' is "
."compressed by an unsupported compression "
- ."method (".$v_header['compression'].") ");
-+ ."method (".$v_header['compression'].") "
++ ."method (".$v_header['compression'].") "
+ );
return PclZip::errorCode();
@@ -380845,7 +380845,7 @@
/** WP_Http class */
require_once ABSPATH . WPINC . '/class-wp-http.php';
diff --git a/wp-includes/class-json.php b/wp-includes/class-json.php
-index 6091d567f5..91afc0649e 100644
+index 6091d567f5..017053439d 100644
--- a/wp-includes/class-json.php
+++ b/wp-includes/class-json.php
@@ -1,7 +1,7 @@
@@ -380903,8 +380903,8 @@
- . chr((0xC0 & (ord($utf8[0]) << 6))
- | (0x3F & ord($utf8[1])));
+ . chr(
-+ (0xC0 & (ord($utf8[0]) << 6))
-+ | (0x3F & ord($utf8[1]))
++ (0xC0 & (ord($utf8[0]) << 6))
++ | (0x3F & ord($utf8[1]))
+ );
case 3:
@@ -380919,8 +380919,8 @@
+ | (0x0F & (ord($utf8[1]) >> 2))
+ )
+ . chr(
-+ (0xC0 & (ord($utf8[1]) << 6))
-+ | (0x7F & ord($utf8[2]))
++ (0xC0 & (ord($utf8[1]) << 6))
++ | (0x7F & ord($utf8[2]))
+ );
}
Differences when running `PSR2.Methods.FunctionCallSignature`--- pr-38.wordpress.master.PSR2.Methods.FunctionCallSignature.diff 2024-01-16 16:36:52.090689591 +0000
+++ pr-38.wordpress.whitespace-incompatibility.PSR2.Methods.FunctionCallSignature.diff 2024-01-16 16:52:07.347124466 +0000
@@ -83536,7 +83536,7 @@
}
diff --git a/wp-admin/includes/class-pclzip.php b/wp-admin/includes/class-pclzip.php
-index 963f31178c..108549d0f6 100644
+index 963f31178c..25379c1972 100644
--- a/wp-admin/includes/class-pclzip.php
+++ b/wp-admin/includes/class-pclzip.php
@@ -27,7 +27,7 @@
@@ -83746,8 +83746,8 @@
- "Invalid number / type of arguments");
+ PclZip::privErrorLog(
+ PCLZIP_ERR_INVALID_PARAMETER,
-+ "Invalid number / type of arguments"
-+ );
++ "Invalid number / type of arguments"
++ );
return 0;
}
}
@@ -83761,10 +83761,10 @@
- $v_supported_attributes);
+ $v_result = $this->privFileDescrParseAtt(
+ $v_entry,
-+ $v_filedescr_list[],
-+ $v_options,
-+ $v_supported_attributes
-+ );
++ $v_filedescr_list[],
++ $v_options,
++ $v_supported_attributes
++ );
if ($v_result != 1) {
return 0;
}
@@ -83802,10 +83802,10 @@
- $v_supported_attributes);
+ $v_result = $this->privFileDescrParseAtt(
+ $v_entry,
-+ $v_filedescr_list[],
-+ $v_options,
-+ $v_supported_attributes
-+ );
++ $v_filedescr_list[],
++ $v_options,
++ $v_supported_attributes
++ );
if ($v_result != 1) {
return 0;
}
@@ -83898,12 +83898,12 @@
+ $v_arg_list,
+ $v_size,
+ $v_options,
-+ array (PCLZIP_OPT_BY_NAME => 'optional',
++ array (PCLZIP_OPT_BY_NAME => 'optional',
PCLZIP_OPT_BY_EREG => 'optional',
PCLZIP_OPT_BY_PREG => 'optional',
- PCLZIP_OPT_BY_INDEX => 'optional' ));
-+ PCLZIP_OPT_BY_INDEX => 'optional' )
-+ );
++ PCLZIP_OPT_BY_INDEX => 'optional' )
++ );
if ($v_result != 1) {
return 0;
}
@@ -83948,9 +83948,9 @@
- .$p_options_list[$i]."'");
+ PclZip::privErrorLog(
+ PCLZIP_ERR_INVALID_PARAMETER,
-+ "Unknown parameter '"
-+ .$p_options_list[$i]."'"
-+ );
++ "Unknown parameter '"
++ .$p_options_list[$i]."'"
++ );
// ----- Return
return PclZip::errorCode();
@@ -83962,8 +83962,8 @@
- "Unknown parameter '".$v_key."'");
+ PclZip::privErrorLog(
+ PCLZIP_ERR_INVALID_PARAMETER,
-+ "Unknown parameter '".$v_key."'"
-+ );
++ "Unknown parameter '".$v_key."'"
++ );
// ----- Return
return PclZip::errorCode();
@@ -84098,11 +84098,11 @@
- "Filename '".$v_header['stored_filename']."' is "
+ PclZip::privErrorLog(
+ PCLZIP_ERR_UNSUPPORTED_COMPRESSION,
-+ "Filename '".$v_header['stored_filename']."' is "
++ "Filename '".$v_header['stored_filename']."' is "
."compressed by an unsupported compression "
- ."method (".$v_header['compression'].") ");
-+ ."method (".$v_header['compression'].") "
-+ );
++ ."method (".$v_header['compression'].") "
++ );
return PclZip::errorCode();
}
@@ -84114,11 +84114,11 @@
- "Unsupported encryption for "
+ PclZip::privErrorLog(
+ PCLZIP_ERR_UNSUPPORTED_ENCRYPTION,
-+ "Unsupported encryption for "
++ "Unsupported encryption for "
." filename '".$v_header['stored_filename']
- ."'");
-+ ."'"
-+ );
++ ."'"
++ );
return PclZip::errorCode();
}
@@ -84130,8 +84130,8 @@
- $p_file_list[$v_nb_extracted++]);
+ $v_result = $this->privConvertHeader2FileInfo(
+ $v_header,
-+ $p_file_list[$v_nb_extracted++]
-+ );
++ $p_file_list[$v_nb_extracted++]
++ );
if ($v_result != 1) {
$this->privCloseFd();
$this->privSwapBackMagicQuotes();
@@ -84145,11 +84145,11 @@
- $p_options);
+ $v_result1 = $this->privExtractFile(
+ $v_header,
-+ $p_path,
++ $p_path,
+ $p_remove_path,
-+ $p_remove_all_path,
-+ $p_options
-+ );
++ $p_remove_all_path,
++ $p_options
++ );
if ($v_result1 < 1) {
$this->privCloseFd();
$this->privSwapBackMagicQuotes();
@@ -84161,8 +84161,8 @@
- $p_entry['filename']);
+ = PclZipUtilPathInclusion(
+ $p_options[PCLZIP_OPT_EXTRACT_DIR_RESTRICTION],
-+ $p_entry['filename']
-+ );
++ $p_entry['filename']
++ );
if ($v_inclusion == 0) {
- PclZip::privErrorLog(PCLZIP_ERR_DIRECTORY_RESTRICTION,
@@ -84230,9 +84230,9 @@
- .' Some trailing bytes exists after the archive.');
+ PclZip::privErrorLog(
+ PCLZIP_ERR_BAD_FORMAT,
-+ 'The central dir is not at the end of the archive.'
-+ .' Some trailing bytes exists after the archive.'
-+ );
++ 'The central dir is not at the end of the archive.'
++ .' Some trailing bytes exists after the archive.'
++ );
// ----- Return
return PclZip::errorCode();
@@ -320781,7 +320781,7 @@
}
diff --git a/wp-includes/ID3/module.audio-video.matroska.php b/wp-includes/ID3/module.audio-video.matroska.php
-index eb5febf433..fd447fc344 100644
+index eb5febf433..b4481b8878 100644
--- a/wp-includes/ID3/module.audio-video.matroska.php
+++ b/wp-includes/ID3/module.audio-video.matroska.php
@@ -1047,7 +1047,8 @@ class getid3_matroska extends getid3_handler
@@ -320789,7 +320789,7 @@
$attachedfile_entry['FileName'],
$attachedfile_entry['data_offset'],
- $attachedfile_entry['data_length']);
-+ $attachedfile_entry['data_length']
++ $attachedfile_entry['data_length']
+ );
$this->current_offset = $sub_subelement['end'];
@@ -320844,7 +320844,7 @@
case 3: // 14-bit little-endian
$fhBS .= substr(getid3_lib::BigEndian2Bin(strrev(substr($DTSheader, $word_offset, 2))), 2, 14);
diff --git a/wp-includes/ID3/module.audio.flac.php b/wp-includes/ID3/module.audio.flac.php
-index 014061da94..d1b96ea4df 100644
+index 014061da94..f507d625f3 100644
--- a/wp-includes/ID3/module.audio.flac.php
+++ b/wp-includes/ID3/module.audio.flac.php
@@ -423,7 +423,8 @@ class getid3_flac extends getid3_handler
@@ -320852,13 +320852,13 @@
$this->ftell(),
$picture['datalength'],
- $picture['image_mime']);
-+ $picture['image_mime']
++ $picture['image_mime']
+ );
}
$info['flac']['PICTURE'][] = $picture;
diff --git a/wp-includes/ID3/module.audio.mp3.php b/wp-includes/ID3/module.audio.mp3.php
-index 0d8fee3e5d..bca1016aa2 100644
+index 0d8fee3e5d..a1873fd917 100644
--- a/wp-includes/ID3/module.audio.mp3.php
+++ b/wp-includes/ID3/module.audio.mp3.php
@@ -1358,7 +1358,8 @@ class getid3_mp3 extends getid3_handler
@@ -320866,7 +320866,7 @@
$LongMPEGlayerLookup[$head4],
$LongMPEGpaddingLookup[$head4],
- $LongMPEGfrequencyLookup[$head4]);
-+ $LongMPEGfrequencyLookup[$head4]
++ $LongMPEGfrequencyLookup[$head4]
+ );
}
if ($MPEGaudioHeaderLengthCache[$head4] > 4) {
@@ -322619,7 +322619,7 @@
return true;
}
diff --git a/wp-includes/atomlib.php b/wp-includes/atomlib.php
-index 90f6282243..0dc60416bd 100644
+index 90f6282243..281145681d 100644
--- a/wp-includes/atomlib.php
+++ b/wp-includes/atomlib.php
@@ -147,8 +147,8 @@ class AtomParser {
@@ -322652,13 +322652,12 @@
function xml_escape($content)
{
- return str_replace(array('&','"',"'",'<','>'),
-- array('&','"',''','<','>'),
-- $content );
+ return str_replace(
+ array('&','"',"'",'<','>'),
-+ array('&','"',''','<','>'),
-+ $content
-+ );
+ array('&','"',''','<','>'),
+- $content );
++ $content
++ );
}
}
diff --git a/wp-includes/author-template.php b/wp-includes/author-template.php
@@ -484676,7 +484675,7 @@
-unset( $filter, $action );
+unset($filter, $action);
diff --git a/wp-includes/deprecated.php b/wp-includes/deprecated.php
-index e63708f91b..c0496fd5e8 100644
+index e63708f91b..7344caf2a9 100644
--- a/wp-includes/deprecated.php
+++ b/wp-includes/deprecated.php
@@ -23,7 +23,7 @@
@@ -484908,7 +484907,7 @@
+ 'hide_empty',
+ 'use_desc_for_title',
+ 'children',
-+ 'child_of',
++ 'child_of',
+ 'categories',
+ 'recurse',
+ 'feed',
@@ -576135,7 +576134,7 @@
+_deprecated_file(basename(__FILE__), '2.1.0', WPINC . '/rss.php');
require_once ABSPATH . WPINC . '/rss.php';
diff --git a/wp-includes/rss.php b/wp-includes/rss.php
-index 6d8941ab38..f09972ba58 100644
+index 6d8941ab38..b83c195d54 100644
--- a/wp-includes/rss.php
+++ b/wp-includes/rss.php
@@ -16,7 +16,7 @@
@@ -576292,28 +576291,28 @@
if ( $this->initem ) {
$this->concat(
- $this->current_item[ $this->current_namespace ][ $el ], $text);
-+ $this->current_item[ $this->current_namespace ][ $el ],
++ $this->current_item[ $this->current_namespace ][ $el ],
+ $text
+ );
}
elseif ($this->inchannel) {
$this->concat(
- $this->channel[ $this->current_namespace][ $el ], $text );
-+ $this->channel[ $this->current_namespace][ $el ],
++ $this->channel[ $this->current_namespace][ $el ],
+ $text
+ );
}
elseif ($this->intextinput) {
$this->concat(
- $this->textinput[ $this->current_namespace][ $el ], $text );
-+ $this->textinput[ $this->current_namespace][ $el ],
++ $this->textinput[ $this->current_namespace][ $el ],
+ $text
+ );
}
elseif ($this->inimage) {
$this->concat(
- $this->image[ $this->current_namespace ][ $el ], $text );
-+ $this->image[ $this->current_namespace ][ $el ],
++ $this->image[ $this->current_namespace ][ $el ],
+ $text
+ );
}
@@ -576322,28 +576321,28 @@
if ( $this->initem ) {
$this->concat(
- $this->current_item[ $el ], $text);
-+ $this->current_item[ $el ],
++ $this->current_item[ $el ],
+ $text
+ );
}
elseif ($this->intextinput) {
$this->concat(
- $this->textinput[ $el ], $text );
-+ $this->textinput[ $el ],
++ $this->textinput[ $el ],
+ $text
+ );
}
elseif ($this->inimage) {
$this->concat(
- $this->image[ $el ], $text );
-+ $this->image[ $el ],
++ $this->image[ $el ],
+ $text
+ );
}
elseif ($this->inchannel) {
$this->concat(
- $this->channel[ $el ], $text );
-+ $this->channel[ $el ],
++ $this->channel[ $el ],
+ $text
+ );
}I'm reviewing these differences now. I expect to post an update with my findings in the coming days. |
|
After reviewing the differences, I've identified a bug. The bug was that the PEAR (and PSR2 by inheritance) sniff didn't cope well when lines were indented with tabs, which is explicitly forbidden by the standard. I've fixed this bug (and added more tests) in e98cbb5. I'm not completely happy with some of the "fixed" lines in these tests, but I have verified that running the whole standard (not just the one sniff) produces expected results. In order to get the test-suite to pass, I needed to merge in the main branch. I opted to do this instead of re-basing to make reviewing this pull request easier. @jrfnl please squash-merge this pull request when the time comes. I've re-run the three sniffs over both Joomla and WordPress again. As before, I'm getting identical results for all three sniffs on the Joomla codebase. Also as before, the Also as before, for the WordPress codebase, (GitHub won't let me put these into this comment as they're too big.) From what I can tell, these all produce a better result than before. |
|
@fredden Just checking for the codebases on which you tested which use tab indents (like WordPress), did you set the |
|
No, I did not use any |
There shouldn't need to be any as - as long as standards which are tab-based use the The This means that all sniffs can presume a space-based token stream and work based on that. Yes, this will mean that fixers will make space-based replacements, but that is fine as a tab-based standard should always include the |
|
I have re-run the |
There was a problem hiding this comment.
As discussed in our call, we cannot do this.
Yes, the PSR2 sniff was originally written for PSR2 use and explicitly via the ruleset exclude the OpeningIndent errorcode.
However, the sniff has now been in the PHPCS codebase for over 10 years and people may be including it in their ruleset stand-alone (i.e. not as part of PSR2, but as a single sniff).
Those people will be relying on the OpeningIndent error to be in the sniff and work as expected, so removing that error completely from the sniff is a breaking change we cannot make.
Can you think of another way to solve this ?
Description
This is a re-creation of squizlabs/PHP_CodeSniffer#3631
Suggested changelog entry
GenericandPSR2standards for function call indentationRelated issues/external references
Fixes squizlabs/PHP_CodeSniffer#2078
Related to (and fixes one item from) #152
Types of changes
PR checklist