-
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] Apply Full PHP 7.3 Rector Set List #4600
[Rector] Apply Full PHP 7.3 Rector Set List #4600
Conversation
2b2182c
to
dc8f30a
Compare
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 like the JSON_THROW_ON_ERROR flag addition but I'm worrying someone's code will now break and throw the blame on us. I think this should be on the changelog to say that json operations now throw errors to force users to rectify their code.
@@ -45,6 +45,8 @@ parameters: | |||
- '#Return type \(bool\) of method CodeIgniter\\Log\\Logger::(emergency|alert|critical|error|warning|notice|info|debug|log)\(\) should be compatible with return type \(null\) of method Psr\\Log\\LoggerInterface::(emergency|alert|critical|error|warning|notice|info|debug|log)\(\)#' | |||
- '#Return type \(bool\) of method CodeIgniter\\HTTP\\Files\\UploadedFile::move\(\) should be compatible with return type \(CodeIgniter\\Files\\File\) of method CodeIgniter\\Files\\File::move\(\)#' | |||
- '#Return type \(bool\) of method CodeIgniter\\Test\\TestLogger::log\(\) should be compatible with return type \(null\) of method Psr\\Log\\LoggerInterface::log\(\)#' | |||
- '#Parameter \#2 \$assoc of function json_decode expects bool, null given#' | |||
reportUnmatchedIgnoredErrors: false |
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 think this should be set as true so that we can remove errors no longer applicable.
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.
yes, it currently shows notice in php < 8
I can create another alternative PR for allow |
Generally I agree about throwing new errors but looking through these specific instances I think a failed operation will already cause other errors that will just be harder to track down. The exception may be the Toolbar changes but that's only for dev instances anyways when (I believe) we want to be throwing more errors anyway. |
Closing in favor of #4601 |
Using :
to apply full php 7.3 rector set list.
Checklist: