PHPCSStandards / PHPCSExtra

A collection of code standards for use with PHP_CodeSniffer
GNU Lesser General Public License v3.0
91 stars 8 forks source link

preg_replace replacement with variable must be wrapped in addcslashes #281

Open kkmuffme opened 1 year ago

kkmuffme commented 1 year ago

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.

jrfnl commented 1 year ago

@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:

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