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.

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

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Jun 9, 2024

Description

  • The SimplifyRegexPatternRector not used
  • The RemoveUnusedPromotedPropertyRector bug already resolved on latest rector.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@samsonasik
Copy link
Member Author

Ready to merge 👍

@@ -155,8 +149,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

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.

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.

@samsonasik
Copy link
Member Author

@kenjis I am ok for performance reason then, thank you for the review, let's merge ;)

@samsonasik samsonasik merged commit 5b3c374 into codeigniter4:develop Jun 10, 2024
10 checks passed
@samsonasik samsonasik deleted the chore-clean-rector-skip-regex branch June 10, 2024 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants