Skip to content
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

Allow for custom exception handlers. #5675

Closed
wants to merge 2 commits into from
Closed

Conversation

lonnieezell
Copy link
Member

Allows developers to easily create and integrate their own Exception handler classes, and limiting their usage
to only the exception or HTTP status code desired.

Supercedes #5355

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member

MGatner commented Feb 11, 2022

This is more then I can handle on mobile, but looks really good! On my "to do" list.

Copy link
Collaborator

@iRedds iRedds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that this PR is BC.
Services::exceptions() returns an instance of the CodeIgniter\Debug\Exceptions class, which can be extended.
Changing this class will cause breaking changes for the custom implementation.

Comment on lines +78 to +80
* if ($exception instanceOf PageNotFoundException) {
* return new \App\Libraries\MyExceptionHandler();
* }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR implies handling of unhandled exceptions, but PageNotFoundException is handled separately.
Therefore, the mention of PageNotFoundException can be misleading.

Comment on lines +193 to +195
if ($exception instanceOf PageNotFoundException) {
return new \App\Libraries\MyExceptionHandler();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR implies handling of unhandled exceptions, but PageNotFoundException is handled separately.
Therefore, the mention of PageNotFoundException can be misleading.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PageNotFound is still handled in the ExceptionHandler, so if someone wants to override the handling on that one they still can, unless it's been overridden at the routing level, of course.

Comment on lines +88 to +94
$config = config('Exceptions');

if (strpos($this->request->getHeaderLine('accept'), 'text/html') === false) {
$this->respond(ENVIRONMENT === 'development' ? $this->collectVars($exception, $statusCode) : '', $statusCode)->send();

exit($exitCode);
}
if (! method_exists($config, 'handler')) {
exit('Config\Exception must have a handler() method.');
}

$this->render($exception, $statusCode);
$handler = config('Exceptions')->handler($statusCode, $exception);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config('Exceptions') is called twice even though we already have the $config variable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thanks. I forgot to go back and clean that up. Will do.

Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
@lonnieezell
Copy link
Member Author

It seems to me that this PR is BC. Services::exceptions() returns an instance of the CodeIgniter\Debug\Exceptions class, which can be extended. Changing this class will cause breaking changes for the custom implementation.

Yeah, you're correct. I have been sick the last couple of days but needed to do something other than sleep for a few hours and wasn't up to tackling the refactor of Tasks needed to get it integrated into the framework so I figured I could knock this out. Obviously wasn't thinking 100%.

We can shelve it or start a 5.0 branch so it doesn't just clog up the PR lists, or something.

@kenjis kenjis added breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities labels Feb 12, 2022
@lonnieezell lonnieezell added this to the 5.0 milestone Feb 22, 2022
{
if (! is_cli()) {
$this->response->setStatusCode($this->statusCode);
header(sprintf('HTTP/%s %s %s', $this->request->getProtocolVersion(), $this->response->getStatusCode(), $this->response->getReasonPhrase()), true, $this->statusCode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If headers have been sent, calling header() causes error.

#5680

@@ -107,20 +85,25 @@ public function exceptionHandler(Throwable $exception)
]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenjis kenjis changed the base branch from develop to 4.3 June 4, 2022 21:26
@kenjis kenjis added stale Pull requests with conflicts 4.3 labels Jun 4, 2022
@kenjis kenjis mentioned this pull request Oct 18, 2022
5 tasks
Base automatically changed from 4.3 to develop January 10, 2023 06:36
@gphg
Copy link

gphg commented Feb 27, 2023

Any update on this PR?

@kenjis
Copy link
Member

kenjis commented Feb 27, 2023

I have updated this PR for 4.4, and it is waiting for reviews.
#7087

@kenjis kenjis mentioned this pull request Apr 8, 2023
5 tasks
@kenjis
Copy link
Member

kenjis commented Apr 9, 2023

Closed by #7087

@kenjis kenjis closed this Apr 9, 2023
@kenjis kenjis deleted the new_exceptions branch April 9, 2023 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities stale Pull requests with conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants