Skip to content

Commit

Permalink
[8.x] Catch suppressed deprecation logs (#40942)
Browse files Browse the repository at this point in the history 
* Report "suppressed"  deprecations

* Report "suppressed" deprecations on tests
  • Loading branch information
@nunomaduro
nunomaduro authored Feb 10, 2022
1 parent 29bc877 commit c48540d
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 5 deletions.
8 changes: 4 additions & 4 deletions src/Illuminate/Foundation/Bootstrap/HandleExceptions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ public function bootstrap(Application $app)
*/
public function handleError($level, $message, $file = '', $line = 0, $context = [])
{
if (error_reporting() & $level) {
if ($this->isDeprecation($level)) {
return $this->handleDeprecation($message, $file, $line);
}
if ($this->isDeprecation($level)) {

This comment has been minimized.

Sign in to view

Copy link
@knobik

knobik Feb 24, 2022

this change breaks the correct flow as it suppresses the Trying to get property 'id' of non-object errors and others, resulting effect is that relations that dont exist instead of throwing an error which can be catched return null (they are suppressed) Its a breaking error.

This comment has been minimized.

Sign in to view

Copy link
@nunomaduro

nunomaduro Feb 24, 2022

Author Member

Can you give me an example please?

This comment has been minimized.

Sign in to view

Copy link
@knobik

knobik Feb 24, 2022

sure. Somemodel::find(1)->relationThatDoesNotExist->id returns null instead of throwing Trying to get property 'id' of non-object error.

This comment has been minimized.

Sign in to view

Copy link
@knobik

knobik Feb 24, 2022

everything works fine with 8.83.0. i narrowed it down to this change.

This comment has been minimized.

Sign in to view

Copy link
@knobik

knobik Feb 24, 2022 •
edited
Loading

what i found is that error_reporting is set to 0 for fpm. our php.ini for fpm has it set to E_ALL, and our code does not set it manualy to 0.

Edit: scratch that, its -1 in fpm. but when i dumped it in the handleError it was 0. so you can ignore this comment.

This comment has been minimized.

Sign in to view

Copy link
@knobik

knobik Feb 24, 2022

@nunomaduro I think i know whats the problem. Once an error is silenced the error_reporting does not go back to the old value. End effect is that after the first silenced error, error_reporting stays at 0.

This comment has been minimized.

Sign in to view

Copy link
@nunomaduro

nunomaduro Feb 24, 2022

Author Member

It does not matter what do you have in your php.ini. Laravel will always set the error reporting to -1. It must be something in your application. The code bellow results on an ErrorException on my machine:

➜  deprecations php artisan inspire

   ErrorException 

  Attempt to read property "id" on null

  at routes/console.php:20
     16▕ 
     17▕ Artisan::command('inspire', function () {
     18▕     $user = null;
     19▕ 
  ➜  20▕     $user->id;

This comment has been minimized.

Sign in to view

Copy link
@knobik

knobik Feb 24, 2022 •
edited
Loading

image

public function handleError($level, $message, $file = '', $line = 0, $context = [])
    {
        dump(
            error_reporting(),
            $message,
        );

first dump is the error_reporting value, second is the first silenced error, handleError should be called atleast 2 times, one for the silenced error and 2nd time for the non-object error. Yet, its called only once and acts like the non-object error never happened. I dont think i need to say that error_reporting is set to E_ALL before this happens. Please try testing in the browser, in CLI i also get the correct flow.

This comment has been minimized.

Sign in to view

Copy link
@knobik

knobik Feb 24, 2022

@nunomaduro do you need me to create a PoC repository for this? This can be replicated by adding a depracated warning before the non-object error. All other errors are ignored in that case.

This comment has been minimized.

Sign in to view

Copy link
@nunomaduro

nunomaduro Feb 24, 2022

Author Member

Yes - create a POC repository.

This comment has been minimized.

Sign in to view

Copy link
@knobik

knobik Feb 24, 2022

cant replicate this error in laravel 9, will search for a old small project that has laravel 8.
return $this->handleDeprecation($message, $file, $line);
}

if (error_reporting() & $level) {
throw new ErrorException($message, 0, $level, $file, $line);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Foundation/Testing/Concerns/InteractsWithDeprecationHandling.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ protected function withoutDeprecationHandling()
{
if ($this->originalDeprecationHandler == null) {
$this->originalDeprecationHandler = set_error_handler(function ($level, $message, $file = '', $line = 0) {
if (error_reporting() & $level) {
if (in_array($level, [E_DEPRECATED, E_USER_DEPRECATED]) || (error_reporting() & $level)) {
throw new ErrorException($message, 0, $level, $file, $line);
}
});
Expand Down

0 comments on commit c48540d

Please sign in to comment.