preg_replace replacement with variable must be wrapped in addcslashes

[kkmuffme]

Is your feature request related to a problem?

Simplistic example:

$text = 'The product is free';
$price = '$0.5';

echo preg_replace( '/is free/', 'costs ' . $price, $text ) . PHP_EOL;
// wrong:
// The product costs is free.5
echo preg_replace( '/is free/', addcslashes( 'costs ' . $price, '\\$' ), $text ) . PHP_EOL;
// correct:
// The product costs $0.5

In practice anytime you use a variable as the replacement, if it contains \ or $ followed by a number, preg replace will convert this to a back replacement group.
Since this is an extremely rare occurence, most people don't know that this can possibly happen or aren't aware.

Describe the solution you'd like

If a preg_replace call uses a variable as 2nd argument, the whole replacement arg or each variable in it must be wrapped in addcslashes( $variable, '\\$' ) to fix this issue.

Additional context (optional)

If this rule is something you think is better added in another phpcs ruleset, please let me know.

• I intend to create a pull request to implement this feature.

[jrfnl]

@kkmuffme Thanks for opening this issue. That's a nasty one and you do have a point.

Since this is an extremely rare occurrence, most people don't know that this can possibly happen or aren't aware.

I'm not sure the overhead of always requiring addcslashes() for all preg_replace() calls containing a variable in the $replacement parameter is the right solution for something which, by your own admission, will be pretty rare in practice.

I mean, when such a sniff would be used, the only way to "solve" the issue (aside from ignoring it) is to add the function call, even when it is not needed as you know the variable used in safe, which it will probably be in > 95% of all cases.

All in all, I'm not adverse to such a sniff, but I consider it low priority and up for grabs if someone wants to work on it.

---

Some notes for if/when someone would want to work on this:

• The sniff should only flag $replacement parameters containing a T_VARIABLE token as one of the tokens in the parameter.
• The sniff would need to bow out if parameter unpacking is used preg_replace( $search, ...$otherParams).
• The sniff would need to take into account that the $replacement parameter can either be a string or an array.
  • If the parameter passed is a variable, whether it is a string or an array cannot be determined (based on the function call alone), so this can be flagged, but most definitely not auto-fixed.
  • If the parameter passed is clearly an array (because the array is defined within the parameter), each array entry will need to be examined and, if necessary, flagged individually.
  • If the parameter passed/array item is clearly a string, it should be possible to auto-fix this, with a preference for fixing this by wrapping the whole array item/parameter in a function call to addcslashes() to prevent potentially having to add the function call multiple times if multiple variables are being concatenated together, though the implementation details of addcslashes() should be checked to be sure this is the right fix.
• The sniff would need to take PHP 8.0 named parameters into account. This will also impact any potential auto-fixers.
• The sniff would need to take a replacement array with keys into account. This will also impact any potential auto-fixers.

So, yes, this sniff will be pretty complex to write.