The "Generic.WhiteSpace.DisallowSpaceIndent" doesn't handle mixed indents

[aik099]

When line is indented using tab + 4 spaces then this isn't reported as an error with tab-width set to 4:

[aik099]

If this will be fixed, then Generic.WhiteSpace.DisallowTabIndent sniff needs to be changed too.

We can:

1. take 1st T_WHITESPACE token on a line (it's content would be mixed spaces/tabs)
2. replace TABs with spaces according to tab-width setting (in fact that content might already be replaced but orig_content contain non-replaced version)
3. replace each tab-width space group (e.g. 4 spaces) with 1 tab (that's what this sniff does for fixing)
4. compare to original whitespace content on that line
5. if they don't match then this is an error

What's not right in above actions is that we're doing same actions for detecting wrong indentation as we'll be doing for fixing, but nobody have requested fixing at that point. This might affect sniff performance.

[gsherwood]

This is intentional.

When people use tabs, they are using them for general alignment and so that different developers can use different tab-width settings in their text editor. But if they switch to spaces at the end of the indent, they are using that for fine adjustments, like I use in the Squiz standard for arrays.

If PHPCS detects tabs | spaces | tabs then it knows the spaces are a mistake and it is can error and fix them. But if it detects tabs | spaces it doesn't know if the developer actually wants those spaces to always remain exactly 4 spaces even if the tab-width of the text editor is changed.

So the sniff has to ignore them or else it will be generating errors based on the tab-width settings, which it should not be doing. The size of a tab shouldn't affect the way the file is reported on or the way the file is fixed or else the point of using tabs is gone.

I know you've got a case where the spaces are a mistake and you want a tab, but I don't think the generic sniff should be fixing this, for the reasons above.

[aik099]

If PHPCS detects tabs | spaces | tabs then it knows the spaces are a mistake and it is can error and fix them.

This is already being detected, right?

I know you've got a case where the spaces are a mistake and you want a tab, but I don't think the generic sniff should be fixing this, for the reasons above.

OK. I guess I'll have to pay more attention during code reviews to spot these wrong indentation, that usually happens when code from space-indentation only project is copied to tab-indentation only project.

Or maybe I can write my own sniff version to instantly report tab/space mixed usage in 1st whitespace on a line as an error (based on original token content before tabs are replaced with spaces)? I know for sure, that in my projects spaces are not used for fine tuning of indentation.

[gsherwood]

Or maybe I can write my own sniff version to instantly report tab/space mixed usage in 1st whitespace on a line as an error

Actually, maybe we can add a strict option to that sniff so you can enforce that spaces can never be used for indentation of any kind.

[aik099]

Actually, maybe we can add a strict option to that sniff so you can enforce that spaces can never be used for indentation of any kind.

That would be great. I hope, that an extra IF for option checking won't slow down the sniff. I won't be able to use that option until next PHPCS release though.

[atstormcup]

I recently ran into this problem too while setting up PHP_CodeSniffer for the first time.

Actually, maybe we can add a strict option to that sniff so you can enforce that spaces can never be used for indentation of any kind.

While you're at it, would you also be able to incorporate a 'greedy' option? Something like #3 in @aik099's first reply.

[gsherwood]

While you're at it, would you also be able to incorporate a 'greedy' option?

I'm not exactly sure what you are after. Is this for checking or fixing? Can you explain a bit more for me?

[atstormcup]

For checking. I think it's already implemented in the fixer code.

By greedy I mean that it converts as many spaces as it can into tabs according to the tab-width, but there may be some spaces left over for alignment adjustments. For example with a tab-width of 4 and a line indented by 1 tab and 5 spaces, the expected content would be 2 tabs and 1 space. This could produce a (fixable?) warning.

This is in contrast to a 'strict' option, which would see the mixture of tabs and spaces and output an error. The default option could be as it is now, accepting any spaces after tabs.

[gsherwood]

I see what you mean now. Thanks for clarifying. Seems like a good option as well.

[atstormcup]

I think it could be done by modifying line 104, although I don't know about the performance implications:

if ($content[0] === ' ') {

becomes

if (strpos($content, ' ') !== false) {

[atstormcup]

Sorry about the confusion, I just realised I had accidentally linked to pull number 3. As I think you've worked out, I meant point number 3 listed above.