Do not override error_reporting
#6885
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
That's expected, this is development tool, and any raised error should be shown so it is easy to catch regression and report error. |
But shouldn't that be a decision of the PHP environment? As it is right now, executing rector will display hundreds of deprecation messages, all unrelated to yourself or rector. |
|
The error can be in any part, and should be fixed instead of ignored, report to the target repository if came from dependencies. |
That's already done - but alas no changes have been implemented yet. In any case, reporting the issue caused by transient depenencies is never an immediate fix and other than patching the Again, the decision on whether or not to show various types of errors, warnings or deprecations should be left to the PHP environment. There is not point in |
|
That actually overridden in ApplicationFileProcessor: rector-src/src/Application/ApplicationFileProcessor.php Lines 200 to 218 in ef1c19f That's due to parallel, that possibly only hit once, this need to work in both non-parallel and parallel if needed so back and forth check not needed, this also require e2e test to prove it, that you can write under |
|
The custom error handler only applies later (where you would not show |
|
e2e is needed to avoid shot in the dark :) |
|
I think we could do something in the middle. Use error reporting, but do not show the deprecations. Those are mostly annoying and better handled by PHPStan deprecation rules package. PHPStan does it the same way: |
|
But still - why override the error reporting level set by the PHP environment at all? If I want to show deprecations I can do so by changing the PHP settings, either globally or just when executing rector. |
|
@fritzmg Because by definition Rector is used to help with the most legacy codebases there are. It should be able to fix all of those reports, if there is one way to fix them. Displaying them again would only spam the output and confuse the developer there is something wrong with the codebase. Think of it as external tool that has to run standalone and separately from your main application behavior. |
You do not use rector to help with the codebase of a foreign dependency you have no control over, you use rector for your own codebase.
Exactly, so why use |
|
May be it was not clear what I actually mean. You can reproduce the issue with the following {
"type": "project",
"require": {
"spomky-labs/otphp": "^10.0"
},
"require-dev": {
"rector/rector": "^2.0"
}
}Then run rector outputEvery time you run rector you see the PHP 8.4+ related deprecations from a dependency you have no control over. In this case I am using So everytime I am running rector I am reminded of a deprecation that is caused by a dependency of a dependency of a dependency of my project - not only is this not helpful, but the deeper the deprecation originates in the dependency tree, the less likely it becomes to be able to do something about it. In this particular case the issue could be fixed by updating to What I can do of course is not put |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently under PHP 8.4+ you get a deprecation message spam when running rector, depending on the other dev dependencies that might be installed alongside rector (similar to composer/composer#12285 and symfony/symfony#59975). This is because rector sets
\error_reporting(\E_ALL);in itsbin/rector.php.Is there any particular reason why rector should override the
error_reportinglevel defined by the PHP environment?