PEAR FunctionCallSignature checks wrong indent when first token on line is part of a multi-line string

[phlopsi]

I have tested this with the current master branch (last commit: ab7e490)

I have 2 examples showing what the sniffer/fixer is doing wrong. The first example is actually fine according to PSR-2, as far as I know.

test_0.php:

<?php
if (true) {
    $test = '
   ' . function_0(
        $argument_0
    );
}

$ phpcs --standard=PSR2 test_0.php

FILE: C:\[...]\test_0.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 5 | ERROR | [x] Multi-line function call not indented correctly;
   |       |     expected 7 spaces but found 8
 6 | ERROR | [x] Multi-line function call not indented correctly;
   |       |     expected 3 spaces but found 4
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Result after "fixing":

<?php
if (true) {
    $test = '
   ' . function_0(
       $argument_0
   );
}

As you can see, it indents according to the closing single quote instead of the indentation from the variable $test, anything after that is correctly formatted, though.

The second example goes a bit farther than the first one and leads to an endless fixing loop, not fixing anything in that file, in the end.

test_1.php

<?php
if (true) {
    if (true) {
        $test = '
   ' . function_0(
            $argument_0
        );
    }
}

$ phpcs --standard=PSR2 test_1.php

FILE: C:\[...]\test_1.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 6 | ERROR | [x] Multi-line function call not indented correctly;
   |       |     expected 7 spaces but found 12
 7 | ERROR | [x] Multi-line function call not indented correctly;
   |       |     expected 3 spaces but found 8
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

$ phpcbf --no-patch --standard=PSR2 test_1.php
Changing into directory C:\[...]
Processing test_1.php [PHP => 43 tokens in 9 lines]... DONE in 5ms (2 fixable violations)
        => Fixing file: 1/2 violations remaining [made 50 passes]... ERROR in 123ms
Fixed 0 files

[gsherwood]

I'm just commenting on the first bit here.

As you can see, it indents according to the closing single quote instead of the indentation from the variable $test, anything after that is correctly formatted, though.

The function call it is checking is function_0(). To determine how it is indented, it looks at the indent of the line on which the function call starts. In this case, it has an indent of 3 spaces. It adds 4 spaces, and then requires an indent of 7 spaces for the argument list. So the sniff and fix is working fine.

Here is a neater example to show the actual format it wants:

<?php
$result = foo(
    $arg1,
    $arg2
);

Or another being used as a function argument.

<?php
$result = foo(
    bar(
        $arg1,
        $arg2
    ),
    true
);

I'll look at the second part of your report in a min.

[phlopsi]

Hmm, just to avoid any possible misunderstandings, because I used spaces for the example:

If it would instead be

if (true) {
    $var = 'rtewtertwet
wetwetertw' . function_0(
    $argument_0
);
}

You would still argue, that this is correctly indented?

[phlopsi]

My comment made me realize just now, that the fixer is wrong either way, because the spaces are part of the string and not indent, i.e. the actual indent is always 0 spaces in this case and not 3.

[gsherwood]

Let me try changing the sniff so that it goes back to the start of the statement and not the start of the line. If that doesn't break any existing tests, then I'll commit that.

[gsherwood]

There is a failure. It is this code, which previously generated no errors:

public function doSomething()
{
    return $this->getFoo()
        ->doBar(
            $this->getX() // no comma here
                ->doY()  // this is still the first method argument
                ->doZ()  // this is still the first method argument
        );
}

It now wants to use the return as the indent level, forcing this:

public function doSomething()
{
    return $this->getFoo()
        ->doBar(
        $this->getX() // no comma here
            ->doY()  // this is still the first method argument
            ->doZ()  // this is still the first method argument
    );
}

I might need to just add an exception for the -> token in this specific case.

[gsherwood]

I ended up just checking the indent for the start of line (as it does now) and the start of the statement. Plus I also fixed that issue where the empty string was seen as the indent. The sniff now uses the max of those two indents to figure out what indent rules the code is using.

With this change, all your code posted here is now considered correct. This also fixed the PHPCBF conflict because the expected indent is the same for this sniff as it is for the generic ScopeIndent sniff.

Thanks for reporting it, and finding that blank string indent issue in your follow-up comments.

[phlopsi]

I think this might get pretty ugly, if you do it that way around, since the real exceptions are spaces inside of a string being at the start of a line.

If you look at the tokenizer from php, you should know that only T_WHITESPACE, T_COMMENT and T_DOC_COMMENT contain any whitespace that has to be considered for indentation purposes, the other whitespace we have in this issue is in the T_STRING token which has nothing to do with that.

If you make "->" the exception there might be lots of problems coming up for any infix operator you can place at the beginning of a line followed by a multi-line function.

What you probably want to do instead is, when you encounter a string you might want to search for the opening quote and treat the following multi-line function as if it would look like:

$var = '' . function_0(
    $argument_0
);

since that is what you need to look at from a syntactic point of view.

[phlopsi]

Didn't see your follow-up comment, when i was writing mine, but that means you didn't have to implement any exceptional cases and fixed the problem the clean way?

[gsherwood]

but that means you didn't have to implement any exceptional cases and fixed the problem the clean way?

Yeah. I didn't make an exception after all. It made more sense to look at the two possible indents and make a clearer assumption about what indent rule is being used. It allows some freedom while still enforcing the same basic rule it always did.