Support multiline logical conditions in `MultilineWhitespaceBeforeSemicolonsFixer`

[coldic3]

Feature request

In the v3.16 release, the MultilineWhitespaceBeforeSemicolonsFixer rule with strategy new_line_for_chained_calls started to work differently than before. For example for a following class:

class Foo
{
    public function bar(): bool
    {
        return
            $this->foo &&
            $this->bar
        ;
    }
}

semicolon is moved to previous line. I would like to keep previous behavior so I'm requesting for new strategy in multiline_whitespace_before_semicolons fixer, so it can handle semicolons in conditions like above.

---

Please read this.

[kubawerlos]

Fixer multiline_whitespace_before_semicolons is handling wrongly some cases because it confuses "mutliline" with "chained", but in this case it works as expected. See it's description

Forbid multi-line whitespace before the closing semicolon or move the semicolon to the new line for chained calls.

Your example is not a chained call, so it does the first part - removes linebreak.

[Wirone]

@kubawerlos I am not completely sure if you're right... I mean, what should be more important - fixer's name (its intention, that is unchangeable) or description (that is dynamic and can be changed at any point)? I find @coldic3's case totally valid and if I wanted semicolons in new line, I would want it like this for this case too (not only for strict -> chains).

Side note: multiline_whitespace_before_semicolons is not descriptive enough in general. I mean... what's exactly the intention? Putting multiple empty lines before semicolon? 🤷‍♂️ Shouldn't it be rather something like semicolon_placing?

Thinking out loud 😉.

[kubawerlos]

Both name and description are important, if the description says one thing, no one should expect it to do the other.

I agree that the fixer name should be better, like semicolon_position with maybe more options to cover more than chained methods, but that's not related to the bug.

[Wirone]

I didn't say that description is not important, only suggested that name is more important. Description should extend name's intention, in this case name is not about chaining, so description is narrowing rather than extending (if it makes sense 😅).

Maybe we should't convert this bug report into feature request?

[coldic3]

The thing that confused me the most is that multiline_whitespace_before_semicolons worked for all cases up to 3.15. I started facing this problem after I bumped CS Fixer up to 3.16 in my dependencies.

And yeah, would be nice to convert it into a feature request :+1:

[Wirone]

The thing that confused me the most is that multiline_whitespace_before_semicolons worked for all cases up to 3.15

If it worked before, then we should treat it like a bug. I'm going to reopen it so it won't be forgotten.

[jakubtobiasz]

Maybe a new strategy providing the old behavior is a way 🤔.

[Rainrider]

The same is true for multiline string concatenation, with 3.17 it now suggests the following:

$csv = "header_1,header_2,header_3\n"
    ."value_11,value_12,value_13\n"
    ."value_21,value_22,value_23\n"
+   ."value_31,value_32,value_33\n";
-;

[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.

[Wirone]

Revisited this and I am going to change this into feature request - neither in v3.15 and current master we don't have test case for ensuring semicolon being in new line in complex logical conditions using &&. It was working like this before as a side effect rather than planned coverage. Support for it has to be added as a new strategy, but I believe we also need to add support for enabling multiple strategies, like 'strategy' => ['new_line_for_chained_calls', 'new_line_for_complex_conditions'] because currently you can only choose one strategy. It's not the exact solution, but only an idea to have in mind 🙂.

Also, @Rainrider's case could be covered as yet another strategy.

[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.

[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.

[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.

[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.

[github-actions[bot]]

The fact this was automatically closed doesn't mean that the idea got rejected - it simply didn't get any priority for way too long to keep it open. If you're still interested in this, please let as know, we can consider re-opening it.