[NodeNameResolver] Check regex start delimiter and contains * on NodeNameResolver to use fnmatch() #4964
Conversation
| { | ||
| public function run() | ||
| { | ||
| return 'Nette\Utils\FileSystem'; |
There was a problem hiding this comment.
the configured test is using Nette\*, this is new fixture to test it :)
| if (fnmatch($classToSkip, $classLikeName, FNM_NOESCAPE)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
here removed and ensure the new fixture working (skipped) as expected.
|
All checks have passed 🎉 @TomasVotruba it is ready for review. |
|
@TomasVotruba @staabm the check regex and fnmatch is still fast enough imo, since it check string start with regex delimiter, or contains * before executing regex. |
samsonasik
changed the title
| @@ -7,5 +7,10 @@ | |||
|
|
|||
| return static function (RectorConfig $rectorConfig): void { | |||
| $rectorConfig | |||
| ->ruleWithConfiguration(StringClassNameToClassConstantRector::class, ['Nette\*', 'Error', 'Exception']); | |||
| ->ruleWithConfiguration(StringClassNameToClassConstantRector::class, [ | |||
| 'Nette\*', | |||
There was a problem hiding this comment.
this is existing example using fnmatch().
| ->ruleWithConfiguration(StringClassNameToClassConstantRector::class, ['Nette\*', 'Error', 'Exception']); | ||
| ->ruleWithConfiguration(StringClassNameToClassConstantRector::class, [ | ||
| 'Nette\*', | ||
| '#^PHPStan\\\\Type#', |
There was a problem hiding this comment.
this is example using regex check.
|
All checks have passed 🎉 @TomasVotruba I think it is ready. |
|
@TomasVotruba @staabm I am going to merge this :) The reason it needs to be kept is this doesn't necessary make downgrade the performance, it still fast, since before check regex, it checked via native string php function first. I will lookup other area for removing regex check instead of introducting BC break for current API :) |
|
I don't say we need to drop any regex stuff.. we should only use it when necessary. I worked only on the areas which hit performance. its fine most of the time |
|
@staabm yes, that checked on last resort, also verified first before check regex, so it still fast, and we still have stable API. |
|
@samsonasik The BC break is ok here, as we've adjusted the used places. The goal to make |
|
@TomasVotruba ok, fine, I created PR for it. |
* Revert "Fix fixture typo (#4967)" This reverts commit 76cf20f. * Revert "[NodeNameResolver] Check regex start delimiter and contains * on NodeNameResolver to use fnmatch() (#4964)" This reverts commit ef6c8bc. * fixture * fixture * [ci-review] Rector Rectify --------- Co-authored-by: GitHub Action <[email protected]>
@TomasVotruba @staabm this is to avoid bc break with check string contains first :)