String interpolation: don't suggest sprintf()

[derrabus]

Our coding standard currently forbids string interpolation and recommends sprintf() or concatenation as alternatives. However, in PHP 7 string interpolation has been optimized and might outperform both alternatives on a hot path.

Of course, we're talking about micro-optimization here and I can totally understand that projects want to stick to sprintf() for the sake of consistency. But I believe that the general recommendation to avoid string interpolation is not up to date anymore.

My proposal is to allow double-quoted interpolated strings.

Update: The PR now removes the suggestion to use sprintf() instead of interpolation.

[Ocramius]

Disagree. The point of disallowing string interpolation is not performance, but maintainability.

While looking for replacements in HEREDOC may sometimes be OK, starting a hunt for variable usages in strings is not.

Overall, string interpolation is to be avoided, IMO.

[greg0ire]

@Ocramius do you mean that if we enable this, you fear that you are going to have to check every string for variable usages? Because you will only have to do that for strings starting with double quotes? But maybe what you are pointing at is that you don't know how many variables there could be in the string?

[Ocramius]

Let me rephrase: while reading code (human) or analyzing code (machine), I don't want to interpret strings as segments of code that represent anything but constants.

Having to interpret strings as portions of code that include evaluation ("foo $bar baz" and "foo ${bar} ${baz()}") reduces clarity, and requires more effort.

If language features could be removed from PHP, interpolation would certainly be one that should go 👍

This CS ruleset permitted this simplification thus far, and I've been enjoying an interpolation-free ecosystem for a while: let's not reverse that 😁

[Ocramius]

BTW "foo " . $bar is fine: clear distinction between constant and expression 👍

[derrabus]

The point of disallowing string interpolation is not performance, but maintainability.

I didn't question that. But back when the rule was established, avoiding string interpolation did not have a negative performance impact. Now it does.

Overall, string interpolation is to be avoided, IMO.

Okay, but should it still be forbidden?

Having to interpret strings as portions of code that include evaluation ("foo $bar baz" and "foo ${bar} ${baz()}") reduces clarity, and requires more effort.

I agree and this is what stopped me from suggesting a rule that converts all sprintf() to interpolated strings basically. 😅

Even after my change, double quoted strings would be converted to single quoted ones, so when you actually encounter double quotes, you know that you have to look for interpolation or escape sequences. Also, modern IDEs are pretty good at highlighting interpolation.

Alternatively, can we at least remove the recommendation to use sprintf()?

[Ocramius]

should it still be forbidden?

Yes, please 🙏

Alternatively, can we at least remove the recommendation to use ssprintf()?

Works for me 👍

[derrabus]

The default error message is:

Variable "%s" not allowed in double quoted string; use concatenation instead

No need to override that.

[Ocramius]

Should the block be kept, or is it just for specifying <message/>?

[derrabus]

If was needed for overriding the message. If we don't override the message, the line directly above should be enough:

<rule ref="Squiz.Strings.DoubleQuoteUsage"/>

[derrabus]

Alternatively, can we at least remove the recommendation to use ssprintf()?

Works for me 👍

PR updated. I'm using the default error message now. The new tests are still useful though, so I kept them.

[Ocramius]

Should the block be kept, or is it just for specifying <message/>?

[greg0ire]

Thanks @derrabus !