-
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
[Rector] Clean up skip config and re-run Rector #5813
[Rector] Clean up skip config and re-run Rector #5813
Conversation
a6b01b0
to
2888011
Compare
e484f95
to
4f97aac
Compare
All green 🎉 |
Co-authored-by: Mostafa Khudair <59371810+mostafakhudair@users.noreply.github.com>
Co-authored-by: Mostafa Khudair <59371810+mostafakhudair@users.noreply.github.com>
Co-authored-by: kenjis <kenji.uui@gmail.com>
Co-authored-by: kenjis <kenji.uui@gmail.com>
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.
LGTM
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'm going to approve this but I still mildly prefer being able to use an un-namespaced class name as a string: 'InvalidArgumentException'. If there's no way to differentiate those the overall benefit outweighs my perturbation. 😊
system/CodeIgniter.php
Outdated
@@ -830,7 +830,7 @@ protected function startController() | |||
$this->benchmark->start('controller_constructor'); | |||
|
|||
// Is it routed to a Closure? | |||
if (is_object($this->controller) && (get_class($this->controller) === 'Closure')) { | |||
if (is_object($this->controller) && (get_class($this->controller) === Closure::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.
There isn't by chance a way to "exempt core classes without namespaces" is there?
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.
Not yet, it possibly need to have configured, eg skip_native_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.
It actually configurable, but currently need to manually define what need to be skipped https://github.com/rectorphp/rector-src/blob/de3c9b6f90ccc901c135a7d9786c4a9ea772bd4d/rules/Php55/Rector/String_/StringClassNameToClassConstantRector.php#L118
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.
0bd15b7
to
0176bb4
Compare
I am merging it ;) |
Clean up skip config
clean unneded skip
RemoveUnusedPrivateMethodRector
on__DIR__ . '/system/CodeIgniter.php
fileonly specificy on specific files on skip
StringClassNameToClassConstantRector
due to internal PHPStan issue for detecting namespaced string for and expected string in testsRe-run Rector
Checklist: