Skip to content

Event approach: clean up API error handling with concise user messages #115

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Event approach: clean up API error handling with concise user messages #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions Cli/EventListener/ApiErrorListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?php

namespace SensioLabs\Insight\Cli\EventListener;

use SensioLabs\Insight\Sdk\Exception\ApiClientException;
use SensioLabs\Insight\Sdk\Exception\ApiParserException;
use SensioLabs\Insight\Sdk\Exception\ApiServerException;
use Symfony\Component\Console\ConsoleEvents;
use Symfony\Component\Console\Event\ConsoleErrorEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;

class ApiErrorListener implements EventSubscriberInterface
{
public static function getSubscribedEvents(): array
{
return [
ConsoleEvents::ERROR => ['onConsoleError', 256],
];
}

public function onConsoleError(ConsoleErrorEvent $event): void
{
$output = $event->getOutput();
$error = $event->getError();

if ($error instanceof ApiClientException) {
$this->showClientError($error, $output);
} elseif ($error instanceof ApiServerException) {
$output->writeln('<error>Server temporarily unavailable. Please try again in a few minutes.</error>');
} elseif ($error instanceof ApiParserException) {
$output->writeln('<error>Unable to process server response. Please try again later.</error>');
} elseif ($error instanceof TransportExceptionInterface) {
$output->writeln('<error>Network connection failed. Check your internet connection.</error>');
} else {
$output->writeln('<error>Something went wrong. Please try again, or run with -v for more details.</error>');
}

if ($output->isVerbose()) {
throw $error;
}

$event->setExitCode(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

0?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will terminate the process gracefully and display only a user-friendly message. No exception details or additional output.

Copy link
Contributor

Choose a reason for hiding this comment

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

correct, but is that what ppl will expect? I'm not sure

Copy link
Member Author

Choose a reason for hiding this comment

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

exit code is 0

➜  insight git:(handler) ✗ bin/insight analyze 1c92dfd89776     
Something went wrong. Please try again, or run with -v for more details.

exit code 1

➜  insight git:(handler) ✗ bin/insight analyze 1c92dfd89776
Something went wrong. Please try again, or run with -v for more details.

In AnalyzeCommand.php line 43:
                                  
  Testing the exception handling  
                                  

analyze [--format FORMAT] [--reference REFERENCE] [--branch BRANCH] [--show-ignored-violations] [--poll-period POLL-PERIOD] [--fail-condition FAIL-CONDITION] [--no-wait] [--] <project-uuid>

Copy link
Contributor

@nicolas-grekas nicolas-grekas Sep 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't make sense. Optimizing the displayed output shouldn't come at the price of breaking exit codes. Can you figure out another way that doesn't break the exit code?

}

private function showClientError(ApiClientException $e, $output): void
{
$message = $e->getMessage();
if (false !== strpos($message, '401') || false !== strpos($message, 'Unauthorized')) {
$output->writeln('<error>Invalid API credentials. Run "php insight.phar configure" to set up your token.</error>');
} elseif (false !== strpos($message, '404') || false !== strpos($message, 'Not Found')) {
$output->writeln('<error>Project not found. Check the project UUID and try again.</error>');
} elseif (false !== strpos($message, '403') || false !== strpos($message, 'Forbidden')) {
$output->writeln('<error>Access denied. You don\'t have permission for this project.</error>');
} else {
$output->writeln('<error>Request failed. Please check your input and try again.</error>');
}
}
}
22 changes: 16 additions & 6 deletions Sdk/Api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,12 +289,22 @@ private function processClientError(HttpExceptionInterface $e)

private function logException(ExceptionInterface $e)
{
$message = sprintf("Exception: Class: \"%s\", Message: \"%s\", Response:\n%s",
\get_class($e),
$e->getMessage(),
$e->getResponse()->getInfo('debug')
);
if (!$this->logger) {
return;
}

$parts = [
sprintf('Exception: Class: "%s"', \get_class($e)),
sprintf('Message: "%s"', $e->getMessage()),
];

if ($e instanceof HttpExceptionInterface) {
$debug = $e->getResponse()->getInfo('debug');
if ($debug) {
$parts[] = "Response:\n".$debug;
}
}

$this->logger && $this->logger->error($message, ['exception' => $e]);
$this->logger->error(implode(', ', $parts), ['exception' => $e]);
}
}
6 changes: 5 additions & 1 deletion bin/insight
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ if (file_exists($a = __DIR__.'/../../../autoload.php')) {
}

use SensioLabs\Insight\Cli\Application;
use Symfony\Component\EventDispatcher\EventDispatcher;
use SensioLabs\Insight\Cli\EventListener\ApiErrorListener;

$application = new Application();

$dispatcher = new EventDispatcher();
$dispatcher->addSubscriber(new ApiErrorListener());
$application->setDispatcher($dispatcher);
$application->run();
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
"psr/log": "^1.0",
"symfony/http-client": "^5.4|^6.4|^7.0",
"symfony/console": "^5.4|^6.4|^7.0",
"symfony/expression-language": "^5.4|^6.4|^7.0"
"symfony/expression-language": "^5.4|^6.4|^7.0",
"symfony/event-dispatcher": "^5.4|^6.4|^7.0"
},
"require-dev": {
"monolog/monolog": "^1.4",
Expand Down
Loading