Squiz.WhiteSpace.LanguageConstructSpacing does not properly check for tabs and newlines

[michalbundyra]

As we discussed in #1337 (comment) I've created this PR to issues noted in PR #1337

[michalbundyra]

@gmponos please have a look

[gmponos]

I believe that this change here will raise a lot of issues. According to my understanding (maybe I am wrong) the goal of this sniff is to ensure that there are no two spaces between language constructs and $variables/functions etc.

So as I understand it the goal is to avoid this:

return  ['test' => 'value'];

and not this:

return  
    [
        'test' => 'value'
    ];

[michalbundyra]

You have right, but sniff description says:

Ensures all language constructs contain a single space between themselves and their content.

And I think that it is exactly what the sniff should do, we didn't have covered that case previously in tests, so I think @gsherwood is gonna decide what to do.

[jrfnl]

That could be avoided by checking for ' ' or $phpcsFile->eolChar

[glen-84]

To work around this problem, you can use:

return (
    [
        'test' => 'value'
    ]
);

Edit: Or just:

return [
    'test' => 'value'
];

But in my case I have stuff like:

return
    $qb->from(Entities\Player::class, 'p')
        ->leftJoin('p.user', 'u')
        ->select('p', 'u')
        ->where('CASE WHEN p.name IS NULL THEN u.username ELSE p.name END = ?1')
        ->setParameter(1, $name)
        ->getQuery()
        ->getOneOrNullResult();

... so the parentheses work better.

[gmponos]

Personally I avoid assignments that can be tricky to the eye. I would've added the assignment on a different line.

[gmponos]

Now that I see it more carefully the $tokenContent is not used anywhere. So why is it assinged to a variable first?

[michalbundyra]

It was my mistake, the variable shouldn't be there, thanks. Now removed.

[gmponos]

if this is gonna be merged I believe it should cover one more test case. What happens if there is a single space after return and then a new line.

[michalbundyra]

@gmponos I've added this test case and it seems to be fine 😄 Please see 5324d96

[gmponos]

@gsherwood can you check on this PR I need for another one?

[gsherwood]

The intention of this sniff is for a newline to be banned. That's why a return followed by space newline is auto-fixed by removing the newline character. It's just an old sniff I wrote before I knew better, so this fix for tabs and newlines looks correct.

[gsherwood]

Thanks for this fix.