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

Custom ErrorHandlers are not used by ErrorHandlerMiddleware #11323

Open
chinpei215 opened this Issue Oct 13, 2017 · 21 comments

Comments

Projects
None yet
4 participants
@chinpei215
Member

chinpei215 commented Oct 13, 2017

This is a (multiple allowed):

  • bug

  • enhancement

  • feature-discussion (RFC)

  • CakePHP Version: 3.5.3

  • Platform and Target: PHP7

What you did

Declare the following controller:

class TestsController extends AppController
{
    public function exception() {
        throw new \Exception();
    }

    public function error() {
        throw new \Error();
    }
}

Access /tests/exception and /tests/error.

What happened

While Exceptions are handled by ErrorHandlerMiddleware::handleException(), PHP7 Errors are handled by ErrorHandler::handleException().

What you expected to happen

Both Exceptions and PHP7 Errors are handled by ErrorHandlerMiddleware::handleException() if ErrorHandlerMiddleware is enabled.

Related code

public function __invoke($request, $response, $next)
{
try {
return $next($request, $response);
} catch (Exception $e) {
return $this->handleException($e, $request, $response);
}
}

Related issues

#8997, #9500, #9931

@chinpei215 chinpei215 added this to the 3.6.0 milestone Oct 13, 2017

@chinpei215 chinpei215 added the RFC label Oct 13, 2017

@robertpustulka

This comment has been minimized.

Show comment
Hide comment
@robertpustulka

robertpustulka Oct 13, 2017

Member

Would something like that work both in PHP 5 and 7?

         try { 
             return $next($request, $response); 
         } catch (Exception $e) { 
             return $this->handleException($e, $request, $response); 
         } catch (Throwable $e) { 
             return $this->handleException($e, $request, $response); 
         }  
Member

robertpustulka commented Oct 13, 2017

Would something like that work both in PHP 5 and 7?

         try { 
             return $next($request, $response); 
         } catch (Exception $e) { 
             return $this->handleException($e, $request, $response); 
         } catch (Throwable $e) { 
             return $this->handleException($e, $request, $response); 
         }  
@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Oct 13, 2017

Member

Yes. And if there are no problems, I would like to add the same catch blocks to Connection::transactional() too. Otherwise, if people use DatabaseLog engine, the logs are not logged if PHP7 errors happen in transactions.

Member

chinpei215 commented Oct 13, 2017

Yes. And if there are no problems, I would like to add the same catch blocks to Connection::transactional() too. Otherwise, if people use DatabaseLog engine, the logs are not logged if PHP7 errors happen in transactions.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 13, 2017

Member

Related to #8997 and #9500

Member

markstory commented Oct 13, 2017

Related to #8997 and #9500

@markstory markstory added the ORM label Oct 13, 2017

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Oct 14, 2017

Member

By the way, what do you think about using the ErrorHandler in the ErrorHandlerMiddleware? Right now I can see kinds of duplicate code in the two classes:

protected function _getMessage(Exception $exception)

protected function getMessage($request, $exception)

But I don't think we can deprecate BaseErrorHandler's one. Because, even if there were another catch block for PHP7 Errors in the ErrorHandlerMiddleware::handleException(), BaseErrorHandler::handleException() would be still called when an exception happens before or after HTTP middleware layer. And, ConsoleErrorHandler, derived from BaseErrorHandler, would continue to be used in CLI applications (if new Command class doesn't change the current error handling process).

So I think it would be better to use the ErrorHandler in the ErrorHandlerMiddleware:

try { 
    return $next($request, $response); 
} catch (Exception $e) {
} catch (Throwable $e) {
}

$this->errorHandler->setRequest($request);
$this->errorHandler->setResponse($response);
return $this->errorHandler->handleException($e); 

The BaseErrorHandler::setRequest() is a new method. If the request is given, BaseErrorHandler will use it instead of Router::getRequest(). The BaseErrorHandler::setReponse() may also be needed. Probably, this change would allow people to create a custom error handler in the same way as before. As of CakePHP 3.3, it is bit hard to do it.

Member

chinpei215 commented Oct 14, 2017

By the way, what do you think about using the ErrorHandler in the ErrorHandlerMiddleware? Right now I can see kinds of duplicate code in the two classes:

protected function _getMessage(Exception $exception)

protected function getMessage($request, $exception)

But I don't think we can deprecate BaseErrorHandler's one. Because, even if there were another catch block for PHP7 Errors in the ErrorHandlerMiddleware::handleException(), BaseErrorHandler::handleException() would be still called when an exception happens before or after HTTP middleware layer. And, ConsoleErrorHandler, derived from BaseErrorHandler, would continue to be used in CLI applications (if new Command class doesn't change the current error handling process).

So I think it would be better to use the ErrorHandler in the ErrorHandlerMiddleware:

try { 
    return $next($request, $response); 
} catch (Exception $e) {
} catch (Throwable $e) {
}

$this->errorHandler->setRequest($request);
$this->errorHandler->setResponse($response);
return $this->errorHandler->handleException($e); 

The BaseErrorHandler::setRequest() is a new method. If the request is given, BaseErrorHandler will use it instead of Router::getRequest(). The BaseErrorHandler::setReponse() may also be needed. Probably, this change would allow people to create a custom error handler in the same way as before. As of CakePHP 3.3, it is bit hard to do it.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Oct 14, 2017

Member

Related to #9931

Member

chinpei215 commented Oct 14, 2017

Related to #9931

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 14, 2017

Member

What is harder about creating an exception handler now compared with 3.3?

Member

markstory commented Oct 14, 2017

What is harder about creating an exception handler now compared with 3.3?

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Oct 14, 2017

Member

As of 3.3, ErrorHandler doesn't catch any exceptions usually, as ErrorHandlerMiddleware catches and discards them earlier than ErrorHandler. So the following section of the Cookbook is no longer accurate.
https://book.cakephp.org/3.0/en/development/errors.html#creating-your-own-error-handler
In addition to create a custom error handler, people have to extend ErrorHandlerMiddleware, or remove ErrorHandlerMiddleware from HTTP middleware stack.

Member

chinpei215 commented Oct 14, 2017

As of 3.3, ErrorHandler doesn't catch any exceptions usually, as ErrorHandlerMiddleware catches and discards them earlier than ErrorHandler. So the following section of the Cookbook is no longer accurate.
https://book.cakephp.org/3.0/en/development/errors.html#creating-your-own-error-handler
In addition to create a custom error handler, people have to extend ErrorHandlerMiddleware, or remove ErrorHandlerMiddleware from HTTP middleware stack.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 14, 2017

Member

The global error handler is still used, but only for errors before the middleware is reached. I can see how the current state is not as elegant as it could be though.

Member

markstory commented Oct 14, 2017

The global error handler is still used, but only for errors before the middleware is reached. I can see how the current state is not as elegant as it could be though.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Oct 14, 2017

Member

Uncatchable errors, such as E_ERROR, would also be handled by the global error handler via the handleFatalError() method. Anyway, the current error handling process seems more complex than 3.3. So I suggested to make ErrorHandlerMiddleware use ErrorHandler::handleException() instead of its own exception handling code. ErrorHandler::handleException() will come to be called for any type of errors/exceptions.

Member

chinpei215 commented Oct 14, 2017

Uncatchable errors, such as E_ERROR, would also be handled by the global error handler via the handleFatalError() method. Anyway, the current error handling process seems more complex than 3.3. So I suggested to make ErrorHandlerMiddleware use ErrorHandler::handleException() instead of its own exception handling code. ErrorHandler::handleException() will come to be called for any type of errors/exceptions.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 14, 2017

Member

All PHP errors go to the existing ErrorHandler and not the middleware. Using the ErrorHandler inside the middleware presents a couple challenges.

  1. The current handleException() method directly outputs whereas the middleware needs to return a response.
  2. We don't yet have a way to pass an ErrorHandler into the middleware.

These are solvable issues, but they should be figured out.

Member

markstory commented Oct 14, 2017

All PHP errors go to the existing ErrorHandler and not the middleware. Using the ErrorHandler inside the middleware presents a couple challenges.

  1. The current handleException() method directly outputs whereas the middleware needs to return a response.
  2. We don't yet have a way to pass an ErrorHandler into the middleware.

These are solvable issues, but they should be figured out.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Oct 15, 2017

Member

I didn't realize the second one... How about doing as follows:

// In bootstrap.php
$this->setErrorHandler(new ErrorHandler(Configure::read('Error')));

// In Application.php
$errorHandler = new ErrorHandlerMiddleware();
$errorHandler->setErrorHandler($this->getErrorHandler());
$middlewareQueue->add($errorHandler);

Though we have to also update ConsoleApplicationInterface and HttpApplicationInterface.

Member

chinpei215 commented Oct 15, 2017

I didn't realize the second one... How about doing as follows:

// In bootstrap.php
$this->setErrorHandler(new ErrorHandler(Configure::read('Error')));

// In Application.php
$errorHandler = new ErrorHandlerMiddleware();
$errorHandler->setErrorHandler($this->getErrorHandler());
$middlewareQueue->add($errorHandler);

Though we have to also update ConsoleApplicationInterface and HttpApplicationInterface.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 15, 2017

Member

Could the ErrorHandler be a constructor argument for the middleware? We could fall back to the current behaviour if no handler is provided.

Member

markstory commented Oct 15, 2017

Could the ErrorHandler be a constructor argument for the middleware? We could fall back to the current behaviour if no handler is provided.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Oct 15, 2017

Member

It could. The method signature of the ErrorHandlerMiddleware::__construct() is the following:

public function __construct($exceptionRenderer = null, array $config = []);

How do we pass the ErrorHandler to this? There would be several choices - using the $config argument, adding the third argument, or constructor overloading.

Member

chinpei215 commented Oct 15, 2017

It could. The method signature of the ErrorHandlerMiddleware::__construct() is the following:

public function __construct($exceptionRenderer = null, array $config = []);

How do we pass the ErrorHandler to this? There would be several choices - using the $config argument, adding the third argument, or constructor overloading.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 16, 2017

Member

That is unfortunate. If an error handler instance is passed in wouldn't that be better than & replace an exception renderer being passed in?

Even if we can get an ErrorHandler into the middleware we'll need more methods on the ErrorHandler to return a modified response. Right now the handleException method outputs the response directly whereas the middleware stack will want a response to be returned.

Should we consider factoring out the currently duplicated sections of code into other objects that can be used by both the middleware and global/default handler?

Member

markstory commented Oct 16, 2017

That is unfortunate. If an error handler instance is passed in wouldn't that be better than & replace an exception renderer being passed in?

Even if we can get an ErrorHandler into the middleware we'll need more methods on the ErrorHandler to return a modified response. Right now the handleException method outputs the response directly whereas the middleware stack will want a response to be returned.

Should we consider factoring out the currently duplicated sections of code into other objects that can be used by both the middleware and global/default handler?

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Oct 16, 2017

Member

If an error handler instance is passed in wouldn't that be better than & replace an exception renderer being passed in?

Sorry, I cannot parse the structure of the above English sentence 😞
Did you say, "If an error handler instance is passed in, it would be better to replace an exception renderer too."?

Should we consider factoring out the currently duplicated sections of code into other objects that can be used by both the middleware and global/default handler?

At first, I was planning to create a new ExceptionLogger class for chained exception rendering. But it doesn't solve the above problem on creating custom handlers.

Member

chinpei215 commented Oct 16, 2017

If an error handler instance is passed in wouldn't that be better than & replace an exception renderer being passed in?

Sorry, I cannot parse the structure of the above English sentence 😞
Did you say, "If an error handler instance is passed in, it would be better to replace an exception renderer too."?

Should we consider factoring out the currently duplicated sections of code into other objects that can be used by both the middleware and global/default handler?

At first, I was planning to create a new ExceptionLogger class for chained exception rendering. But it doesn't solve the above problem on creating custom handlers.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 17, 2017

Member

Sorry, I cannot parse the structure of the above English sentence

That's my mistake. It was an awkward sentence.

I was trying to say that the current exceptionRenderer argument could be overloaded to accept either an instance of BaseErrorHandler or ExceptionRendererInterface. Getting an instance of BaseErrorHandler would allow ErrorHandlerMiddleware to use it for more logic than just getting an ExceptionRenderer would.

An ExceptionLogger would allow us to consolidate the code that is currently duplicated and reuse it. In the middleware the logger could be provided as a config option.

Member

markstory commented Oct 17, 2017

Sorry, I cannot parse the structure of the above English sentence

That's my mistake. It was an awkward sentence.

I was trying to say that the current exceptionRenderer argument could be overloaded to accept either an instance of BaseErrorHandler or ExceptionRendererInterface. Getting an instance of BaseErrorHandler would allow ErrorHandlerMiddleware to use it for more logic than just getting an ExceptionRenderer would.

An ExceptionLogger would allow us to consolidate the code that is currently duplicated and reuse it. In the middleware the logger could be provided as a config option.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Oct 17, 2017

Member

Thank you for the clarification. I will try to overload ErrorHandlerMiddleware::__construct(). But if it is difficult, I will give up and create ExceptionLogger instead.

Member

chinpei215 commented Oct 17, 2017

Thank you for the clarification. I will try to overload ErrorHandlerMiddleware::__construct(). But if it is difficult, I will give up and create ExceptionLogger instead.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Dec 7, 2017

Member

If someone interests, please do this instead of me. I will give my priority to the other tasks.

Member

chinpei215 commented Dec 7, 2017

If someone interests, please do this instead of me. I will give my priority to the other tasks.

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Dec 7, 2017

Member

Resolved with above PR and follow up #11518

Member

dereuromark commented Dec 7, 2017

Resolved with above PR and follow up #11518

@dereuromark dereuromark closed this Dec 7, 2017

@chinpei215 chinpei215 changed the title from PHP7 Errors aren't handled by ErrorHandlerMiddleware to Custom ErrorHandlers are not used by ErrorHandlerMiddleware Dec 7, 2017

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Dec 7, 2017

Member

Probably, I should have changed the issue title. The current error handling process is bit strange.

  • (Uncatchable) Fatal errors are handled by the legacy ErrorHandler
  • PHP7 errors were handled by the legacy ErrorHandler Changed in 3.5.7
  • Exceptions are handled b y the ErrorHandlerMiddleware. (But client IP are not logged.

So I wanted to change ErrorHandlerMiddleware to use the legacy ErrorHandler. It allows people to create their custom ErrorHandler in the documented way. Currently, our Cookbook lies.

Member

chinpei215 commented Dec 7, 2017

Probably, I should have changed the issue title. The current error handling process is bit strange.

  • (Uncatchable) Fatal errors are handled by the legacy ErrorHandler
  • PHP7 errors were handled by the legacy ErrorHandler Changed in 3.5.7
  • Exceptions are handled b y the ErrorHandlerMiddleware. (But client IP are not logged.

So I wanted to change ErrorHandlerMiddleware to use the legacy ErrorHandler. It allows people to create their custom ErrorHandler in the documented way. Currently, our Cookbook lies.

@chinpei215 chinpei215 reopened this Dec 7, 2017

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Dec 7, 2017

Member

Streamlining that would probably be a good move, also remove un-DRY code here.
If you have a fix better than the PR I opened, go for it!

Please bear in mind that also any custom third party handler should work with this then of course.

Member

dereuromark commented Dec 7, 2017

Streamlining that would probably be a good move, also remove un-DRY code here.
If you have a fix better than the PR I opened, go for it!

Please bear in mind that also any custom third party handler should work with this then of course.

@markstory markstory removed this from the 3.6.0 milestone Apr 15, 2018

@markstory markstory added this to the 3.7.0 milestone Apr 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment