New sniff to remove unnecessary white space in small if conditions#2769
Conversation
Crabcyborg
deleted the
new_sniff_to_remove_unnecessary_whitespace_in_small_if_conditions
branch
WalkthroughThis pull request introduces a new PHP_CodeSniffer sniff that enforces removal of blank lines within short if blocks, while simultaneously applying whitespace cleanup across multiple existing files to remove extraneous blank lines. The changes are entirely whitespace-focused with no modifications to runtime logic or control flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @phpcs-sniffs/Formidable/Sniffs/WhiteSpace/NoBlankLineInShortIfSniff.php:
- Around line 113-117: The error text is inconsistent with the check in
NoBlankLineInShortIfSniff: the sniff only triggers when $bodyLines === 3 but the
message says "3 lines or less"; update either the condition or the message to
match. Easiest fix: change the message passed to $phpcsFile->addFixableError in
NoBlankLineInShortIfSniff to "Remove blank line inside short if block (exactly 3
lines)." Alternatively, if the intention is to cover blocks with up to 3 lines,
adjust the $bodyLines check (currently comparing $bodyLines !== 3) to $bodyLines
<= 3 so the behavior matches the original message.
- Around line 125-128: The fix fails when the whitespace token contains a blank
line with indentation because substr_count( $content, "\n" ) detects 2+ newlines
but the preg_replace( "/\n\n/", "\n", $content, 1 ) only matches consecutive
newlines; update the preg_replace call in NoBlankLineInShortIfSniff (operating
on $content and $blankLineToken) to match a newline, any intervening
spaces/tabs, then another newline (use a regex like newline + optional [ \t]* +
newline), capture the indentation and replace with a single newline plus the
captured indentation (still with a limit of 1) so indented blank lines are
collapsed correctly.
🧹 Nitpick comments (1)
phpcs-sniffs/Formidable/Sniffs/WhiteSpace/NoBlankLineInShortIfSniff.php (1)
92-107: Potential edge case: blank lines with indentation may be missed.If PHPCS tokenizes a blank line as separate tokens with indentation whitespace between them (e.g.,
\n→→\n), the intermediate spaces would resetlastWasNewlineto false, causing the blank line to go undetected.Consider checking if the intermediate token is only horizontal whitespace (spaces/tabs) rather than resetting on any non-newline:
♻️ Suggested improvement
for ( $i = $scopeOpener + 1; $i < $scopeCloser; $i++ ) { if ( $tokens[ $i ]['code'] === T_WHITESPACE && $tokens[ $i ]['content'] === "\n" ) { if ( $lastWasNewline ) { $blankLineToken = $i; break; } $lastWasNewline = true; - } else { + } elseif ( $tokens[ $i ]['code'] !== T_WHITESPACE || strpos( $tokens[ $i ]['content'], "\n" ) !== false ) { + // Reset only if it's not horizontal-only whitespace $lastWasNewline = false; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
phpcs.xmlis excluded by!**/*.xml
📒 Files selected for processing (13)
classes/controllers/FrmAddonsController.phpclasses/controllers/FrmFormsController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmEntriesListHelper.phpclasses/helpers/FrmFormsHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/FrmAddon.phpclasses/models/FrmCreateFile.phpclasses/models/FrmEntryMeta.phpclasses/models/FrmEntryValidate.phpclasses/models/FrmFieldValue.phpclasses/models/FrmStyle.phpphpcs-sniffs/Formidable/Sniffs/WhiteSpace/NoBlankLineInShortIfSniff.php
💤 Files with no reviewable changes (9)
- classes/controllers/FrmAddonsController.php
- classes/models/FrmAddon.php
- classes/models/FrmCreateFile.php
- classes/models/FrmEntryMeta.php
- classes/helpers/FrmXMLHelper.php
- classes/helpers/FrmAppHelper.php
- classes/helpers/FrmFormsHelper.php
- classes/helpers/FrmEntriesListHelper.php
- classes/controllers/FrmFormsController.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmFieldValue.php (1)
classes/factories/FrmFieldFactory.php (2)
FrmFieldFactory(9-141)get_field_object(59-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (6)
classes/models/FrmFieldValue.php (1)
225-229: LGTM – whitespace-only cleanup in short if block.The change removes unnecessary blank lines within the conditional block, aligning with the PR's intent to enforce cleaner formatting in short if statements.
classes/models/FrmStyle.php (1)
568-571: LGTM – whitespace alignment in short if block.The spacing adjustment improves visual alignment with the subsequent line that accesses
$default_style->ID. Consistent with the PR's whitespace cleanup objective.classes/models/FrmEntryValidate.php (1)
1128-1132: LGTM – whitespace alignment for name field string conversion.The spacing adjustment improves visual alignment when converting the name field's first/last array to a string. Consistent with the PR's whitespace cleanup pattern and matches the similar pattern at lines 1102–1104.
phpcs-sniffs/Formidable/Sniffs/WhiteSpace/NoBlankLineInShortIfSniff.php (3)
1-31: LGTM!The file structure, namespace, imports, and class documentation follow PHP_CodeSniffer conventions. The docblock clearly illustrates the intended behavior with good and bad examples.
38-40: LGTM!The register method correctly returns the tokens for if/elseif/else blocks.
50-73: LGTM!The scope validation and body line calculation are correct. The exact 3-line constraint aligns with the sniff's purpose of targeting short if blocks.
| $fix = $phpcsFile->addFixableError( | ||
| 'Remove blank line inside short if block (3 lines or less).', | ||
| $blankLineToken, | ||
| 'BlankLineInShortIf' | ||
| ); |
There was a problem hiding this comment.
Error message inconsistency.
The message says "3 lines or less" but the sniff only triggers for exactly 3-line blocks (line 71 checks $bodyLines !== 3).
📝 Suggested fix
$fix = $phpcsFile->addFixableError(
- 'Remove blank line inside short if block (3 lines or less).',
+ 'Remove blank line inside short if block (exactly 3 body lines).',
$blankLineToken,
'BlankLineInShortIf'
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $fix = $phpcsFile->addFixableError( | |
| 'Remove blank line inside short if block (3 lines or less).', | |
| $blankLineToken, | |
| 'BlankLineInShortIf' | |
| ); | |
| $fix = $phpcsFile->addFixableError( | |
| 'Remove blank line inside short if block (exactly 3 body lines).', | |
| $blankLineToken, | |
| 'BlankLineInShortIf' | |
| ); |
🤖 Prompt for AI Agents
In @phpcs-sniffs/Formidable/Sniffs/WhiteSpace/NoBlankLineInShortIfSniff.php
around lines 113 - 117, The error text is inconsistent with the check in
NoBlankLineInShortIfSniff: the sniff only triggers when $bodyLines === 3 but the
message says "3 lines or less"; update either the condition or the message to
match. Easiest fix: change the message passed to $phpcsFile->addFixableError in
NoBlankLineInShortIfSniff to "Remove blank line inside short if block (exactly 3
lines)." Alternatively, if the intention is to cover blocks with up to 3 lines,
adjust the $bodyLines check (currently comparing $bodyLines !== 3) to $bodyLines
<= 3 so the behavior matches the original message.
| if ( substr_count( $content, "\n" ) >= 2 ) { | ||
| // Replace double newline with single newline, preserving indentation. | ||
| $newContent = preg_replace( "/\n\n/", "\n", $content, 1 ); | ||
| $phpcsFile->fixer->replaceToken( $blankLineToken, $newContent ); |
There was a problem hiding this comment.
Fix may fail when newlines aren't consecutive in the token content.
The detection at line 85 triggers when a whitespace token contains 2+ newlines anywhere (substr_count), but the fix regex /\n\n/ only matches consecutive newlines. If the blank line has indentation (e.g., content is "\n \n"), the regex won't match and $newContent equals $content, leaving the blank line unfixed.
🐛 Proposed fix
// Remove one newline from the whitespace.
if ( substr_count( $content, "\n" ) >= 2 ) {
- // Replace double newline with single newline, preserving indentation.
- $newContent = preg_replace( "/\n\n/", "\n", $content, 1 );
+ // Remove the first blank line (newline + any following whitespace up to next newline).
+ $newContent = preg_replace( "/\n[ \t]*\n/", "\n", $content, 1 );
$phpcsFile->fixer->replaceToken( $blankLineToken, $newContent );🤖 Prompt for AI Agents
In @phpcs-sniffs/Formidable/Sniffs/WhiteSpace/NoBlankLineInShortIfSniff.php
around lines 125 - 128, The fix fails when the whitespace token contains a blank
line with indentation because substr_count( $content, "\n" ) detects 2+ newlines
but the preg_replace( "/\n\n/", "\n", $content, 1 ) only matches consecutive
newlines; update the preg_replace call in NoBlankLineInShortIfSniff (operating
on $content and $blankLineToken) to match a newline, any intervening
spaces/tabs, then another newline (use a regex like newline + optional [ \t]* +
newline), capture the indentation and replace with a single newline plus the
captured indentation (still with a limit of 1) so indented blank lines are
collapsed correctly.
…ary_whitespace_in_small_if_conditions New sniff to remove unnecessary white space in small if conditions
1 participant
No description provided.