Skip to content
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

Squiz.WhiteSpace.ScopeKeywordSpacing does not report tabs after "private" #3901

Closed
3 tasks done
Daimona opened this issue Oct 18, 2023 · 2 comments
Closed
3 tasks done

Comments

@Daimona
Copy link
Contributor

Daimona commented Oct 18, 2023

Describe the bug

The Squiz.WhiteSpace.ScopeKeywordSpacing is used to enforce a single space after scope keywords, which include visibility modifiers on method. While the sniff correctly emits an issue if the public keyword is followed by a tab, it doesn't do that for the private keyword. Judging from the error message, it looks like the sniff might be computing the length of the spacing, instead of checking the characters.

Code sample

class MyClass {
	public	function getPublicFoo() { // <-- tab between "public" and "function"
		return 'foo';
	}
	private	function getPrivateFoo() { // <-- tab between "private" and "function"
		return 'foo';
	}
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
  <rule ref="Squiz.WhiteSpace.ScopeKeywordSpacing" />
</ruleset>

To reproduce

Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above...
  2. Run phpcs test.php
  3. See error message displayed
 4 | ERROR | [x] Scope keyword "public" must be followed by a single space; found 2
   |       |     (Squiz.WhiteSpace.ScopeKeywordSpacing.Incorrect)
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY

Expected behavior

PHPCS should emit an error for the getPrivateFoo function, similar to the one emitted for getPublicFoo.

Versions (please complete the following information)

Operating System Ubuntu 22
PHP version 8.2.10
PHP_CodeSniffer version master
Standard custom
Install type composer

Please confirm:

  • I have searched the issue list and am not opening a duplicate issue.
  • I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
  • I have verified the issue still exists in the master branch of PHP_CodeSniffer.
@jrfnl
Copy link
Contributor

jrfnl commented Oct 18, 2023

@Daimona Thanks for the report. This is however not a false positive as tabs vs spaces is not the concern of this sniff.

When you use inline tabs, the size of the tab is determined by the context and the tabWidth setting.

In your case, I presume you have this set to --tab-width=4 ?

That would result (correctly) in:

$ phpcs ./phpcs-3901.php --standard=Squiz --sniffs=Squiz.WhiteSpace.ScopeKeywordSpacing --tab-width=4

FILE: phpcs-3901.php
------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------
 4 | ERROR | [x] Scope keyword "public" must be followed by a single space; found 2
------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------

While if you would set the tabWidth to 6, it would result in:

$ phpcs ./phpcs-3901.php --standard=Squiz --sniffs=Squiz.WhiteSpace.ScopeKeywordSpacing --tab-width=6

FILE: phpcs-3901.php
-------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-------------------------------------------------------------------------------------
 4 | ERROR | [x] Scope keyword "public" must be followed by a single space; found 6
 7 | ERROR | [x] Scope keyword "private" must be followed by a single space; found 5
-------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------

If you want to forbid tabs for anything but indentation, I suggest you add a sniff to that effect to your ruleset.

The PHPCSExtra package, for instance, has the Universal.WhiteSpace.DisallowInlineTabs sniff to do just that.

I suggest closing this issue.

@Daimona
Copy link
Contributor Author

Daimona commented Oct 19, 2023

This is however not a false positive as tabs vs spaces is not the concern of this sniff.

Oh, sorry, I understand now. I thought "single space" meant "exactly one space (U+0020) character", but indeed, that was a bad assumption on my end.

In your case, I presume you have this set to --tab-width=4 ?

I did not set that explicitly, but I just realized that the wrapper I was using to test this did that internally. Sorry about that!

If you want to forbid tabs for anything but indentation, I suggest you add a sniff to that effect to your ruleset.

The PHPCSExtra package, for instance, has the Universal.WhiteSpace.DisallowInlineTabs sniff to do just that.

I'll look into that, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants