chore: Clean up Rector skip config on SimplifyRegexPatternRector and RemoveUnusedPromotedPropertyRector#8944
Conversation
…RemoveUnusedPromotedPropertyRector
|
Ready to merge 👍 |
| // use mt_rand instead of random_int on purpose on non-cryptographically random | ||
| RandomFunctionRector::class, | ||
|
|
||
| SimplifyRegexPatternRector::class, |
There was a problem hiding this comment.
I don't like the SimplifyRegexPatternRector rule. Because it may change the behavior.
In PHP, [0-9] and \d (also [a-zA-Z] and \w) are not exactly the same when using u option.
https://3v4l.org/N8sQ5
There was a problem hiding this comment.
Ok, it seems due to called on $rectorConfig->rule(), I removed it and revert its change on tests files 48f36ef
There was a problem hiding this comment.
btw, the u usage already skipped on rector :) https://getrector.com/demo/00372e82-e4bc-451b-a7a2-495637f29d88
There was a problem hiding this comment.
@kenjis do you want to re-enable SimplifyRegexPatternRector ? as it already skipped on u usage, see https://getrector.com/demo/00372e82-e4bc-451b-a7a2-495637f29d88
There was a problem hiding this comment.
I don't want. Because these patterns have a bit different meanings.
An answer by ChatGPT:
In PHP's PCRE, \d and [0-9] both match digits but are not exactly the same.
-
Meaning:
\dmatches any digit character, including Unicode digits in some cases.[0-9]matches only ASCII digits (0-9).
-
Compatibility:
\dcan match non-ASCII digits if Unicode is enabled.[0-9]always matches ASCII digits only.
-
Performance:
[0-9]can be slightly faster since it's a simple character class.
Example
$pattern_d = '/\d/';
$pattern_class = '/[0-9]/';
$string = "123abc";
preg_match($pattern_d, $string); // Matches
preg_match($pattern_class, $string); // MatchesConclusion
Use [0-9] for consistent matching of ASCII digits. Use \d if you need to match broader digit sets, considering encoding and Unicode support.
There was a problem hiding this comment.
By the way, where does this code https://github.com/rectorphp/rector/blob/main/rules/CodeQuality/Rector/FuncCall/SimplifyRegexPatternRector.php check for the u option?
There was a problem hiding this comment.
It only check delimiter, which limited to ~, #, / via RegexPatternDetector, so u in the last will always be skipped.
|
@kenjis I am ok for performance reason then, thank you for the review, let's merge ;) |
Description
SimplifyRegexPatternRectornot usedRemoveUnusedPromotedPropertyRectorbug already resolved on latest rector.Checklist: