Add support for multiline indenting to ObjectOperatorIndentSniff

[marcospassos]

This PR adds supports for multilevel indenting to the amazing sniff ObjectOperatorIndentSniff created by @gsherwood.

Multilevel indenting contributes to improve code readability in long chained method calls. This convention is widely used by Symfony in configuration definitions:

$rootNode
    ->children()
        ->booleanNode('auto_connect')
            ->defaultTrue()
        ->end()
        ->scalarNode('default_connection')
            ->defaultValue('default')
        ->end()
    ->end()
;

The option multilevel allows enabling/disabled the new behavior, which is disabled by default to be backward compatible with previous versions.

The sniff can automatically fix violations by adjusting the number of spaces based on the value of property indent and the previous indentation:

// Before fix
$rootNode
    ->children()
		        ->booleanNode('auto_connect')
        ->end();

// After fix
$rootNode
    ->children()
        ->booleanNode('auto_connect')
    ->end();

Tests are included to confirm the correctness.

[jrfnl]

Just wondering: should $previousIndent be (re)set in this leaf of the condition as well ? Maybe to $expectedIndent ?

[marcospassos]

This is one of the possible heuristics.

Notice that it affects error reporting only:

// Keeping the last valid
$builder
    ->start()
            ->do() // Excpected 8, found 12
                ->end(); // Expected 8, found 16

// Keeping the last adjusted
$builder
    ->start()
            ->do() // Excpected 8, found 12
                ->end(); // Expected 12, found 16

Let's go with your suggestion as it may lead to more correct results most of the time, assuming that if there are extraneous spaces, it is more likely that there should be indentation than nothing.

[marcospassos]

@jrfnl I've adjusted the logic based on your suggestion.

[jrfnl]

I haven't tested the PR, but based on the code itself, this looks to be a good change.

[marcospassos]

@jrfnl any chances of merging it anytime soon, once the PR includes tests?

[jrfnl]

@marcospassos That's not up to me. Depends on when @gsherwood has time to have a look at this as well.

once the PR includes tests?

What do you mean by this ? AFAICS, the PR does include some unit tests.
If you like, I could have a look to see if I can think of cases not yet covered by current tests ?

Other than that, you may want to reset the custom property to the default after the tests covering the property, to prevent confusion if more unit tests are added later, but that's minor.

[marcospassos]

Sorry, I didn't realize you don't have such privilege.

Other than that, you may want to reset the custom property to the default after the tests covering the property, to prevent confusion if more unit tests are added later, but that's minor.

Nice catch.

[marcospassos]

Done!

[marcospassos]

@gsherwood, did you have time to take a look into this?

[gsherwood]

If I had to document this (which I will need to), would I say that the multilevel option allows for a line in a chained call to either be indented 1 level more or less than the previous line, with the only exception being that lines must be indented at least 1 level from the first line?

So this valid:

<?php
$rootNode
    ->one()
        ->two()
            ->three()
        ->four()
    ->five();

This is not:

<?php
$rootNode
    ->one()
        ->two()
            ->three()
    ->four()
    ->five();

This is not:

<?php
$rootNode
    ->one()
    ->two()
        ->three()
    ->four()
->five();

But is this valid, or does the chain need to close properly?

<?php
$rootNode
    ->one()
        ->two()
            ->three()
        ->four()
        ->five();

[marcospassos]

@gsherwood thanks for reviewing it.

If I had to document this (which I will need to), would I say that the multilevel option allows for a line in a chained call to either be indented 1 level more or less than the previous line, with the only exception being that lines must be indented at least 1 level from the first line?

Exactly, this precisely describes the current implementation.

I believe the current implementation covers the most common use cases. The widespread use case is balanced method calls, such as open() and close(), start() and end(), etc. Another common use case is nested builders that requires calling parent() to return to the parent builder, which is also covered in the current implementation. The last case you mentioned is valid to support cases where the chained calls are not necessarily balanced as shown in the following snippet:

$writer
    ->writeln("First line")
    ->indent()
        ->writeln("Indent level 1")
        ->indent()
            ->writeln("Indent level 2")

As a side note, I've been using it for two months now in different projects with no issues so far.

[marcospassos]

@gsherwood are there any issues still pending in this PR?

[gsherwood]

are there any issues still pending in this PR?

No, sorry. Just haven't had time to look at this yet.

[gsherwood]

Sorry this took so long to merge, but it is in now and will be released in 3.5.0. Thanks a lot for this PR.