Skip to content

Add a check for the leading and trailing spaces #2504

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

Open
amieiro wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Add a check for the leading and trailing spaces #2504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d130e94
Add the check for the leading and trailing spaces
amieiro Nov 4, 2024
258b3a5
Remove a debug code
amieiro Nov 4, 2024
c33ae93
Lint
amieiro Nov 4, 2024
2fab295
Add tabs to the checks
amieiro Jun 10, 2025
409f1dd
Add new lines to the checks
amieiro Jun 10, 2025
b215081
Refactor the method to have different error messages for each check
amieiro Jun 11, 2025
7dc4bd4
Add vertical tabs to the checks
amieiro Jun 11, 2025
b8b4fc5
Change the regexs to use hex codes and reorganize some code
amieiro Jun 13, 2025
0f6a099
♻️ refactor(I18n): consolidate leading/trailing whitespace check into…
amieiro Apr 22, 2026
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
37 changes: 37 additions & 0 deletions WordPress/Sniffs/WP/I18nSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p
$has_content = $this->check_string_has_translatable_content( $matched_content, $param_name, $param_info );
if ( true === $has_content ) {
$this->check_string_has_no_html_wrapper( $matched_content, $param_name, $param_info );
$this->check_string_has_no_leading_trailing_whitespace( $matched_content, $param_name, $param_info );
}
}
}
Expand Down Expand Up @@ -804,6 +805,42 @@ private function check_string_has_no_html_wrapper( $matched_content, $param_name
}
}

/**
* Check if a translatable string has leading or trailing whitespace.
*
* @since 3.4.0
*
* @param string $matched_content The token content (function name) which was matched
* in lowercase.
* @param string $param_name The name of the parameter being examined.
* @param array|false $param_info Parameter info array for an individual parameter,
* as received from the PassedParameters class.
*
* @return void
*/
private function check_string_has_no_leading_trailing_whitespace( $matched_content, $param_name, $param_info ) {
Copy link
Copy Markdown
Contributor

@rodrigoprimo rodrigoprimo Apr 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This method accepts $matched_content and $param_name but doesn't use either. The sibling check_string_has_no_html_wrapper() and other related methods (introduced in c9e8dbd) have the same pattern.

@jrfnl, is there a reason for the uniform signature with unused parameters that I'm missing?

If not, I lean toward dropping the unused parameters for check_string_has_no_leading_trailing_whitespace().

Copy link
Copy Markdown
Member

@jrfnl jrfnl Apr 23, 2026

Choose a reason for hiding this comment

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

@jrfnl, is there a reason for the uniform signature with unused parameters that I'm missing?

IIRC, in the original sniff (before extensive refactoring), most, if not all, the "sub-functions" were called via call_user_func(), which meant the expected parameters of these "sub-functions" needed to be the same.

You may very well be correct in that the various refactoring done since that time have made that requirement redundant. This would need to be verified and confirmed and if so, any method signatures for private methods can be cleaned up.

Copy link
Copy Markdown
Contributor

@rodrigoprimo rodrigoprimo Apr 24, 2026

Choose a reason for hiding this comment

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

Thanks for the additional details, @jrfnl.

@amieiro, considering the above, I think it is better to remove the unused parameters from check_string_has_no_leading_trailing_whitespace(). No need to change the other methods.

Copy link
Copy Markdown
Contributor

@rodrigoprimo rodrigoprimo Apr 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Nitpick: maybe check_string_has_no_surrounding_whitespace() is a clearer name?

$content_without_quotes = TextStrings::stripQuotes( $param_info['clean'] );
$first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param_info['start'], ( $param_info['end'] + 1 ), true );

if ( preg_match( '/^\s+/u', $content_without_quotes ) ) {
Copy link
Copy Markdown
Contributor

@rodrigoprimo rodrigoprimo Apr 23, 2026

Choose a reason for hiding this comment

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

Nitpick: I believe the + quantifier is redundant here and in the regex below. A single surrounding whitespace character is enough to trigger these errors.

Suggested change
if ( preg_match( '/^\s+/u', $content_without_quotes ) ) {
if ( preg_match( '/^\s/u', $content_without_quotes ) ) {

Copy link
Copy Markdown
Contributor

@rodrigoprimo rodrigoprimo Apr 23, 2026

Choose a reason for hiding this comment

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

I wonder about the use of the u modifier. I assume it was added so that \s also matches non-ASCII whitespace characters? I worry a bit about unintended consequences of this modifier, as PHP's UTF-8 handling can be subtle, but I'm not sure whether any such edge cases apply here.

If it is important to match non-ASCII whitespace, I'd suggest adding tests to cover the cases the sniff should catch (presumably the ones you mentioned here). Currently, the tests pass if the u modifier is removed.

$this->phpcsFile->addError(
'Translatable string should not have leading whitespace. Found: %s',
$first_non_empty,
'LeadingWhiteSpace',
array( $param_info['clean'] )
);
}

if ( preg_match( '/\s+$/u', $content_without_quotes ) ) {
$this->phpcsFile->addError(
'Translatable string should not have trailing whitespace. Found: %s',
$first_non_empty,
'TrailingWhiteSpace',
array( $param_info['clean'] )
);
}
}

/**
* Check for inconsistencies in the placeholders between single and plural form of the translatable text string.
*
Expand Down
52 changes: 52 additions & 0 deletions WordPress/Tests/WP/I18nUnitTest.1.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,4 +328,56 @@ MyNamespace\__( 'foo', 'my-slug' ); // Ok.
namespace\esc_html_e( 'foo', '' ); // The sniff should start flagging this once it can resolve relative namespaces.
namespace\Sub\translate( 'foo', 'my-slug' ); // Ok.

__( ' string', 'my-slug' ); // Bad: leading space.
Copy link
Copy Markdown
Contributor

@rodrigoprimo rodrigoprimo Apr 23, 2026

Choose a reason for hiding this comment

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

Looking at the new test cases for this check, I wonder if there is a reason to have duplicate tests that differ only in function name? They seem redundant to me because the method's logic doesn't vary based on the function name.

To me, the tests should cover different whitespace positions, whitespace types, and potentially parameter names, plus a few variations in whitespace quantity to guard against future regressions. What do you think?

_e( 'string ', 'my-slug' ); // Bad: trailing space.
_x( ' string ', 'context', 'my-slug' ); // Bad: leading and trailing spaces.
_ex( ' string', 'context', 'my-slug' ); // Bad: leading spaces.
_n( ' There is %1$d monkey in the %2$s', 'In the %2$s there are %1$d monkeys', $number, 'my-slug' ); // Bad: leading space.
_n( 'There is %1$d monkey in the %2$s', 'In the %2$s there are %1$d monkeys ', $number, 'my-slug' ); // Bad: trailing space.
_n( ' There is %1$d monkey in the %2$s', 'In the %2$s there are %1$d monkeys ', $number, 'my-slug' ); // Bad: leading and trailing spaces.
_nx( ' I have %d cat.', 'I have %d cats.', $number, 'Not really.', 'my-slug' ); // Bad: leading spaces.
_nx( 'I have %d cat.', 'I have %d cats. ', $number, 'Not really.', 'my-slug' ); // Bad: trailing spaces.
_nx( ' I have %d cat.', 'I have %d cats. ', $number, 'Not really.', 'my-slug' ); // Bad: leading and trailing spaces.
_n_noop( ' I have %d cat.', 'I have %d cats.', 'my-slug' ); // Bad: leading space.
_n_noop( 'I have %d cat.', 'I have %d cats. ', 'my-slug' ); // Bad: trailing space.
_n_noop( ' I have %d cat.', 'I have %d cats. ', 'my-slug' ); // Bad: leading and trailing spaces.
_nx_noop( ' I have %d cat.', 'I have %d cats.', 'Not really.', 'my-slug' ); // Bad: leading spaces.
_nx_noop( 'I have %d cat.', 'I have %d cats. ', 'Not really.', 'my-slug' ); // Bad: trailing spaces.
_nx_noop( ' I have %d cat.', 'I have %d cats. ', 'Not really.', 'my-slug' ); // Bad: leading and trailing spaces.
esc_html__( ' string', 'my-slug' ); // Bad: leading space.
esc_html_e( 'string ', 'my-slug' ); // Bad: trailing space.
esc_html_x( ' string ', 'context', 'my-slug' ); // Bad: leading and trailing spaces.
esc_attr__( ' string', 'my-slug' ); // Bad: leading space.
esc_attr_e( 'string ', 'my-slug' ); // Bad: trailing space.
esc_attr_x( ' string ', 'context', 'my-slug' ); // Bad: leading and trailing spaces.
__( ' string', 'my-slug' ); // Bad: leading tab.
__( 'string ', 'my-slug' ); // Bad: trailing tab.
__( ' string ', 'my-slug' ); // Bad: leading and trailing tabs.
__( ' string', 'my-slug' ); // Bad: two leading tabs.
__( 'string ', 'my-slug' ); // Bad: two trailing tabs.
__( " String with leading vertical tab", 'my-slug' ); // Bad: leading vertical tab.
__( "String with trailing vertical tab ", 'my-slug' ); // Bad: trailing vertical tab.
__( " String with leading and trailing vertical tabs ", 'my-slug' ); // Bad: leading and trailing vertical tabs.
__( " String with 2 leading vertical tabs", 'my-slug' ); // Bad: 2 leading vertical tabs.
__( "String with 2 trailing vertical tabs ", 'my-slug' ); // Bad: 2 trailing vertical tabs.
__( '
string', 'my-slug' ); // Bad: leading new line.
__( 'string
', 'my-slug' ); // Bad: trailing new line.
__( '
string
', 'my-slug' ); // Bad: leading and trailing new line.
__( '

string', 'my-slug' ); // Bad: two leading new lines.
__( 'string

', 'my-slug' ); // Bad: two trailing new lines.
__( ' string', 'my-slug' ); // Bad: leading tab + space (1 leading whitespace error).
__( 'string ', 'my-slug' ); // Bad: trailing space + tab (1 trailing whitespace error).
__( ' string ', 'my-slug' ); // Bad: mixed leading and trailing whitespace (2 errors).
__( '
string
', 'my-slug' ); // Bad: leading newline+tab and trailing tab+newline (2 errors).

// phpcs:enable WordPress.WP.I18n.MissingTranslatorsComment
52 changes: 52 additions & 0 deletions WordPress/Tests/WP/I18nUnitTest.1.inc.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,4 +328,56 @@ MyNamespace\__( 'foo', 'my-slug' ); // Ok.
namespace\esc_html_e( 'foo', '' ); // The sniff should start flagging this once it can resolve relative namespaces.
namespace\Sub\translate( 'foo', 'my-slug' ); // Ok.

__( ' string', 'my-slug' ); // Bad: leading space.
_e( 'string ', 'my-slug' ); // Bad: trailing space.
_x( ' string ', 'context', 'my-slug' ); // Bad: leading and trailing spaces.
_ex( ' string', 'context', 'my-slug' ); // Bad: leading spaces.
_n( ' There is %1$d monkey in the %2$s', 'In the %2$s there are %1$d monkeys', $number, 'my-slug' ); // Bad: leading space.
_n( 'There is %1$d monkey in the %2$s', 'In the %2$s there are %1$d monkeys ', $number, 'my-slug' ); // Bad: trailing space.
_n( ' There is %1$d monkey in the %2$s', 'In the %2$s there are %1$d monkeys ', $number, 'my-slug' ); // Bad: leading and trailing spaces.
_nx( ' I have %d cat.', 'I have %d cats.', $number, 'Not really.', 'my-slug' ); // Bad: leading spaces.
_nx( 'I have %d cat.', 'I have %d cats. ', $number, 'Not really.', 'my-slug' ); // Bad: trailing spaces.
_nx( ' I have %d cat.', 'I have %d cats. ', $number, 'Not really.', 'my-slug' ); // Bad: leading and trailing spaces.
_n_noop( ' I have %d cat.', 'I have %d cats.', 'my-slug' ); // Bad: leading space.
_n_noop( 'I have %d cat.', 'I have %d cats. ', 'my-slug' ); // Bad: trailing space.
_n_noop( ' I have %d cat.', 'I have %d cats. ', 'my-slug' ); // Bad: leading and trailing spaces.
_nx_noop( ' I have %d cat.', 'I have %d cats.', 'Not really.', 'my-slug' ); // Bad: leading spaces.
_nx_noop( 'I have %d cat.', 'I have %d cats. ', 'Not really.', 'my-slug' ); // Bad: trailing spaces.
_nx_noop( ' I have %d cat.', 'I have %d cats. ', 'Not really.', 'my-slug' ); // Bad: leading and trailing spaces.
esc_html__( ' string', 'my-slug' ); // Bad: leading space.
esc_html_e( 'string ', 'my-slug' ); // Bad: trailing space.
esc_html_x( ' string ', 'context', 'my-slug' ); // Bad: leading and trailing spaces.
esc_attr__( ' string', 'my-slug' ); // Bad: leading space.
esc_attr_e( 'string ', 'my-slug' ); // Bad: trailing space.
esc_attr_x( ' string ', 'context', 'my-slug' ); // Bad: leading and trailing spaces.
__( ' string', 'my-slug' ); // Bad: leading tab.
__( 'string ', 'my-slug' ); // Bad: trailing tab.
__( ' string ', 'my-slug' ); // Bad: leading and trailing tabs.
__( ' string', 'my-slug' ); // Bad: two leading tabs.
__( 'string ', 'my-slug' ); // Bad: two trailing tabs.
__( " String with leading vertical tab", 'my-slug' ); // Bad: leading vertical tab.
__( "String with trailing vertical tab ", 'my-slug' ); // Bad: trailing vertical tab.
__( " String with leading and trailing vertical tabs ", 'my-slug' ); // Bad: leading and trailing vertical tabs.
__( " String with 2 leading vertical tabs", 'my-slug' ); // Bad: 2 leading vertical tabs.
__( "String with 2 trailing vertical tabs ", 'my-slug' ); // Bad: 2 trailing vertical tabs.
__( '
string', 'my-slug' ); // Bad: leading new line.
__( 'string
', 'my-slug' ); // Bad: trailing new line.
__( '
string
', 'my-slug' ); // Bad: leading and trailing new line.
__( '

string', 'my-slug' ); // Bad: two leading new lines.
__( 'string

', 'my-slug' ); // Bad: two trailing new lines.
__( ' string', 'my-slug' ); // Bad: leading tab + space (1 leading whitespace error).
__( 'string ', 'my-slug' ); // Bad: trailing space + tab (1 trailing whitespace error).
__( ' string ', 'my-slug' ); // Bad: mixed leading and trailing whitespace (2 errors).
__( '
string
', 'my-slug' ); // Bad: leading newline+tab and trailing tab+newline (2 errors).

// phpcs:enable WordPress.WP.I18n.MissingTranslatorsComment
41 changes: 41 additions & 0 deletions WordPress/Tests/WP/I18nUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,47 @@ public function getErrorList( $testFile = '' ) {
315 => 1,
318 => 1,
323 => 1,
331 => 1,
332 => 1,
333 => 2,
334 => 1,
335 => 1,
336 => 1,
337 => 2,
338 => 1,
339 => 1,
340 => 2,
341 => 1,
342 => 1,
343 => 2,
344 => 1,
345 => 1,
346 => 2,
347 => 1,
348 => 1,
349 => 2,
350 => 1,
351 => 1,
352 => 2,
353 => 1,
354 => 1,
355 => 2,
356 => 1,
357 => 1,
358 => 1,
359 => 1,
360 => 2,
361 => 1,
362 => 1,
363 => 1,
365 => 1,
367 => 2,
370 => 1,
373 => 1,
376 => 1,
377 => 1,
378 => 2,
379 => 2,
);

case 'I18nUnitTest.2.inc':
Expand Down
Loading