Skip to content
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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 10, 2024
Merged

chore: Clean up Rector skip config on SimplifyRegexPatternRector and RemoveUnusedPromotedPropertyRector #8944

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
260c379
chore: Clean up Rector skip config on SimplifyRegexPatternRector and …
samsonasik Jun 9, 2024
48f36ef
remove SimplifyRegexPatternRector from rule()
samsonasik Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions rector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
use Rector\CodeQuality\Rector\Expression\InlineIfToExplicitIfRector;
use Rector\CodeQuality\Rector\Foreach_\UnusedForeachValueToArrayKeysRector;
use Rector\CodeQuality\Rector\FuncCall\ChangeArrayPushToArrayAssignRector;
use Rector\CodeQuality\Rector\FuncCall\SimplifyRegexPatternRector;
use Rector\CodeQuality\Rector\FuncCall\SimplifyStrposLowerRector;
use Rector\CodeQuality\Rector\FuncCall\SingleInArrayToCompareRector;
use Rector\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector;
Expand All @@ -36,7 +35,6 @@
use Rector\Config\RectorConfig;
use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedConstructorParamRector;
use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedPrivateMethodRector;
use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedPromotedPropertyRector;
use Rector\DeadCode\Rector\If_\UnwrapFutureCompatibleIfPhpVersionRector;
use Rector\EarlyReturn\Rector\Foreach_\ChangeNestedForeachIfsToEarlyContinueRector;
use Rector\EarlyReturn\Rector\If_\ChangeIfElseValueAssignToEarlyReturnRector;
Expand Down Expand Up @@ -107,11 +105,6 @@

YieldDataProviderRector::class,

RemoveUnusedPromotedPropertyRector::class => [
// Bug in rector 1.0.0. See https://github.com/rectorphp/rector-src/pull/5573
__DIR__ . '/tests/_support/Entity/CustomUser.php',
],

RemoveUnusedPrivateMethodRector::class => [
// private method called via getPrivateMethodInvoker
__DIR__ . '/tests/system/Test/ReflectionHelperTest.php',
Expand Down Expand Up @@ -155,8 +148,6 @@
// use mt_rand instead of random_int on purpose on non-cryptographically random
RandomFunctionRector::class,

SimplifyRegexPatternRector::class,
Copy link
Member

@kenjis kenjis Jun 10, 2024 •
edited
Loading

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

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Member Author

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

Copy link
Member

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.

  1. Meaning:

    • \d matches any digit character, including Unicode digits in some cases.
    • [0-9] matches only ASCII digits (0-9).

  2. Compatibility:

    • \d can match non-ASCII digits if Unicode is enabled.
    • [0-9] always matches ASCII digits only.

  3. 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.

Copy link
Member

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?

Copy link
Member Author

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.


// PHP 8.0 features but cause breaking changes
ClassPropertyAssignToConstructorPromotionRector::class => [
__DIR__ . '/system/Database/BaseResult.php',
Expand Down Expand Up @@ -221,7 +212,6 @@
$rectorConfig->rule(ChangeArrayPushToArrayAssignRector::class);
$rectorConfig->rule(UnnecessaryTernaryExpressionRector::class);
$rectorConfig->rule(RemoveErrorSuppressInTryCatchStmtsRector::class);
$rectorConfig->rule(SimplifyRegexPatternRector::class);
$rectorConfig->rule(FuncGetArgsToVariadicParamRector::class);
$rectorConfig->rule(MakeInheritedMethodVisibilitySameAsParentRector::class);
$rectorConfig->rule(SimplifyEmptyArrayCheckRector::class);
Expand Down