Add a check for the leading and trailing spaces by amieiro · Pull Request #2504 · WordPress/WordPress-Coding-Standards

amieiro · 2024-11-04T10:31:15Z

As I described in #2501, at translate.w.org, sometimes we get translations that start or end with one or more empty spaces, and we want to avoid this.

This PR adds a new check to detect if a translatable string has leading or trailing spaces.

Fixes #2501.

jrfnl

Hi @amieiro, thanks for this PR.

I haven't tested it yet, but based on a code review alone, this is looking good.

I wonder if the tests can be made a little more varied ? I.e. not just use leading/trailing spaces, but also have some leading/trailing tabs, new lines and combinations of spaces, tabs, new lines.

What do you think ?

jrfnl · 2024-12-24T22:22:42Z

@amieiro Just checking in as you never responded to the question I posed in my initial review.... Do you still intend to finish off this PR ?

amieiro · 2025-05-27T13:31:43Z

Sorry, I don't have the time to finish it at this time.

GaryJones · 2025-06-15T16:14:05Z

+				array( $param_info['clean'] )
+			);
+		}
+


Suggested change

GaryJones · 2025-06-15T16:14:15Z

+				array( $param_info['clean'] )
+			);
+		}
+


Suggested change

GaryJones · 2025-06-15T16:14:25Z

+				array( $param_info['clean'] )
+			);
+		}
+


Suggested change

GaryJones · 2025-06-15T16:14:32Z

+				array( $param_info['clean'] )
+			);
+		}
+


Suggested change

GaryJones · 2025-06-15T16:14:40Z

+				array( $param_info['clean'] )
+			);
+		}
+


Suggested change

GaryJones · 2025-06-15T16:16:29Z

+		$pattern_trailing_spaces = '/[\x20]+$/u';
+		$pattern_leading_tabs    = '/^\x09+/u';
+		$pattern_trailing_tabs   = '/\x09+$/u';
+		$pattern_leading_vtabs   = '/^\x0B+/u';


Suggested change

$pattern_leading_vtabs = '/^\x0B+/u';

$pattern_leading_vtabs = '/^\x0B+/u';

GaryJones · 2025-06-15T16:16:37Z

+		$pattern_leading_spaces  = '/^[\x20]+/u';
+		$pattern_trailing_spaces = '/[\x20]+$/u';
+		$pattern_leading_tabs    = '/^\x09+/u';
+		$pattern_trailing_tabs   = '/\x09+$/u';


Suggested change

$pattern_trailing_tabs = '/\x09+$/u';

$pattern_trailing_tabs = '/\x09+$/u';

GaryJones · 2025-06-15T16:16:48Z

+		// Define regex patterns.
+		$pattern_leading_spaces  = '/^[\x20]+/u';
+		$pattern_trailing_spaces = '/[\x20]+$/u';
+		$pattern_leading_tabs    = '/^\x09+/u';


Suggested change

$pattern_leading_tabs = '/^\x09+/u';

$pattern_leading_tabs = '/^\x09+/u';

GaryJones · 2025-06-15T16:17:07Z

+
+		// Define regex patterns.
+		$pattern_leading_spaces  = '/^[\x20]+/u';
+		$pattern_trailing_spaces = '/[\x20]+$/u';


Suggested change

$pattern_trailing_spaces = '/[\x20]+$/u';

$pattern_trailing_spaces. = '/[\x20]+$/u';

GaryJones · 2025-06-15T16:17:13Z

+		$first_non_empty        = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param_info['start'], ( $param_info['end'] + 1 ), true );
+
+		// Define regex patterns.
+		$pattern_leading_spaces  = '/^[\x20]+/u';


Suggested change

$pattern_leading_spaces = '/^[\x20]+/u';

$pattern_leading_spaces = '/^[\x20]+/u';

jrfnl · 2025-06-15T16:36:19Z

@amieiro Seeing as you've been working on this again - could you please explain why you have set things up to have separate errors for spaces vs tabs vs vtabs vs newlines ?

My previous question was only about having tests in place with these and the sniff handling all types of whitespace, not about splitting that up into multiple errors.

I cannot think of any situation where one type of (leading/trailing) whitespace would be acceptable, but another would not be, so having different error codes doesn't make sense to me.

Is there a reason for this distinction based on your experiences in the Polyglots team ? I'd be interested to hear your reasoning for this.

amieiro · 2025-06-16T08:28:52Z

@jrfnl Sorry, this PR is not yet ready for review and I forgot to convert it to draft. As we return to contribute with WordPress.org, I resumed work on this PR.

The horizontal tabulations are not often seen at the beginning or end of sentences, but they are seen in the middle, so I wanted to prevent its use, since I don't think it is correct to use it. You can see a use case inside a string in the core here. The same applies to the new lines. You can see an example in the core here.
And we have another thing to check: the different whitespace characters. The regular space characters is U+0020, but there are a lot of different whitespace characters: U+00A0, U+2000, U+2001, U+2002,...

The most common problem is the U+0020 space problem (in fact it is the only one I usually see), so the others are really edge situations. I can continue this PR in two different approaches:

Contemplate only the U+0020 case and add the other cases to the tests.
Add all the commented situations.

Which approach do you prefer?

jrfnl · 2025-06-16T14:47:34Z

@jrfnl Sorry, this PR is not yet ready for review and I forgot to convert it to draft. As we return to contribute with WordPress.org, I resumed work on this PR.

Okay, thanks for letting me know. I've converted it to draft now and won't do an in-detail review until it has been marked as "Ready to review".

The horizontal tabulations are not often seen at the beginning or end of sentences, but they are seen in the middle, so I wanted to prevent its use, since I don't think it is correct to use it. You can see a use case inside a string in the core here. The same applies to the new lines. You can see an example in the core here.

I think that this PR should probably focus on the original problem it was trying to solve - leading and trailing whitespace.

Expanding the scope to mid-string whitespace characters feels like scope-creep and may have different implications (i.e., would involve a different decision point, potentially delaying the merging of the code related to the original problem).

I suggest opening a follow-up issue for that to discuss if this needs flagging and if so, what would need flagging. (I mean, looking at the examples, these seem like perfectly valid use-cases for mid string vertical whitespace, especially if you also consider RTL and BTT languages).

And we have another thing to check: the different whitespace characters. The regular space characters is U+0020, but there are a lot of different whitespace characters: U+00A0, U+2000, U+2001, U+2002,...

The most common problem is the U+0020 space problem (in fact it is the only one I usually see), so the others are really edge situations. I can continue this PR in two different approaches:

Contemplate only the U+0020 case and add the other cases to the tests.

Add all the commented situations.

Which approach do you prefer?

For leading and trailing whitespace, IMO the new errors should flag all whitespace characters, but without differentiating in the error message or code.
What I mean by that is:

Change the error message(s) to use the term "whitespace" instead of explicitly saying what type of whitespace you're flagging.
Change the error code(s) to also use the term "whitespace" - this would mean you'd end up with two error codes: LeadingWhiteSpace and TrailingWhiteSpace.

Does that help ?

jrfnl · 2026-03-19T16:31:34Z

@amieiro Just checking in - will you have time to finish this PR in the near future ? It's been over a year since you opened it and it's still not marked as ready for review, while the base premise of the PR seems like a valid and valuable addition, which would help WPCS users.

amieiro · 2026-04-22T16:11:41Z

Thanks for the feedback. I just pushed a refactor that addresses all of it.

What changed:

Collapsed the eight error codes (Leading/Trailing × Spaces/Tabs/VTabs/NewLines) into just two: LeadingWhiteSpace and TrailingWhiteSpace.
Reworded both error messages to use the generic term "whitespace".
Replaced the eight hex-code regexes with a single \s match on each side. It already covers space, tab, vertical tab, newline, CR, and form feed in one pass.
Added four combination test cases (leading tab + space, trailing space + tab, mixed leading/trailing, and multi-line with newline + tab on each side).
Bumped the @since annotation to 3.4.0.

Out of scope here, happy to open follow-up issues: mid-string whitespace and Unicode whitespace characters (U+00A0, U+2000, U+200B, etc.).

.

rodrigoprimo · 2026-04-23T14:37:06Z

@amieiro, thanks for updating this PR! Could you please rebase it to fix the conflicts?

@SInCE

… two error codes - replace eight per-character error codes (Leading/Trailing x Spaces/Tabs/VTabs/NewLines) with two generic codes: LeadingWhiteSpace and TrailingWhiteSpace - reduce the eight hex-code regexes to a single `\s` match on each side (covers space, tab, vertical tab, newline, CR, form feed in one pass) - reword error messages to use the generic term "whitespace" - bump @SInCE annotation from 3.2.0 to 3.4.0 - add four combination test cases (leading tab+space, trailing space+tab, mixed leading/trailing, multi-line with newline+tab on each side) to verify strings with multiple whitespace types produce at most one leading and one trailing error

amieiro · 2026-04-23T15:09:40Z

@amieiro, thanks for updating this PR! Could you please rebase it to fix the conflicts?

Done!

rodrigoprimo

Thanks for updating this PR, @amieiro. I confirmed that you addressed the points raised in previous reviews. Besides that, I also left some inline comments for us to discuss.

rodrigoprimo · 2026-04-23T17:31:27Z

+		$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 ) ) {


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 ) ) {

rodrigoprimo · 2026-04-23T17:53:03Z

+		$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 ) ) {


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.

rodrigoprimo · 2026-04-23T18:18:13Z

+	 *
+	 * @return void
+	 */
+	private function check_string_has_no_leading_trailing_whitespace( $matched_content, $param_name, $param_info ) {


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().

@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.

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.

rodrigoprimo · 2026-04-23T18:22:12Z

+	 *
+	 * @return void
+	 */
+	private function check_string_has_no_leading_trailing_whitespace( $matched_content, $param_name, $param_info ) {


Nitpick: maybe check_string_has_no_surrounding_whitespace() is a clearer name?

rodrigoprimo · 2026-04-23T18:46:11Z

 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.


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?

jrfnl reviewed Nov 5, 2024

View reviewed changes

jrfnl added Type: Enhancement Component: Core Focus: I18n labels Nov 5, 2024

jrfnl added this to the 3.2.0 milestone Nov 5, 2024

jrfnl added the Status: Awaiting feedback label Nov 11, 2024

GaryJones previously requested changes Jun 15, 2025

View reviewed changes

jrfnl marked this pull request as draft June 16, 2025 14:29

jrfnl removed this from the 3.2.0 milestone Jun 23, 2025

amieiro marked this pull request as ready for review April 22, 2026 16:11

amieiro requested review from GaryJones and jrfnl April 22, 2026 16:13

amieiro added 9 commits April 23, 2026 16:55

Add the check for the leading and trailing spaces

d130e94

Remove a debug code

258b3a5

Lint

c33ae93

Add tabs to the checks

2fab295

Add new lines to the checks

409f1dd

Refactor the method to have different error messages for each check

b215081

Add vertical tabs to the checks

7dc4bd4

Change the regexs to use hex codes and reorganize some code

b8b4fc5

amieiro force-pushed the leading-trailing-spaces branch from d03a56f to 0f6a099 Compare April 23, 2026 15:04

rodrigoprimo reviewed Apr 23, 2026

View reviewed changes

	$pattern_leading_vtabs = '/^\x0B+/u';
	$pattern_leading_vtabs = '/^\x0B+/u';

	$pattern_trailing_tabs = '/\x09+$/u';
	$pattern_trailing_tabs = '/\x09+$/u';

	$pattern_leading_tabs = '/^\x09+/u';
	$pattern_leading_tabs = '/^\x09+/u';

	$pattern_trailing_spaces = '/[\x20]+$/u';
	$pattern_trailing_spaces. = '/[\x20]+$/u';

	$pattern_leading_spaces = '/^[\x20]+/u';
	$pattern_leading_spaces = '/^[\x20]+/u';

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

Uh oh!

Conversation

amieiro commented Nov 4, 2024

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

jrfnl commented Dec 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amieiro commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl commented Jun 15, 2025

Uh oh!

amieiro commented Jun 16, 2025

Uh oh!

jrfnl commented Jun 16, 2025

Uh oh!

jrfnl commented Mar 19, 2026

Uh oh!

amieiro commented Apr 22, 2026

Uh oh!

rodrigoprimo commented Apr 23, 2026

Uh oh!

amieiro commented Apr 23, 2026

Uh oh!

rodrigoprimo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jrfnl commented Dec 24, 2024 •

edited

Loading

rodrigoprimo Apr 23, 2026 •

edited

Loading

rodrigoprimo Apr 23, 2026 •

edited

Loading