WordPress-Coding-Standards icon indicating copy to clipboard operation
WordPress-Coding-Standards copied to clipboard

Add a variable tab width in the ArrayIndentationSniff?

Open dingo-d opened this issue 6 years ago • 16 comments

From what I've looked at this sniff, the private $tab_width is The --tab-width CLI value that is being used.

I tried changing the default tab width in my ruleset to 2 instead of 4, and that fails (being a private property)

<rule ref="WordPress.Arrays.ArrayIndentation">
  <properties>
    <property name="tabIndent" value="false"/>
    <property name="tab_width" value="2"/> // This will fail.
  </properties>
</rule>

I also have <arg name="tab-width" value="2"/> in my ruleset, but seems like this doesn't work for me, since I always have to indent the array key-value pairs by 4 instead of 2

// This won't throw an error.
array(
    'bla' => 'blaaaa',
);

// This will throw an error.
array(
  'bla' => 'blaaaa',
);

I'm not sure if setting this property to public will break something down the road, but I manually changed it and the above rule works.

I can submit a PR if needed.

dingo-d avatar Jun 19 '18 10:06 dingo-d

@dingo-d <arg name="tab-width" value="2"/> should be respected by the sniff.

I'll look into this when I find some time.

jrfnl avatar Jun 19 '18 10:06 jrfnl

Thank you :)

dingo-d avatar Jun 19 '18 10:06 dingo-d

@dingo-d Just checking the obvious in case it was overlooked: your example uses the property name tab_width (underscore), but @jrfnl's used tab-width (hyphen).

Does the tab-width version work for you?

GaryJones avatar Jun 19 '18 19:06 GaryJones

@GaryJones Nope, the tab-width property won't work, since it's not defined in the ArrayIndentationSniff.

dingo-d avatar Jun 20 '18 07:06 dingo-d

Noting here that $tab_width property is also private in PrecisionAlignmentSniff and DisallowInlineTabsSniff. I can replicate the issue.

GaryJones avatar Jun 20 '18 13:06 GaryJones

Noting here that $tab_width property is also private...

That is by design. The tab width is taken from the PHPCS native --tab-width command line/ruleset argument which is used by numerous sniffs. This is not a (WPCS) sniff specific property.

jrfnl avatar Jun 21 '18 02:06 jrfnl

@dingo-d Ok, so I've just run some tests and cannot reproduce the issue.

I've used "bleeding edge" PHPCS and WPCS to run the tests. I've also tested against WPCS 0.14.1.

I've used the test code as you posted above in combination with the following test ruleset:

<?xml version="1.0"?>
<ruleset name="Test-1380">

	<autoload>/path/to/PHPCSAliases.php</autoload>

	<arg name="tab-width" value="2"/>

	<rule ref="WordPress.Arrays.ArrayIndentation">
	  <properties>
	    <property name="tabIndent" value="false"/>
	  </properties>
	</rule>
</ruleset>

When I then run the command phpcs -p -s ./test-1380.php --standard=test-1380.xml, I get the expected result:

FILE: ./test-1380.php
------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------
 5 | ERROR | [x] Array item not aligned correctly; expected 2 spaces but found 4
   |       |     (WordPress.Arrays.ArrayIndentation.ItemNotAligned)
------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------

Can you post your complete ruleset somewhere ? I'm thinking it may be a ruleset order issue.

WPCS sets the tab-width to 4, so if you include any of the WPCS rulesets, you need to overrule this. To do so, you need to pass <arg name="tab-width" value="2"/> after the WPCS ruleset has loaded, so at the bottom of your own custom ruleset.

Could that be it ?

jrfnl avatar Jun 21 '18 02:06 jrfnl

WPCS sets the tab-width to 4, so if you include any of the WPCS rulesets, you need to overrule this. To do so, you need to pass after the WPCS ruleset has loaded, so at the bottom of your own custom ruleset.

This could be it, as I put the tab-width argument at the top, before overwriting the indentation rules.

Thanks! I'll close the issue.

dingo-d avatar Jun 21 '18 07:06 dingo-d

Interestingly, I read the OP as wanting two spaces for array indentation, and 4-spaces for other indentation.

GaryJones avatar Jun 21 '18 11:06 GaryJones

Nope, 2 spaces throughout the code (deviation from the traditional WPCS)

dingo-d avatar Jun 21 '18 11:06 dingo-d

OK, I'll reopen the issue since adding <arg name="tab-width" value="2"/> didn't solve the original issue. But I noticed that if I add either

<rule ref="Generic.WhiteSpace.ScopeIndent">
  <properties>
    <property name="indent" value="2"/>
    <property name="tabIndent" value="false"/>
  </properties>
</rule>

or

<rule ref="Generic.WhiteSpace.DisallowTabIndent" />

to the ruleset, the WordPress.Arrays.ArrayIndentation.ItemNotAligned will start acting out.

I noticed that when writing unit tests for a sniff, in the WPThemeReview test file, this won't cause any issues (indentation is in tabs)

public function getWarningList() {
  return array(
    3 => 1,
    4 => 1,
    5 => 1,
    6 => 1,
    7 => 1,
    8 => 1,
  );
}

So when I removed the above Generic rules, and converted spaces to tabs the indentation was ok.

With those rules the only way I won't get an error was if I add 2 extra spaces to the array indentation (spaces instead of tabs indentation).

public function getWarningList() {
  return array(
      4 => 1,
      5 => 1,
      6 => 1,
      7 => 1,
      8 => 1,
  );
}

dingo-d avatar Sep 30 '18 16:09 dingo-d

I had the same issue as @dingo-d

In my ruleset I have

<arg name="tab-width" value="2"/>

<rule ref="WordPress.Arrays.ArrayIndentation">
    <properties>
        <property name="tabIndent" value="false" />
    </properties>
</rule>

Originally, in my testing environment I used PHPCS 2.9.1 and the latest version of WPCS. Everything worked as intended. Then I switched to PHPCS 3.3.2 and the tab-width from my ruleset was ignored. Reverted back to 2.9.1 and everything is ok.

Also, when running PHPCS 3.3.2 with --tab-width=2 on the command line everything works as expected.

klemens-st avatar Oct 03 '18 13:10 klemens-st

Hmmm maybe this is a PHPCS issue? 🤔

dingo-d avatar Oct 03 '18 13:10 dingo-d

I tested it a bit more. I have found the cause of this problem.

The reason @jrfnl wasn't able to reproduce this is because she only included the WordPress.Arrays.ArrayIndentation. In such a setup the <arg name="tab-width" value="2"/> is interpreted correctly and everything flies. The problem appears when we want to use it properly in a project: include the WordPress ruleset and then modify indentation:

<arg name="tab-width" value="2"/>

<rule ref="WordPress">

<rule ref="WordPress.Arrays.ArrayIndentation">
  <properties>
    <property name="tabIndent" value="false"/>
  </properties>
</rule>

(It doesn't matter where the tab-width is placed within the file)

I guess this is similar to what you were trying to do @dingo-d?

According to my tests, PHPCS 3 gets the tab-width from the custom ruleset first, then includes the WordPress ruleset and finds this: https://github.com/WordPress/WordPress-Coding-Standards/blob/b04a7843cd544bdf56509bc587f8d830aa78a430/WordPress-Core/ruleset.xml#L6-L7 which overrides the value. I've tested it with phpcs -vvv | grep tab-width and also a var_dump in ArrayIndentationSniff.php:

if ( ! isset( $this->tab_width ) ) {
    $this->tab_width = Helper::getTabWidth( $this->phpcsFile );
    var_dump($this->tab_width);
 }

Still, if the same version of WPCS is run with PHPCS 2 the issue doesn't exist.

klemens-st avatar Mar 12 '21 10:03 klemens-st

I changed a lot in the meantime, but looking at the old codebase, I used

  <!-- Load WordPress Coding standards -->
  <rule ref="WordPress-Core"/>
  <rule ref="WordPress-Docs"/>
  <rule ref="WordPress-Extra"/>

Before defining the WordPress.Arrays.ArrayIndentation, which is basically the same as including the WordPress one.

dingo-d avatar Mar 12 '21 14:03 dingo-d

Solution: bandaided by adding --tab-width=2 every time phpcs is run. Will report this in PHPCS issues to find the cause of this behaviour.

klemens-st avatar Mar 16 '21 08:03 klemens-st

I'm closing the issue.

Based on the above discussion, the issues is not with the sniff itself, which handles this perfectly fine, but with the ruleset interpretation order in PHPCS itself. I know changes to improve the handling of configuration options have been made for PHPCS 4.x, so I don't think there's anything we can do here about this.

jrfnl avatar Dec 09 '22 14:12 jrfnl