PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
857 stars 53 forks source link

Weird indentation for lines with more complex HTML & PHP code #337

Open montala-alinc opened 6 months ago

montala-alinc commented 6 months ago

Describe the bug

Using PSR12 standard on files with HTML & PHP complex lines get processed resulting with weird indentation.

Code sample

<input type="text" name="username" id="username" class="stdwidth" <?php if (!$login_autocomplete) { ?>autocomplete="off"<?php } ?> value="<?php echo htmlspecialchars(getval("username", "")) ?>" <?php if ($error != "") { ?>aria-describedby="LoginError"<?php } ?>/>

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
    <description>Indentation issue for HTML + PHP</description>
    <rule ref="PSR12"/>
    <rule ref="Generic.PHP.DisallowShortOpenTag.EchoFound">
        <severity>5</severity>
    </rule>
</ruleset>

To reproduce

Steps to reproduce the behavior:

  1. Create a test.php file with the code sample above
  2. Create a phpcs.xml file with the above config
  3. Run phpcs --report=diff test.php
  4. See output
    
    --- test.php
    +++ PHP_CodeSniffer
    @@ -1 +1,5 @@
    -<input type="text" name="username" id="username" class="stdwidth" <?php if (!$login_autocomplete) { ?>autocomplete="off"<?php } ?> value="<?php echo htmlspecialchars(getval("username", "")) ?>" <?php if ($error != "") { ?>aria-describedby="LoginError"<?php } ?>/>
    +<input type="text" name="username" id="username" class="stdwidth" <?php if (!$login_autocomplete) {
    +    ?>autocomplete="off"<?php
    +                                                                  } ?> value="<?php echo htmlspecialchars(getval("username", "")) ?>" <?php if ($error != "") {
    +    ?>aria-describedby="LoginError"<?php
    +                                                                  } ?>/>


## Expected behavior
Lines with a more complex mixture of HTML & PHP should be left alone (preferred) by some of the available sniffs.

The second if statement was indented in line with closing of the first one instead of its open php tag which didn't seem to follow the same logic as the first if block.

## Versions (please complete the following information)

| | |
|-|-|
| Operating System | Ubuntu 22.04
| PHP version | 8.2.15
| PHP_CodeSniffer version | 3.8.1 (stable)
| Standard | PSR12
| Install type | global (/usr/local/bin/phpcs)

## Additional context
I tried to identify if the problem is with the configuration or the rules. As far as I can tell, the above is caused by (in this order):
- Squiz.ControlStructures.ControlSignature
- Squiz.WhiteSpace.ScopeClosingBrace.ContentBefore
- Squiz.WhiteSpace.ScopeClosingBrace
- Generic.WhiteSpace.ScopeIndent

Using ignoreIndentationTokens seems to not have the effect I'm looking for here and reading the code it looks like it has a different purpose (e.g have my own sniff that will target those tokens later).

Is this something that PHPCS should address or is it simply considered bad practice?

## Please confirm:

- [x] I have searched the issue list and am not opening a duplicate issue.
- [x] I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
- [x] I have verified the issue still exists in the `master` branch of PHP_CodeSniffer.
jrfnl commented 6 months ago

@montala-alinc While I agree this behaviour is unexpected, I'm not sure it is a bug in PHPCS.

PSR12 is pretty clear about how control structure braces should be formatted and as far as I can see, this is handled correctly by the sniffs, though it could be argued that the start of the HTML should be taken as the "start of the statement" for the indentation of the close brace, same as it is for the "four space indent" of the control structure body.

I'm not sure I agree though.

Is this something that PHPCS should address or is it simply considered bad practice?

I do feel that inlining braced control structures as per the code sample is bad practice (and quite unusual) and there are plenty of other ways to write this code which would comply with PSR12 without problems (and would in most cases make the code more readable too).

Here are some examples:

// Example 1: use a ternary.
<input type="text" name="username" id="username" class="stdwidth" <?php echo (!$login_autocomplete) ? 'autocomplete="off"' : ''; ?> value="<?php echo htmlspecialchars(getval("username", "")) ?>" <?php echo ($error != "") ? 'aria-describedby="LoginError"' : ''; ?>/>

// Example 2: use a ternary with the short echo tag.
<input type="text" name="username" id="username" class="stdwidth" <?= (!$login_autocomplete) ? 'autocomplete="off"' : ''; ?> value="<?= htmlspecialchars(getval("username", "")) ?>" <?= ($error != "") ? 'aria-describedby="LoginError"' : ''; ?>/>

// Example 3: verbosely declare variables before creating the HTML.
<?php
$autocomplete = '';
if (!$login_autocomplete) {
    $autocomplete = 'autocomplete="off"';
}

$describedBy = '';
if ($error != "") {
    $describedBy = 'aria-describedby="LoginError"';
}
$userName = htmlspecialchars(getval("username", ""));
?>
<input type="text" name="username" id="username" class="stdwidth" <?php echo $autocomplete ?> value="<?php echo $userName ?>" <?php echo $describedBy ?>/>

// Example 4: split the HTML output across several lines.
<input type="text" name="username" id="username" class="stdwidth"
<?php
if (!$login_autocomplete) {
    ?>
    autocomplete="off"
    <?php
}
?>
value="<?php echo htmlspecialchars(getval("username", "")) ?>"
<?php
if ($error != "") {
    ?>
    aria-describedby="LoginError"
    <?php
}
?>
/>
montala-alinc commented 6 months ago

it could be argued that the start of the HTML should be taken as the "start of the statement" for the indentation of the close brace, same as it is for the "four space indent" of the control structure body.

Indeed, the unnecessary indentation was what caught my attention in the first place.

The code base is from around 2010 and is slowly being brought up on modern practices so I'm aware this is not typical code nowadays.