Preserve indentation from previous PHP block

[stronk7]

Right now all the T_OPEN/T_CLOSE tokens lead, unconditionally,
to the indentation being reset to 0 to their level. In general, it works ok, but
there are standards that could prefer to enforce multiple blocks
indentation a bit harder.

There are situations where it may be desired to continue
checking indentation in a PHP block at the same level
the previous PHP block ended with. A classical example is
when you evaluate a condition in PHP, end the block, produce some
conditional output/html, just to continue in the next PHP block,
closing the condition scope or whatever.

This (opt-in) new setting (observePreviousPhpBlockIndentation) tries
to enforce that by looking for the indentation in the leaving block
when there are open scopes and applying it as default indentation in
the next incoming block. When there are not open scopes, there is no
restriction.

Covered with tests, the best I was able to imagine, adding a fraction of
ScopeIndentUnitTest.1.inc fixture to ensure it does not break normal
operations. I was not sure about the best way to cover that.

FYC, Ciao :-)

[gsherwood]

My initial thought when I read this PR was: why wouldn't this just be made the default setting?

But the unit tests brought up one really really edge case:

<?php
if ($foo) {
    foreach ($bar as $baz) {
        if ($baz) {
            ?>
            <div>
                <div>
                    <div>
                    <?php
                    if ($baz > 1) {
                        echo '1';
                    }
                    ?>
                    </div>
                    <?php
                    if ($baz > 1) {
                        echo '2';
                    }
                    ?>
                </div>
                <?php
                if ($baz > 1) {
                    echo '2';
                }
                ?>
            </div>
            <?php
        }
    }
}

The last IF statement (the one with echo '3') gives an error with your code because the open tag reuses the indentation from the previous IF statement, and asks for 20 spaces instead of the 16 if currently uses.

Having this as an option really feels like it might only ever be used by yourself, so I'd really like to figure a way of solving all the problems without developers having to do anything.

The important thing to note first up is that open and close tags do not reset the indent to zero. They reset the indent to where ever they have been indented. This is a very important distinction because it allows the developer to control how the indents work. This also provides you with a quick way of rewriting your unit test to make it clear where PHP code starts and ends.

A really simple example from your tests:

<?php
$a = 10;
if ($condition) {
    $ok = true;
  $bad = true;
      $okNonExact = true;
    // Now we capture current indentation to be used in next php block
    // (because there are open scopes).
?>
<span>here we want some html</span>
<?php
    $ok = true;
  $bad = true;
      $okNonExact = true;
}

The sniff wants you to indent those open/close tags around the span to the same level as the IF body. Well, it's not that it wants you to put there (you can put them where ever you like) but it reads from them to determine what you want.

The way that code should be written to find all possible errors is:

<?php
$a = 10;
if ($condition) {
    $ok = true;
  $bad = true;
      $okNonExact = true;
    // Now we capture current indentation to be used in next php block
    // (because there are open scopes).
    ?>
<span>here we want some html</span>
    <?php
    $ok = true;
  $bad = true;
      $okNonExact = true;
}

What I'm wondering is: is this code snippet from the tests the only case that needs a solution or do you have some real-world code that is more complex and moving the open and close tags wouldn't fix things?

The reason I ask is that by looking at the unit test, it appears the problem is not actually about copying indentation from previous blocks, but rather the preference you have for placing open and close tags. The copy just allows you to ignore the tags and put them where ever you want and have the sniff ignore them, which might be achieved by just directly asking the sniff to ignore open and close tags from indentation. But I realise you are only doing this when the indent has reduced, so it's a very specific case.

[stronk7]

Hi, thanks for the promptly response.

First of all, yes, let me agree that the "to 0" part of my "being reset to 0" comment is absolutely inaccurate. open/close tags reset the indentation to their own one. In other words, they root the indentation of upcoming code. I've amended the commit message to clarify that point.

Said that, the idea beyond the patch is really simple and it's to offer an alternative way to decide who roots the indentation, the open/close tags (current behavior) or the code indentation itself when there is any indentation already present (aka, there are open scopes).

In other words, to decide who wins each time a new php block is started. And that's exactly what the code is doing, with your edge case example:

<?php                                  // block 0,  code 0 => block wins (taken as base)
if ($foo) {                            // base 0,  code 4
    foreach ($bar as $baz) {           //          code 8
        if ($baz) {                    //          code 12
            ?>                         // There are opened scopes, annotate code 12
            <div>
                <div>
                    <div>
                    <?php              // block 20, annotated 12 => block wins (taken as base)
                    if ($baz > 1) {    // base 20, code 20
                        echo '1';      //          code 24
                    }                  //          code 20
                    ?>                 // There are opened scopes, annotate code 20
                    </div>
                    <?php              // block 20, annotated 20 (deuce) => block wins (taken as base)
                    if ($baz > 1) {    // base 20, code 20
                        echo '2';      //          code 24
                    }                  //          code 20
                    ?>                 // There are opened scopes, annotate code 20
                </div>
                <?php                  // block 16, annotated 20 => code wins (taken as base)
                if ($baz > 1) {        // base 20, code 16 => ERROR (< expected)
                    echo '3';          //          code 20 (with consecutive errors it should be ERROR too)
                }                      //          code 16 (with consecutive errors it should be ERROR too)
                ?>                     // There are opened scopes, annotate code 16
            </div>
            <?php                      // block 12, annotated 16 => code wins (taken as base)
        }                              // close scope reset indent to 8, nothing to check
    }                                  // close scope reset indent to 4, nothing to check
}                                      // close scope reset indet to 0, nothing to check

So, I'd say the behavior is the expected, open tag indentation continues winning if bigger than code indentation (those are code blocks "1" and "2") and code indentation wins if bigger than open tag indentation (that's the situation in code block "3").

Note that, initially I was more radical and the switch was intended to, unconditionally, give precedence to code indentation over open tags and maybe that way it's more clear to understand. By getting rid of this condition in code then code indentation wins always and your "1" code block will fail immediately (and also "2" and "3", all them . Later I relaxed it because found it too hard (strict), specially when there are other indentation checks here and there that do allow an excess of indentation when operating in non-strict mode. And that's the ultimate cause to allow open tags to continue winning sometimes.

About your suggestion to change the test by aligning the 2nd open tag with code is ok, but the point is that, with the new option enabled, the open tag does not root/win anymore so really it doesn't matter if its indentation is 0 or 4. That gives developers the freedom to write them as they want (I've seen code out there where all open/close tags are always at 1st column, irrespectively of how code is indented within them. So, basically, any indentation between 0 and the code indentation is ok. If they exceed the code indentation, then open/close tags win and get the baton, as commented in the edge case.

Finally, yes, I understand that moving the open/close tags around in a solution not requiring any new setting/option. Just we have a huge codebase (Moodle) with lots of plugins (> 1000) out there and it's sort of non-rare practice to observe code indentation, specially for theme plugins where is usual to mix multiple PHP blocks, without forgetting other parts/old core code still using that sort of mix here and there (sure this will impact us less and less while we convert everything to renewed output).

Just recently we provided a couple of interesting integrations with travis (both for core and plugins authors) and also our CI servers... and all them, using our "moodle" standard, do cry here and there because of this issue. Until now it has been maintained in a patched copy of the Generic Sniff. But recently we upgraded to the newer 2.5.x and I though it may be interesting for others as an opt-in, non intrusive way to allow code indentation to get precedence over open/close tags indentation.

Said that, sincerely... don't worry. If you are not convinced that it can be useful for others (for sure it's not useful as default option, IMO), I can move back to our patched copy strategy. NP at all here, at all!

Thanks in advance for your attention and for phpcs, of course!

Ciao :-)

[stronk7]

Hi,

I've evolved this PR including extra commit that changes current "varying" behavior of the setting (only applied when it's smaller than open/close tags indentation) to be stricter and, possibly, more consistent, by making it to always apply previous PHP block indentation, as far as there are open scopes.

It also adds the "edge case" commented above, now with all the blocks (1, 2, 3) failing, no matter the open/close tags indentation.

Maybe it's better that way, with expected outcome (PHP indentation wins) always being observed. Thoughts?

Ciao :-)

[stronk7]

As commented in #945 we finally decided to go ahead with your upstream sniff behavior (php tags root indentation levels) and abandon our own (indentation levels must be kept over php blocks).

So we don't need the adapted behavior of this proposal anymore. So, if you don't think it has any interest for other people, I'm happy if this is closed won't fix.

Thanks for your support! Ciao :-)

[gsherwood]

So, if you don't think it has any interest for other people, I'm happy if this is closed won't fix.

I think it is still valuable. I just haven't had the time to look into it properly again. It's on my list though.

[gsherwood]

I've been looking at this for a few hours now and I can't figure out how to make it work in a way that I think is useful enough for people to turn it on. Essentially, turning on this flag dramatically changes the way mixed PHP and HTML is indented. In some cases, it makes the indentation worse because it forces the PHP code to be indented less than the HTML code, so the flow of the template file is lost.

So I've decided to not merge this in, even though I did really want to make it work. There are cases where this makes perfect sense, but you can't selectively apply this just to some pieces of code. If you turn it on, it needs to provide a consistent clean look for all types of mixed PHP/HTML blocks or I'm going to get bug reports about it.

Thanks for taking the time to dev and test this PR though. I know you're not in need of this code anymore, but it's still a shame I couldn't figure this one out.

[stronk7]

Thanks Greg!