update rule to support multiple occurrences of the class in the string

[dobryy]

Closes #3389

[TomasVotruba]

This looks very crazy 😄 What is going on here?
It's some node stacking in a loop?

Better collect concat nodes in an array, e.g. to $contacts

$concats[] = $concat;

And resolve later in in go, e.g.

if ($concats === []) {
    return ...;
}

return ...;

It's much easier to debug and test in case of troubles.

[dobryy]

This solution helped me to keep myself away from becoming crazy :D Believe me or not but is a million percents better than a couple of the solution I had before this one :D

Maybe it is not clear but this solution is able to process the random amount of class name occurrences in the string (not only one or two) that is why concatenation in the loop is required.

When it comes to the condition it helps to avoid the first empty Node. If I won't use it I'll have to create the first Node with an empty value outside of the loop for first concatenation which will then produce empty node which will in final result look like

'' . \App\ClassName()::class . 'staticCall()'

While it is not a problem to create the array of nodes with the current solution... I can't imagine what I should implement under ... in your code to cover the cases in the test fixtures.

[TomasVotruba]

I see, I think what you mean. Nodes can break human brain sometimes, I've been there :D.
Don't worry about it 👍 , I'll check if there are is some space after we merge this.

[dobryy]

Not sure how much you care about perfomance but to make this loop a bit nicer I can also

+            $returnNode = new String_($part);
+            if (class_exists($part)) {
+                $returnNode = new ClassConstFetch(new FullyQualified(ltrim($part, '\\')), 'class');
+            }
-            if (class_exists($part)) {
-                $returnNode = new ClassConstFetch(new FullyQualified(ltrim($part, '\\')), 'class');
-            } else {
-                $returnNode = new String_($part);
-            }

[TomasVotruba]

Unless a memory/performance leak, it's not an issue as this is CLI app, not a response/request one.

Overriding variable like this is confusing. Personally I'd extract it to own method too. E.g. createReturnNode() with early returns.

But current state is also ok

[dobryy]

I see, I think what you mean. Nodes can break human brain sometimes, I've been there :D.
Don't worry about it , I'll check if there are is some space after we merge this.

Not sure what do you mean by that. Can I leave it as it looks like at the moment (after today's changes)?

[dobryy]

@TomasVotruba I've applied the changes according to the comments but for some reason, checks are failing... locally it looks fine...

[TomasVotruba]

That's Symplify update I made today. I'll fix it.

Thanks for you work!