PEAR.WhiteSpace.ObjectOperatorIndent wants incorrect indentation for property array assignment

[Daimona]

Describe the bug

In a specific snippet where a property is assigned an array value, and an element of that array is a chained call, the sniff will require incorrect indentation.

I did not debug this and I don't know what exactly triggers it, but the property assignment seems necessary (e.g., assigning to a normal variable does not reproduce the bug).

Code sample

<?php

$bar->prop = [
    $foo->first()
        ->second(),
];

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
	<rule ref="PEAR.WhiteSpace.ObjectOperatorIndent" />
</ruleset>

To reproduce

Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above
2. Run phpcs test.php
3. See error message displayed

---------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------
 5 | ERROR | [x] Object operator not indented correctly; expected 4 spaces but found 8
---------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------

Expected behavior

No issue should be emitted.

Versions (please complete the following information)

Operating System	Ubuntu
PHP version	8.4.8
PHP_CodeSniffer version	master
Standard	custom
Install type	composer (local)

Please confirm

• I have searched the issue list and am not opening a duplicate issue.
• I have read the Contribution Guidelines and this is not a support question.
• I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
• I have verified the issue still exists in the master branch of PHP_CodeSniffer.

[Daimona]

I haven't yet determined if it's the same issue, but the sniff also behaves poorly in other scenarios where the first line has "peculiar indentation". For example:

<?php

if ( $foo
        ->first( $bar->unrelated() ) // I believe it thinks that the unrelated call is part of the original chained call
) { 
}

<?php

$foo =
    $bar->first()
        ->second();

<?php

if ( $foo->first()
        ->second()
        ) {
}

<?php

$bar =
    rand()
        ? $foo->first()
            ->second()
        : '';

Some of the above examples are questionable code (for example, just moving the chained call out of the if condition would improve readability), but it's not up to this sniff to judge that.

I can test these again once this issue is fixed and open separate issues as needed.

[jrfnl]

@rodrigoprimo Considering you only recently worked on this sniff - would you like to have a look at this issue too ?

[rodrigoprimo]

@Daimona, thanks for opening another issue with examples where PEAR.WhiteSpace.ObjectOperatorIndent is not behaving as expected.

My understanding is that this sniff expects chained object operators when spread over multiple lines to be indented by one level. Considering this, at first glance, I think the sniff is behaving incorrectly in all the snippets you provided, with one exception:

<?php

if ( $foo->first()
        ->second()
        ) {
}

Here, I think that the sniff is correctly flagging that ->second() should be indented four spaces instead of eight, and phpcbf is correctly fixing it. Am I missing something?

Regarding the other examples, I can confirm that, from my perspective, the sniff is not behaving as expected, and some areas of the sniff code require improvement. Below, I will leave some comments for each of the examples.

<?php

$bar->prop = [
    $foo->first()
        ->second(),
];

I believe it will be necessary to refine how the sniff identifies the start and end of chained object operators, as the current approach appears incorrect to me. The sniff uses the start and end of the statement, which is not necessarily the start and end of the chain of object operators. In the code snippet above, when inspecting the first object operator, the sniff "sees" all three object operators as part of the same chain, which is incorrect.

if ( $foo
        ->first( $bar->unrelated() ) // I believe it thinks that the unrelated call is part of the original chained call
) {
}

You are correct that here the sniff thinks that the second object operator is part of a chained call with the first. When looking for chained object operators, the sniff blindly searches for them until the end of the statement.

<?php

$foo =
    $bar->first()
        ->second();

$bar =
    rand()
        ? $foo->first()
            ->second()
        : '';

In the two examples above, the sniff considers the indentation of the line of the variable being assigned as the base indentation, while I believe it should consider the indentation of the line of the object associated with the chain of object operators.

@rodrigoprimo Considering you only recently worked on this sniff - would you like to have a look at this issue too ?

@jrfnl, sure, I have left my assessment above, and I can confirm that this appears to be a valid issue to me. I can work on a PR if you'd like, but I won't have time next week. I might have time the following week.

I did test, and the issues reported here are not a regression caused by #1158. All issues were already present in the sniff before this PR was merged (@Daimona, please let us know if that is not the case and you are only able to reproduce these problems with the changes added in #1158).

[Daimona]

with one exception: [...]
Here, I think that the sniff is correctly flagging that ->second() should be indented four spaces instead of eight, and phpcbf is correctly fixing it. Am I missing something?

I see. I would consider the correct way to write that snippet to be with 8 spaces, but I can see why that isn't so obvious. Just for the sake of explaining where I'm coming from, and referring to the same snippet, I would consider 4 spaces to be the "default" indentation for the conditional part: if I replace the chained call with a different call + logic operator, I would use:

if ( $foo->first()
    && second() )

Therefore, I would use an extra level of indentation for the chained call within the same expression, so it's visually clearer that it belongs to the same operand:

if ( $foo->first()
        ->second()
    && third() )

Or in other words, I would apply the indentation rules as if the $foo->first() were on its own line, as in:

if (
    $foo->first()
        ->second()
    && third() )

But then again, on second thought, it's clearly not as obvious as I originally thought. So, it's fine if the sniff's behaviour for this snippet won't change. As I said before, it's probably a good idea to avoid the chained call inside the if condition in the first place.

I did test, and the issues reported here are not a regression caused by #1158. All issues were already present in the sniff before this PR was merged (@Daimona, please let us know if that is not the case and you are only able to reproduce these problems with the changes added in #1158).

I can confirm that these are pre-existing issues. Before filing the previous issue, I was trying to enable this sniff in the MediaWiki configuration. I saw a bunch of errors, found squizlabs/PHP_CodeSniffer#3718, thought it was the cause of the false positives I observed, and therefore stopped looking. But most of those false positives remained there after your fix in #1158, so I dug deeper and found these additional examples.

[jrfnl]

Thanks @rodrigoprimo for verifying that this is not a regression and @Daimona for reporting these.

I have a feeling that to fix this, the sniff will need adjusting to skip over anything within any type of brackets when searching for the next object operator. In practice, that means that it needs to search for both the object operators as well as all types of open brackets and if the found token is a bracket, it needs to skip to the close bracket and continue searching.

PR(s) welcome ;-)