-
Notifications
You must be signed in to change notification settings - Fork 48
163 modify 500 code error response page #174
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
base: master
Are you sure you want to change the base?
Conversation
…hub.com/RonasIT/laravel-swagger into 163-modify-500-code-error-response-page
|
src/Services/SwaggerService.php
Outdated
} catch (Exception $exception) { | ||
return $this->generateEmptyData($this->config['defaults']['error'], ['message' => $exception->getMessage()]); | ||
} catch (Throwable $exception) { | ||
$message = $exception instanceof Exception ? $exception->getMessage() : '[]'; |
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.
$message = $exception instanceof Exception ? $exception->getMessage() : '[]'; | |
$message = ($exception instanceof Exception) ? $exception->getMessage() : '[]'; |
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.
Why we can't always write message?
For me always show $exception->getMessage() is fine. What are we trying to hide here?
if we trying hide only next Throwable message 'RonasIT\AutoDoc\Drivers\LocalDriver::getDocumentation(): Return value must be of type array, null returned'. Maybe we can write like kind "Documentation file is empty or have bad format" insted []
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.
@AZabolotnikov I don't mind of your way
@RGO230 any reasons to make an additional check?
src/Services/SwaggerService.php
Outdated
} catch (Exception $exception) { | ||
return $this->generateEmptyData($this->config['defaults']['error'], ['message' => $exception->getMessage()]); | ||
} catch (Throwable $exception) { | ||
$message = $exception instanceof Exception ? $exception->getMessage() : '[]'; |
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.
@AZabolotnikov I don't mind of your way
@RGO230 any reasons to make an additional check?
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.
Pull Request Overview
This PR modifies the error response page formatting for Swagger validation errors, addressing issue #163. The changes enhance error presentation with structured markdown formatting and proper error context display.
- Reformats error page display with markdown structure including headers and visual separators
- Updates error handling to include exception type information alongside error messages
- Fixes test parameter ordering and adds new test case for incorrect documentation structure
Reviewed Changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
resources/views/error.blade.php | Updates error page template with structured markdown formatting |
src/Services/SwaggerService.php | Modifies exception handling to capture and pass exception type information |
tests/SwaggerServiceTest.php | Updates test expectations and adds new test case for documentation structure errors |
tests/TestCase.php | Fixes parameter order in exportContent method call |
tests/fixtures/SwaggerServiceTest/*.html | Updates all error fixture files to match new markdown-formatted error template |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tests/SwaggerServiceTest.php
Outdated
$this->expectException(EmptyContactEmailException::class); | ||
$content = app(SwaggerService::class)->getDocFileContent(); | ||
|
||
$this->assertEqualsFixture('invalid_config_email.html', $content['info']['description'], true); | ||
|
||
app(SwaggerService::class); |
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.
This line creates a new SwaggerService instance but doesn't use it. The test appears to be checking the error handling behavior, but this line won't trigger the expected error condition since the previous lines already retrieved the content. Remove this unused instantiation.
app(SwaggerService::class); |
Copilot uses AI. Check for mistakes.
$exception = new EmptyContactEmailException(); | ||
|
||
$viewData = [ | ||
'message' => $exception->getMessage(), | ||
'type' => $exception::class, | ||
]; |
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.
Creating an exception instance without throwing it is unusual. The original code threw the exception, but now it's only created to extract the message. This changes the behavior from throwing an exception to returning error content, which may break existing error handling expectations.
$exception = new EmptyContactEmailException(); | |
$viewData = [ | |
'message' => $exception->getMessage(), | |
'type' => $exception::class, | |
]; | |
throw new EmptyContactEmailException(); |
Copilot uses AI. Check for mistakes.
|
#163