Php_codesniffer: Squiz.Strings.ConcatenationSpacing.PaddingFound should allow newlines

[ofbeaton]

It would be a cool enhancement if Squiz.Strings.ConcatenationSpacing.PaddingFound would allow newlines before or after a concat operator, as long as it was not alone on a line. That way things like:

$myString = 'This is a really long string.'
  .' It needs to be broken over many lines to observe PSR-2 120-char limit';

// some may prefer this style
$myString = 'This is a really long string.'.
  ' It needs to be broken over many lines to observe PSR-2 120-char limit';

I am fully aware that I could this instead:

$myString = 'This is a really long string.'.
$myString = $myString.' It needs to be broken over many lines to observe PSR-2 120-char limit';

and there are tradeoffs to both, but just like I can split parameters over many lines, I should be able to split strings over many lines through the use of concatenation.

[gsherwood]

Try putting this in your ruleset:

<rule ref="Squiz.Strings.ConcatenationSpacing">
   <properties>
       <property name="ignoreNewlines" value="1"/>
   </properties>
</rule>

[ofbeaton]

ugh! You had already solved it! Thank you, and sorry for the support ticket. I really should have read the source more closely.

[gsherwood]

Thank you, and sorry for the support ticket.

Not at all.

[ofbeaton]

Unfortunately I'm going to have to reopen this as a feature request.

$myString = 'This is a really long string.'
  .' It needs to be broken over many lines to observe PSR-2 120-char limit';

Still produces a Concat operator must not be surrounded by spaces error. Likely because of the spaces used for indentation.

So I tried to remove them.

$myString = 'This is a really long string.'
.' It needs to be broken over many lines to observe PSR-2 120-char limit';

No longer raises a Concat operator must not be surrounded by spaces error, however I now get a Line indented incorrectly; expected at least 2 spaces, found 0. So this is clearly not the right solution.

I also tried to place the . concat operator at the end of the lines instead of at the beginning, but I still got the same error. Can't fool you, CodeSniffer!

So to ignore newlines it will also need to ignore indentation spaces, making it a more involved feature request.

Again, if you're not interested in having this option in the core, I'll implement it in my own sniff.

[gsherwood]

Sorry, try putting _this_ into your ruleset (notice true instead of 1):

<rule ref="Squiz.Strings.ConcatenationSpacing">
   <properties>
       <property name="ignoreNewlines" value="true"/>
   </properties>
</rule>

[ofbeaton]

Ran this against 2 projects, and it seemed to work properly.

function myfunc()
{
   $myString = 'This is a really long string.'
      .' It needs to be broken over many lines to observe PSR-2 120-char limit';
}

is valid.

function myfunc()
{
   $myString = 'This is a really long string.'
.' It needs to be broken over many lines to observe PSR-2 120-char limit';
}

is invalid.

Unfortunately it is not very picky about how many spaces, and I would prefer if it enforced indenting in, as this is also valid:

function myfunc()
{
   $myString = 'This is a really long string.'
   .' It needs to be broken over many lines to observe PSR-2 120-char limit';
}

or too much:

function myfunc()
{
   $myString = 'This is a really long string.'
         .' It needs to be broken over many lines to observe PSR-2 120-char limit';
}

However as far as I'm concerned, it is good enough for me, and we started using it. If we want it to be stricter we'll make our own sniff down the line. Thanks again for the support Greg.

[gsherwood]

The enforced indentation is probably something that needs to be done with a different sniff. I've mostly seen these types of indentation for multi-line string concats:

// Align with var.
if ($foo) {
    $string = 'Some very long string that is '.
    'split over multiple lines';
}

// Single indent from var.
if ($foo) {
    $string = 'Some very long string that is '.
        'split over multiple lines';
}

// Align the strings.
if ($foo) {
    $string = 'Some very long string that is '.
              'split over multiple lines';
}

// Align the operators.
if ($foo) {
    $string = 'Some very long string that is '
            . 'split over multiple lines';
}

So building something to enforce those various types is in my too hard basket for now :)

[ofbeaton]

Ah yes, the trailing operator. Personally I prefer the operator being in front because that is how you do it with arithmetic. It is clearer that it is appending to something, and sometimes I even put the ; on a new line (like a close ]) so I can move lines around indiscriminatingly. aligning the operators instead of just indenting is interesting, I hadn't thought of that.

In the end though I will just do what is easily enforceable, especially since I have a backlog of sniffs to convert from 1.x to 2.x for our company ruleset. We'll see who gets to it first ;)