Add tab support to Squiz.Commenting.BlockComment sniff

[photodude]

(based on original question from Gitter chat)
I think I ran into an issue with Squiz.Commenting.DocCommentAlignment or Squiz.Commenting.BlockComment and the Generic.WhiteSpace.DisallowSpaceIndent when running the fixers on this file on our project for a multiline comment. Running phpcs I get no error message related to this block comment.
Before the fix there was two tabs and one space at line 1546 and 1547. After the fix these lines changed to all spaces with 4 spaces for each tab (9 total spaces).
I expected after the fixer ran that the Generic.WhiteSpace.DisallowSpaceIndent sniff convert the spaces to tab indents for me. I also did not expect a fix to occur when there was no error reported.
The full custom standard using Squiz.Commenting.DocCommentAlignment, Squiz.Commenting.BlockComment and Generic.WhiteSpace.DisallowSpaceIndent is available at https://github.com/photodude/coding-standards/tree/phpcs-2

Running PHP_CodeSniffer version 2.6.1 (stable)

I'm also running this with a slight modification of the full custom ruleset using a secondary ruleset to mute some errors and ignore some files.

<?xml version="1.0"?>
<ruleset name="Joomla-CMS">

    <arg name="report" value="full"/>
    <arg name="tab-width" value="4"/>
    <arg name="encoding" value="utf-8"/>
    <arg value="sp"/>
    <arg name="colors" />

    <!-- Exclude folders not containing production code -->
    <exclude-pattern>*/build/*</exclude-pattern>
    <exclude-pattern>*/tests/*</exclude-pattern>
    <exclude-pattern>*/lib/*</exclude-pattern>
    <exclude-pattern>*/tmpl/*</exclude-pattern>
    <exclude-pattern>*/layouts/*</exclude-pattern>

    <!-- Exclude 3rd party libraries. -->
    <exclude-pattern>*/libraries/*</exclude-pattern>
    <exclude-pattern>*/vendor/*</exclude-pattern>
    <exclude-pattern>*/editors/*</exclude-pattern>

    <rule ref="Joomla">
        <exclude name="Joomla.NamingConventions.ValidVariableName.ClassVarHasUnderscore"/>
        <exclude name="Joomla.NamingConventions.ValidFunctionName.FunctionNoCapital"/>
        <exclude name="Joomla.NamingConventions.ValidVariableName.MemberNotCamelCaps"/>
        <exclude name="Joomla.NamingConventions.ValidFunctionName.MethodUnderscore"/>
        <exclude name="Joomla.NamingConventions.ValidVariableName.NotCamelCaps"/>
        <exclude name="Joomla.NamingConventions.ValidFunctionName.ScopeNotCamelCaps"/>
        <exclude name="Generic.Files.LineEndings.InvalidEOLChar"/>
    </rule>
</ruleset>

[photodude]

@gsherwood I have a ruleset.xml subset that I can consistently reproduce this issue with. Generic.Files.LineEndings was the required part to cause the issue to occur

<?xml version="1.0"?>
<ruleset name="Joomla-CMS-3">

    <arg name="report" value="full"/>
    <arg name="tab-width" value="4"/>
    <arg name="encoding" value="utf-8"/>
    <arg value="sp"/>
    <arg name="colors" />

    <!-- Exclude folders not containing production code -->
    <exclude-pattern>*/build/*</exclude-pattern>
    <exclude-pattern>*/tests/*</exclude-pattern>
    <exclude-pattern>*/lib/*</exclude-pattern>
    <exclude-pattern>*/tmpl/*</exclude-pattern>
    <exclude-pattern>*/layouts/*</exclude-pattern>

    <!-- Exclude 3rd party libraries. -->
    <exclude-pattern>*/libraries/*</exclude-pattern>
    <exclude-pattern>*/vendor/*</exclude-pattern>
    <exclude-pattern>*/editors/*</exclude-pattern>

    <!-- Use Unix newlines -->
    <rule ref="Generic.Files.LineEndings">
        <properties>
            <property name="eolChar" value="\n" />
        </properties>
    </rule>

    <rule ref="Generic.WhiteSpace.DisallowSpaceIndent" />
    <rule ref="PEAR.Commenting.InlineComment" />
    <rule ref="Squiz.Commenting.BlockComment">
        <exclude name="Squiz.Commenting.BlockComment.FirstLineIndent"/>
        <exclude name="Squiz.Commenting.BlockComment.LineIndent"/>
        <exclude name="Squiz.Commenting.BlockComment.LastLineIndent"/>
        <exclude name="Squiz.Commenting.BlockComment.HasEmptyLineBefore"/>
        <exclude name="Squiz.Commenting.BlockComment.NoEmptyLineBefore"/>
        <exclude name="Squiz.Commenting.BlockComment.NoEmptyLineAfter"/>
        <exclude name="Squiz.Commenting.BlockComment.WrongStart"/>
    </rule>
    <rule ref="Squiz.Commenting.DocCommentAlignment" />
    <rule ref="Squiz.Commenting.VariableComment">
        <exclude name="Squiz.Commenting.VariableComment.TagNotAllowed" />
    </rule>
</ruleset>

[gsherwood]

If I run PHPCS over the script.php file using the ruleset your pasted in your second comment, I don't get any error and PHPCBF doesn't fix anything.

I also replaced the committed ruleset in your repo with the one you posted so I could run the custom sniffs as well, but again, PHPCBF didn't fix anything, and the only errors I got were:

--------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
--------------------------------------------------------------------------
  147 | ERROR | Variable "Support" is not in valid camel caps format
  152 | ERROR | Variable "Engine" is not in valid camel caps format
  398 | ERROR | Variable "extension_id" is not in valid camel caps format
  400 | ERROR | Variable "client_id" is not in valid camel caps format
 1605 | ERROR | Variable "parent_id" is not in valid camel caps format
--------------------------------------------------------------------------

[gsherwood]

I've tried replicating in another way, and I think I have found what you are looking at and the reason why.

This is the file I am testing with:

<?php
class Foo {
    public function bar() {
        /*
         * Comment line 1.
         * Comment line 2.
         */
        if ($foo) {
            echo 'foo';
        }
    }
}

and this is the ruleset:

<?xml version="1.0"?>
<ruleset name="MyStandard">
    <description>My custom coding standard.</description>
    <arg name="tab-width" value="4"/>
    <rule ref="Generic.WhiteSpace.DisallowSpaceIndent"/>
    <rule ref="Squiz.Commenting.DocCommentAlignment"/>
    <rule ref="Squiz.Commenting.BlockComment"/>
</ruleset>

The result is that the comment lines are indented correctly, but they are indented using spaces instead of tabs.

[gsherwood]

So the real question here is actually: why doesn't the DisallowSpaceIndent sniff disallow (and fix) spaces inside doc blocks?

The answer to that is because both the indent sniffs purposely skip indent checks inside doc blocks and inline HTML because they can't be sure what you actually want in there. Doc block content is sometimes written as HTML because it is used to generate API docs, so it is a little hard to force a specific indent in there.

In this specific case, your comments are inline doc blocks, and the Squiz sniff is specifically looking for these and fixing their indent. The Squiz standard bans the use of tabs for indenting, so this sniff doesn't indent the doc blocks using tabs and has no option to do so. To make this sniff useful to you, it would need to support tab-based indenting.

So after all that, I think this is actually a request for the Squiz.Commenting.BlockComment sniff to fix inline doc block indents using tabs if the file is indenting using tabs. It can probably figure this out by just looking at the existing line indent and assuming tabs if it finds one at the start.

[gsherwood]

I've pushed a change to this sniff that allows it to fix those comment indents using tabs instead of spaces. You don't need to set anything - it just looks at the existing line indent and infers the indent type from it. It isn't going to be perfect, but it should help.

It's probably still not the sniff you really want because I don't think you really want those lines indented. You're using a function doc block style for those inline comment blocks and that sniff is assuming you aren't using an asterisk for every line and you don't want them lined up.

But give it a go and see if you think it is working now.

[photodude]

Do you have a suggestion on a sniff that would be a better fit for our multi-line block comment that expects an asterisk for every line?

[photodude]

I replaced my Squiz.Commenting.BlockComment sniff with the changed file. With our custom ruleset or the ruleset from my second comment, I'm still ending up with comment lines being indented correctly, but they are indented using spaces instead of tabs. phpcs doesn't throw an error but phpcbf results in a fix being applied.

With the MyStandard you posted I get tabs, but it's indented with an extra tab.

[tab][tab]/*
[tab][tab][tab]* Needed for updates post-3.4
[tab][tab][tab]* If com_weblinks doesn't exist then assume we can delete the weblinks package manifest (included in the update packages)
[tab][tab][space]*/

expected

[tab][tab]/*
[tab][tab][space]* Needed for updates post-3.4
[tab][tab][space]* If com_weblinks doesn't exist then assume we can delete the weblinks package manifest (included in the update packages)
[tab][tab][space]*/

This is the last ruleset I tested with.

<?xml version="1.0"?>
<ruleset name="MyStandard-2">

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

    <!-- Use Unix newlines -->
    <rule ref="Generic.Files.LineEndings">
        <properties>
            <property name="eolChar" value="\n" />
        </properties>
    </rule>

    <rule ref="Generic.WhiteSpace.DisallowSpaceIndent" />
    <rule ref="Squiz.Commenting.BlockComment">
        <exclude name="Squiz.Commenting.BlockComment.FirstLineIndent"/>
        <exclude name="Squiz.Commenting.BlockComment.LineIndent"/>
        <exclude name="Squiz.Commenting.BlockComment.LastLineIndent"/>
        <exclude name="Squiz.Commenting.BlockComment.HasEmptyLineBefore"/>
        <exclude name="Squiz.Commenting.BlockComment.NoEmptyLineBefore"/>
        <exclude name="Squiz.Commenting.BlockComment.NoEmptyLineAfter"/>
        <exclude name="Squiz.Commenting.BlockComment.WrongStart"/>
    </rule>
</ruleset>

[photodude]

I'm also now seeing the following code getting reindented from tabs to spaces after the fix is applied with the ruleset I posted above.

JFactory::getDocument()->addScriptDeclaration('
    Joomla.submitbutton = function(task)
    {
        if (task == "profile.cancel" || document.formvalidator.isValid(document.getElementById("profile-form")))
        {
            Joomla.submitform(task, document.getElementById("profile-form"));
        }
    };
');

to

[gsherwood]

With the MyStandard you posted I get tabs, but it's indented with an extra tab.

That's exactly how the sniff wants it, which is why I said I don't think you are going to be able to use this sniff. It doesn't allow the sort of formatting that you require.

Do you have a suggestion on a sniff that would be a better fit for our multi-line block comment that expects an asterisk for every line?

No, there are no included sniffs that allow for this sort of formatting, and no external ones that I know of.

[gsherwood]

I'm also now seeing the following code getting reindented from tabs to spaces after the fix is applied with the ruleset I posted above.

When I run that ruleset over the linked file using the latest PHPCS code, I get no error and no fixes. I get the same result when I run with the current 2.6.1 version of PHPCS as well. So I can't replicate your issue.

[photodude]

@gsherwood I also get no error with PHPCS which is expected, but PHPCBF is applying a fix which is the unexpected part. I only know it's happening because my github repository lists the file as changed after I run PHPCBF.

The question is what am I doing different which is causing you to not be able to replicate the issue?

My local testing environment is a bitnami stack under php 5.4 and I'm running the CLI commands in the git shell; maybe that has an influence?

I'm going to try updating PHPCS to see if that has any effect.

[gsherwood]

I also get no error with PHPCS which is expected, but PHPCBF is applying a fix which is the unexpected part.

I can't replicate that either.

But it also doesn't make sense given how PHPCS and PHPCBF work. Unless you've got the commands pointing at different code bases, they share the same code and work in the same way. PHPCBF just goes the extra step when an error is reported and runs the fixer code, which is directly under the line where the error is generated.

[gsherwood]

I'm going to close this now as I'm releasing the main change in 2.6.2.