-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Clean up Rector skip config on SimplifyRegexPatternRector and RemoveUnusedPromotedPropertyRector #8944
chore: Clean up Rector skip config on SimplifyRegexPatternRector and RemoveUnusedPromotedPropertyRector #8944
Conversation
…RemoveUnusedPromotedPropertyRector
Ready to merge 👍 |
@@ -155,8 +149,6 @@ | |||
// use mt_rand instead of random_int on purpose on non-cryptographically random | |||
RandomFunctionRector::class, | |||
|
|||
SimplifyRegexPatternRector::class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, the u
usage already skipped on rector :) https://getrector.com/demo/00372e82-e4bc-451b-a7a2-495637f29d88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
\d
matches any digit character, including Unicode digits in some cases.[0-9]
matches only ASCII digits (0-9).
-
Compatibility:
\d
can 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); // Matches
Conclusion
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
SimplifyRegexPatternRector
not usedRemoveUnusedPromotedPropertyRector
bug already resolved on latest rector.Checklist: