Evaluate replacing all usage of sprintf by dot concatenation or variable inside string

Replacing sprintf() is exceptions that are not at all part of the regular runtime is not going to improve performance. Also there are cases where sprintf() improves readability and there are probably cases where it does not. With respect to exception messages I think we should prioritise the reader over the computer.

Those two articles are not very convincing with PHP 8. They are also not exactly authoritative sources. One of them got called out for doing performance analysis while having xdebug enabled, and the other was written by a CS student 12 years ago. But the main thing is that they were testing using PHP 5. Things have changed quite a lot in PHP land in the meantime.

I just did a test concatenating two strings a million times with PHP 8.1.8: with sprintf() this took 0.046515 seconds, while with the concatenation operator it took 0.024491 seconds. So we can potentially gain a whopping 22 milliseconds, at least if we manage to eliminate 1 million calls to sprintf() from the request.

Probably the only code path we have that gets potentially called a million times in a single request is the renderer. I had a look at it and it is not using sprintf() right now (except in exception messages).

So this is probably not going to yield us any significant performance gains.

I think the hypothesized performance aspect of this is needless micro-optimization. (That said, the addition to the call stack is unnecessary.)

The reason not to use sprintf() -- and the reason we've avoided it historically -- is that it adds absolutely no value over simply using a double-quoted string. Compare:

1. "A string with $bar, {$foo['a']}, $baz, $a and $c, plus also $b embedded";
   

2. sprintf('A string with %s, %s, %s, %s and %s, plus also %s embedded', $bar, {$foo['a']}, $baz, $a, $c, $b);
   

The first is much closer to natural language. The second is significantly longer and less readable, plus it's easy to lose track of which %s refers to what.

The only debatable case is if something requires concatenation (e.g. a function call is used), and even then, I find sprintf() makes it less readable. In those cases, defining a local variable with the function call (and thus avoiding concatenation in favor of something that reads like natural language) is probably a more desirable alternative than either concatenation or sprintf().

For tests, it's best to go a step further and use literal fixture values instead (we've been trying to move away from randomMachineName() etc. for years), but a double-quoted string is preferable over sprintf() when variables are necessary or as an intermediate step.

Updated the IS to de-emphasize the micro-optimization in favor of the readability aspects.

Fixing typos in the IS.

I've been looking for a rector rule or phpcs sniff for this but can only find the inverse.

• EncapsedStringsToSprintfRector
• Squiz.Strings.DoubleQuoteUsage.ContainsVar
• SlevomatCodingStandard.Strings.DisallowVariableParsing

Whereas phpstorm has the Convert a 'sprintf()' call to string interpolation intention.

For implementation it would be good to find a way to automate this.

The simple cases with bare variable names in #13 are fine for interpolation, but when it comes to interpolating the results of method calls or deep array structures I personally prefer the sprintf style.

To me there is no single best option here and we should have some flexibility on this, and therefore I think this is won't fix.

I added a case 4 for #17, but I really think we should discourage people from using it when it reduces readability (which is most of the time). People use it for antiquated reasons that don't apply for test assertions or exception messages. More often than not, it's an antipattern. (Even in the cases in #17 I personally would also avoid it, but I see people split 50/50 on that so I can see the counter-argument for it.)

We always use sprintf on internal projects, I find it more readable in most cases (when you only have 1 or 2 variables). I agree that the example in the IS is slightly more readable with embedded variables but I agree with #17 that this shouldn't be hard enforced.

I vote for won't fix. We shouldn't gatekeep functions from the standard library.

We "gatekeep" functional PHP code for readability all the time; that's what coding standards are.

Regarding performance, #3343913: Add comments explaining performance improvement in TypedData has a comment contradicting the above (apparently relating to a real performance issue, I guess because that one was in the critical path).

Moving this to the coding standards project where decisions about policies/standards like this live.

Tagging for needing issue summary update as there is a prescribed format for coding standards issues

My 2c, simple variables should use pure string literals with variables as required (ie "Something $wicked this {$way['comes']}") and sprintf if there are expressions or functions sprintf('Something %s this way %s, $wicked, $way->comes())