Add option to `ternary_operator_spaces` to fix to separate lines

[mvorisek]

Feature request

Code like:

$res = $cond ? $o->a() : $o->b();

to be fixed to:

$res = $cond
    ? $o->a()
    : $o->b();

The motivation is line coverage. I do not think this CS should be default, but I would be grateful if an option for formatting on a separate lines would exist, so projects with strong coverage requirements can be measured better.

[bachinblack]

By the way, there seem to be no fixers that would handle the indentation of a ternary operation, is there ? If not, should it be covered by statement_indentation ?

[Wirone]

@bachinblack yeah, seems like a good place for that 🙂. Standalone fixer would be an overkill.

[github-actions[bot]]

Since this issue has not had any activity within the last 90 days, I have marked it as stale.

The purpose of this action is to enforce backlog review once in a while. This is mostly for maintainers and helps with keeping repository in good condition, because stale issues and PRs can accumulate over time and make it harder for others to find relevant information. It is also possible that some changes has been made to the repo already, and issue or PR became outdated, but wasn't closed for some reason. This action helps with periodic review and closing of such stale items in automated way.

You may let maintainers handle this or verify current relevancy by yourself, to help with re-triage. Any activity will remove stale label so it won't be automatically closed at this point.

I will close it if no further activity occurs within the next 30 days.

[christian-kolb]

I would love to have this. For code coverage, but more importantly for readability 🙂 Can anyone recommend an existing fixer (or multiple ones) that is close to what is needed here as examples? Then I would give it a shot to provide one as a pull request.

[Wirone]

@christian-kolb probably operator_linebreak is close enough, maybe multiline_whitespace_before_semicolons. In general, you can look for a "multiline" in a docs.

[christian-kolb]

@Wirone Is there any kind of documentation on the structure and inner workings of the fixers? I've tried with a timebox but only copied stuff from other fixers without really understanding the dependencies of the moving parts. And basically everything that is necessary is marked as internal. Very challenging to get into that.

[Wirone]

@christian-kolb you can use anything within this repo, including internal parts. These are marked as internal only to exclude it from public API so people don't build external stuff around it (or do it on their own responsibility). Unfortunately we don't have developer docs, so you basically need to learn from existing code.

[christian-kolb]

@Wirone Thanks for the quick reply. Unfortunately my timebox to tackle that topic ran out. I might tackle in the future, but can't work on it any further for now.

If anyone else is interested, that's my current version. It does work for the very simpelst parts (simple assignment of a variable at the start of a function), but not for 80% of my codebase yet.

<?php

declare(strict_types=1);

namespace App\CSFixer;

use PhpCsFixer\Fixer\FixerInterface;
use PhpCsFixer\Fixer\WhitespacesAwareFixerInterface;
use PhpCsFixer\FixerDefinition\CodeSample;
use PhpCsFixer\FixerDefinition\FixerDefinition;
use PhpCsFixer\FixerDefinition\FixerDefinitionInterface;
use PhpCsFixer\Tokenizer\Analyzer\AlternativeSyntaxAnalyzer;
use PhpCsFixer\Tokenizer\Analyzer\GotoLabelAnalyzer;
use PhpCsFixer\Tokenizer\Analyzer\SwitchAnalyzer;
use PhpCsFixer\Tokenizer\Analyzer\WhitespacesAnalyzer;
use PhpCsFixer\Tokenizer\Tokens;
use PhpCsFixer\WhitespacesFixerConfig;

final class TernaryOperatorMultilineFixer implements FixerInterface, WhitespacesAwareFixerInterface
{
    public const string NAME = 'Namespace/ternary_operator_multiline';

    /**
     * @var WhitespacesFixerConfig
     */
    private $whitespacesConfig;

    public function isRisky(): bool
    {
        return false;
    }

    public function getDefinition(): FixerDefinitionInterface
    {
        return new FixerDefinition(
            'Ternary operators must always be used as multiline.',
            [
                new CodeSample(
                    '<?php
$value = $condition
    ? $foo
    : $bar;
',
                ),
            ],
        );
    }

    public function getName(): string
    {
        return self::NAME;
    }

    /**
     * {@inheritdoc}
     *
     * Must run after ArraySyntaxFixer, ListSyntaxFixer, TernaryToElvisOperatorFixer.
     */
    public function getPriority(): int
    {
        return 1;
    }

    // Default
    public function supports(\SplFileInfo $file): bool
    {
        return true;
    }

    // Default
    public function setWhitespacesConfig(WhitespacesFixerConfig $config): void
    {
        $this->whitespacesConfig = $config;
    }

    public function isCandidate(Tokens $tokens): bool
    {
        return $tokens->isAllTokenKindsFound(['?', ':']);
    }

    public function fix(
        \SplFileInfo $file,
        Tokens $tokens,
    ): void {
        $alternativeSyntaxAnalyzer = new AlternativeSyntaxAnalyzer();
        $gotoLabelAnalyzer = new GotoLabelAnalyzer();
        $ternaryOperatorIndices = [];

        foreach ($tokens as $index => $token) {
            if (!$token->equalsAny(['?', ':'])) {
                continue;
            }

            if (SwitchAnalyzer::belongsToSwitch($tokens, $index)) {
                continue;
            }

            if ($alternativeSyntaxAnalyzer->belongsToAlternativeSyntax($tokens, $index)) {
                continue;
            }

            if ($gotoLabelAnalyzer->belongsToGoToLabel($tokens, $index)) {
                continue;
            }

            $ternaryOperatorIndices[] = $index;
        }

        foreach (array_reverse($ternaryOperatorIndices) as $index) {
            $token = $tokens[$index];

            if ($token->equals('?')) {
                $nextNonWhitespaceIndex = $tokens->getNextNonWhitespace($index);
                if ($tokens[$nextNonWhitespaceIndex]->equals(':')) {
                    // Ignore ?: syntax
                    continue;
                }

                $previousNonWhitespaceIndex = $tokens->getPrevNonWhitespace($index);

                $tokens->ensureWhitespaceAtIndex(
                    $index - 1,
                    1,
                    sprintf(
                        '%s%s%s',
                        $this->whitespacesConfig->getLineEnding(),
                        WhitespacesAnalyzer::detectIndent($tokens, $previousNonWhitespaceIndex),
                        $this->whitespacesConfig->getIndent()
                    ),
                );

                continue;
            }

            if ($token->equals(':')) {
                $previousNonWhitespaceIndex = $tokens->getPrevNonWhitespace($index);
                if ($tokens[$previousNonWhitespaceIndex]->equals('?')) {
                    // Ignore ?: syntax
                    continue;
                }

                $tokens->ensureWhitespaceAtIndex(
                    $index - 1,
                    1,
                    sprintf(
                        '%s%s',
                        $this->whitespacesConfig->getLineEnding(),
                        WhitespacesAnalyzer::detectIndent($tokens, $previousNonWhitespaceIndex),
                    ),
                );
            }
        }
    }
}

[mvorisek]

The PR should support ?: as well as ?? - via an configurable option and probably disabled by default, as the syntax for these is not very often multiline by default.

[Wirone]

@mvorisek I don't agree. I don't see a point in making ?: multiline, and ?? is not a ternary, so it does not belong to such a fixer (under the name related to ternary).

@christian-kolb sorry to hear that. I understand you worked on it in your work time and had some specific timeframe, but maybe you could continue in your free time 🙂? Anyway, thanks for your work.

[mvorisek]

@mvorisek I don't agree. I don't see a point in making ?: multiline, and ?? is not a ternary, so it does not belong to such a fixer (under the name related to ternary).

Given this issue is motivated with improved CS but also better coverage measure, it is closely related and the coverage motivation applies to ?? as well.

[christian-kolb]

@Wirone Good that you ask, I wasn't clear enough. I do intend on finishing it. But due to my family with two small children and 3 open source packages (which I'm primarily maintaining), there won't be much time (which it needs to get into the complex code base without documentation) 🙂. So I will still try to finish it, but I don't want "block" anyone else from trying their hand on it expecting, that I'll have it finished within the next few days or weeks. That's also the reason I copied my version in here, so that whoever might take it over, doesn't have to start from scratch.

@mvorisek I guess it's personal taste, but I wouldn't use those just for the reduced readability (in my opinion). So when I'll finish it, I won't include the configuration in the first version. Maybe in future ones, but I would at least do it step by step. First making it work with the "normal" ternary operator then perhaps improving it in the future. Please don't take it as bashing on your idea, just trying to manage expectations 🙂.

[github-actions[bot]]

Since this issue has not had any activity within the last 90 days, I have marked it as stale.

The purpose of this action is to enforce backlog review once in a while. This is mostly for maintainers and helps with keeping repository in good condition, because stale issues and PRs can accumulate over time and make it harder for others to find relevant information. It is also possible that some changes has been made to the repo already, and issue or PR became outdated, but wasn't closed for some reason. This action helps with periodic review and closing of such stale items in automated way.

You may let maintainers handle this or verify current relevancy by yourself, to help with re-triage. Any activity will remove stale label so it won't be automatically closed at this point.

I will close it if no further activity occurs within the next 30 days.

[christian-kolb]

This topic is still very relevant, it's just that nobody tackled it yet 🙂

[github-actions[bot]]

Since this issue has not had any activity within the last 90 days, I have marked it as stale.

The purpose of this action is to enforce backlog review once in a while. This is mostly for maintainers and helps with keeping repository in good condition, because stale issues and PRs can accumulate over time and make it harder for others to find relevant information. It is also possible that some changes has been made to the repo already, and issue or PR became outdated, but wasn't closed for some reason. This action helps with periodic review and closing of such stale items in automated way.

You may let maintainers handle this or verify current relevancy by yourself, to help with re-triage. Any activity will remove stale label so it won't be automatically closed at this point.

I will close it if no further activity occurs within the next 30 days.

[christian-kolb]

This topic is still very relevant, it's just that nobody tackled it yet 🙂

[github-actions[bot]]

Since this issue has not had any activity within the last 90 days, I have marked it as stale.

The purpose of this action is to enforce backlog review once in a while. This is mostly for maintainers and helps with keeping repository in good condition, because stale issues and PRs can accumulate over time and make it harder for others to find relevant information. It is also possible that some changes has been made to the repo already, and issue or PR became outdated, but wasn't closed for some reason. This action helps with periodic review and closing of such stale items in automated way.

You may let maintainers handle this or verify current relevancy by yourself, to help with re-triage. Any activity will remove stale label so it won't be automatically closed at this point.

I will close it if no further activity occurs within the next 30 days.

[christian-kolb]

This topic is still very relevant, it's just that nobody tackled it yet 🙂

[mvorisek]

closing as I am against any bot noise - help welcomed

[christian-kolb]

Is it because I copied the answer I gave last time? The bot noice is coming from the the project, not from me 🙂

[mvorisek]

@christian-kolb definitely nothing personal, actually thanks ❤, I just do not agree with https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/v3.73.1/.github/workflows/issues_and_pull_requests.yml workflow. Valid issues should never go stale IMO and I do not want to get any notifications except only meaningful communication to move things forward. I expressed this multiple times to the maintainers, but ATM all I can do is to respect their decision.