squizlabs / PHP_CodeSniffer

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

Multiline ternaries not being indented correctly #3711

Open paigedwight opened 2 years ago

paigedwight commented 2 years ago

Describe the bug phpcbf will format multiline ternaries to the same level of indentation, instead of having the lines after the first indented an additional level.

Code sample

<?php

$x = $a === $b
    ? true
    : false;

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
    <rule ref="Generic.WhiteSpace.ScopeIndent">
        <properties>
            <property name="exact" value="true" />
        </properties>
    </rule>
</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 2 ERRORS AFFECTING 2 LINES
    -----------------------------------------------------------------------------------------------------------------------
    4 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found 4
    |       |     (Generic.WhiteSpace.ScopeIndent.IncorrectExact)
    5 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found 4
    |       |     (Generic.WhiteSpace.ScopeIndent.IncorrectExact)
    -----------------------------------------------------------------------------------------------------------------------
    PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    -----------------------------------------------------------------------------------------------------------------------

Expected behavior Multiline ternaries should be indented

Versions (please complete the following information):

Additional context n/a

martinjoiner commented 1 year ago

I suggest this issue is a candidate to be closed with no action required.

Multi-line ternaries are poor readability and although PSR-12 - 6.3 Ternary Operators does not explicitly mention multi-line ternaries it certainly doesn't use any in it's examples. I'm sure consensus opinion would be against breaking them onto multiple lines.

If you want a multi-line conditional, use an if-statement or a match expression. If your ternary is too long to fit on 1 line, it should probably be an if-statement or a match expression.

Also, the ternary in your code sample is redundant and can be re-written as:

$x = $a === $b;

Ternaries are elegant for cases where the code blocks are so small they would look silly in an if-statement and would definitely read better were it all on 1 line such as this:

$color = $accountBalance < 0 ? 'red' : 'black';

But they are inappropriate in any other situation.

Only my opinion, I hope you understand. I'm just hoping that with my input, a few low-priority requests can be closed and allow some higher priority ones to get fixed.

ErikBrendel commented 1 year ago

Not indenting them at all makes it worse, though.

The pear style also has examples of indenting them: https://pear.php.net/pepr/pepr-proposal-show.php?id=538#toc5

If this is not officially taken up to get supported, is there a way of automatically disabling the sniff on ternary statements so that the user can at least manually keep indentation here?

jrfnl commented 1 year ago

Not indenting them at all makes it worse, though.

The pear style also has examples of indenting them: https://pear.php.net/pepr/pepr-proposal-show.php?id=538#toc5

If this is not officially taken up to get supported, is there a way of automatically disabling the sniff on ternary statements so that the user can at least manually keep indentation here?

The simple solution here is to set the exact property to false.

The exact property being set to true in the ruleset is the problem, not this sniff.

roslov commented 8 months ago

I also need to have this fixed. exact=false cannot be applied as it ignores other cases to be fixed.

martinjoiner commented 7 months ago

@roslov This has already been marked as wontfix. As I have commended on the issue on your repo, I am yet to see someone present a good use case for multi-line ternaries so I don't think it is a good investment of time for open source maintainers to to support them.