DX: trim trailing whitespaces from multiline string ends in tests

[mvorisek]

finish after #7706

with the 1st commit, it should be easy to review/verify the changes applied in the 2nd commit

multiline_string_to_heredoc is not risky, but no_trailing_whitespace_in_string

example:

<?php
$v = "foo();
  ";

is transformed by multiline_string_to_heredoc into:

<?php
$v = <<<EOD
    foo();
      // <-- notice the two spaces here
    EOD;

and such code is then transformed by no_trailing_whitespace_in_string into:

<?php
$v = <<<EOD
    foo();
// <-- notice trailing spaces are removed by the risky fixer
    EOD;

[keradus]

I find it handy that ' is on next line.
whe I want to modify the code (Eg by adding new line), I do not need to move ', (basically all the same reasoning as behind trailing , in arrays etc

[mvorisek]

This is pure preparation PR for multiline to heredoc conversion (#7706), closing marker will be then on a separate line,..

[keradus]

if only purpose of this PR is to have another PR, both with massive diff, and no benefit of this PR itself, i suggest to skip this PR and focus on 7706 only.

if 7706 is having logic + CS change, split that into 2. if only CS change, ok to have one.

[mvorisek]

if 7706 is having logic + CS change, split that into 2. if only CS change, ok to have one.

Yes, I made this PR to prevent squashing (on merge) pure&large CS changes in #7706.

This PR is not CS, this PR is changing string data. Thus I would be happy if you can be reviewed promtly to prevent conflicts as some parts are made manually.

[Wirone]

@keradus +74,844 −58,592, I'm out 😉. If you want to review such monstrosity at once, then 👍.

[Wirone]

I see the value in #7706 as this make code intended better:

Since whitespace is trimmed on each line (like in ending marker), the outcome is the same, while visually it makes strings to be closer to where they're defined. Obviously I did not look at whole PR and I agree with @julienfalque's comment, but in general it's improvement from my perspective.

[Wirone]

if we do massive CS change for same code lines, it should be one PR, 2 commits - not 2 PRs.

Actually I can agree with this, but it would really be helpful with changes introduced as separate commits, describing the process. To make review easier maybe we can split PR into src and tests?

[mvorisek]

PR can be either squashed, rebased on master, or merged. In both 2 later cases, the landed commits must match the PR commits.

As it is good practice to not commit (large) changes done by fixer together with config changes, we need 2 PRs. So let's please agree on 2 separate PRs and continual improvement :)

[Wirone]

Well, it all depends what is considered as a "single change" from repo's perspective. If it's "changing multiline string to heredoc", then it does not matter that multiple steps were required to achieve it, and maybe it should go into 1 PR but with multiple, meaningful commits. It's @keradus' decision I believe.

[julienfalque]

While I do see the value in standardizing those multiline strings accross the project, I also see the cost of doing this change and I'm really not sure it's worth it. I mean, sure, a lot of strings, especially in tests, are not correctly indented, but is it such a major problem? And if yes, does rewriting all these strings remove completely the problem such as changing the code will be much easier? And if yes, is this gain worth reviewing these changes and creating conflicts with basically any other work in progress? I don't think so.

IMO the goal is legit, but I would perfer making a lot of small steps (e.g. rewrite a string when we need to change its value or some related code) rather than review the massive #7706. Even in this PR, I see strings were we need to introduce some concatenations with "\n"s simply to avoid having actual newlines, which kind of defeats the point of improving code readability to me.