From 5055ddf995717f88eb9b201ad857b5e0f09cfb7d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 28 Nov 2019 18:42:27 +0100 Subject: [PATCH] Updated CLI commands to just print exception messages when possible --- .../CLI/src/Command/Api/DisableKeyCommand.php | 4 +- .../ShortUrl/DeleteShortUrlCommand.php | 13 ++---- .../ShortUrl/GenerateShortUrlCommand.php | 9 +--- .../Command/ShortUrl/ResolveUrlCommand.php | 2 +- .../CLI/src/Command/Tag/RenameTagCommand.php | 4 +- .../GeolocationDbUpdateFailedException.php | 12 ++---- module/CLI/src/Util/GeolocationDbUpdater.php | 3 -- .../Command/Api/DisableKeyCommandTest.php | 9 ++-- .../ShortUrl/DeleteShortUrlCommandTest.php | 14 +++---- .../ShortUrl/GenerateShortUrlCommandTest.php | 9 ++-- .../ShortUrl/ResolveUrlCommandTest.php | 7 ++-- .../test/Command/Tag/RenameTagCommandTest.php | 8 ++-- ...GeolocationDbUpdateFailedExceptionTest.php | 41 ------------------- .../src/Exception/NonUniqueSlugException.php | 2 +- 14 files changed, 40 insertions(+), 97 deletions(-) diff --git a/module/CLI/src/Command/Api/DisableKeyCommand.php b/module/CLI/src/Command/Api/DisableKeyCommand.php index e0f2b79c..f3eb7f6b 100644 --- a/module/CLI/src/Command/Api/DisableKeyCommand.php +++ b/module/CLI/src/Command/Api/DisableKeyCommand.php @@ -4,8 +4,8 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\Api; -use InvalidArgumentException; use Shlinkio\Shlink\CLI\Util\ExitCodes; +use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -45,7 +45,7 @@ class DisableKeyCommand extends Command $io->success(sprintf('API key "%s" properly disabled', $apiKey)); return ExitCodes::EXIT_SUCCESS; } catch (InvalidArgumentException $e) { - $io->error(sprintf('API key "%s" does not exist.', $apiKey)); + $io->error($e->getMessage()); return ExitCodes::EXIT_FAILURE; } } diff --git a/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php index f44e3e18..c2e81d0d 100644 --- a/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php @@ -56,21 +56,16 @@ class DeleteShortUrlCommand extends Command $this->runDelete($io, $shortCode, $ignoreThreshold); return ExitCodes::EXIT_SUCCESS; } catch (Exception\ShortUrlNotFoundException $e) { - $io->error(sprintf('Provided short code "%s" could not be found.', $shortCode)); + $io->error($e->getMessage()); return ExitCodes::EXIT_FAILURE; } catch (Exception\DeleteShortUrlException $e) { - return $this->retry($io, $shortCode, $e); + return $this->retry($io, $shortCode, $e->getMessage()); } } - private function retry(SymfonyStyle $io, string $shortCode, Exception\DeleteShortUrlException $e): int + private function retry(SymfonyStyle $io, string $shortCode, string $warningMsg): int { - $warningMsg = sprintf( - 'It was not possible to delete the short URL with short code "%s" because it has more than %s visits.', - $shortCode, - $e->getVisitsThreshold() - ); - $io->writeln('' . $warningMsg . ''); + $io->writeln(sprintf('%s', $warningMsg)); $forceDelete = $io->confirm('Do you want to delete it anyway?', false); if ($forceDelete) { diff --git a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php index 9d9c464b..5cce7030 100644 --- a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php @@ -141,13 +141,8 @@ class GenerateShortUrlCommand extends Command sprintf('Generated short URL: %s', $shortUrl->toString($this->domainConfig)), ]); return ExitCodes::EXIT_SUCCESS; - } catch (InvalidUrlException $e) { - $io->error(sprintf('Provided URL "%s" is invalid. Try with a different one.', $longUrl)); - return ExitCodes::EXIT_FAILURE; - } catch (NonUniqueSlugException $e) { - $io->error( - sprintf('Provided slug "%s" is already in use by another URL. Try with a different one.', $customSlug) - ); + } catch (InvalidUrlException | NonUniqueSlugException $e) { + $io->error($e->getMessage()); return ExitCodes::EXIT_FAILURE; } } diff --git a/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php b/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php index 28564369..e8db28e2 100644 --- a/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php @@ -65,7 +65,7 @@ class ResolveUrlCommand extends Command $output->writeln(sprintf('Long URL: %s', $url->getLongUrl())); return ExitCodes::EXIT_SUCCESS; } catch (ShortUrlNotFoundException $e) { - $io->error(sprintf('Provided short code "%s" could not be found.', $shortCode)); + $io->error($e->getMessage()); return ExitCodes::EXIT_FAILURE; } } diff --git a/module/CLI/src/Command/Tag/RenameTagCommand.php b/module/CLI/src/Command/Tag/RenameTagCommand.php index a7002f60..b3a21a2e 100644 --- a/module/CLI/src/Command/Tag/RenameTagCommand.php +++ b/module/CLI/src/Command/Tag/RenameTagCommand.php @@ -13,8 +13,6 @@ use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; -use function sprintf; - class RenameTagCommand extends Command { public const NAME = 'tag:rename'; @@ -48,7 +46,7 @@ class RenameTagCommand extends Command $io->success('Tag properly renamed.'); return ExitCodes::EXIT_SUCCESS; } catch (TagNotFoundException $e) { - $io->error(sprintf('A tag with name "%s" was not found', $oldName)); + $io->error($e->getMessage()); return ExitCodes::EXIT_FAILURE; } } diff --git a/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php b/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php index fcb680d5..38bb4c5f 100644 --- a/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php +++ b/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php @@ -12,20 +12,16 @@ class GeolocationDbUpdateFailedException extends RuntimeException implements Exc /** @var bool */ private $olderDbExists; - public function __construct(bool $olderDbExists, string $message = '', int $code = 0, ?Throwable $previous = null) - { - $this->olderDbExists = $olderDbExists; - parent::__construct($message, $code, $previous); - } - public static function create(bool $olderDbExists, ?Throwable $prev = null): self { - return new self( - $olderDbExists, + $e = new self( 'An error occurred while updating geolocation database, and an older version could not be found', 0, $prev ); + $e->olderDbExists = $olderDbExists; + + return $e; } public function olderDbExists(): bool diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php index ebff82aa..2e530a34 100644 --- a/module/CLI/src/Util/GeolocationDbUpdater.php +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -10,7 +10,6 @@ use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\IpGeolocation\Exception\RuntimeException; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface; use Symfony\Component\Lock\Factory as Locker; -use Throwable; class GeolocationDbUpdater implements GeolocationDbUpdaterInterface { @@ -40,8 +39,6 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface try { $this->downloadIfNeeded($mustBeUpdated, $handleProgress); - } catch (Throwable $e) { - throw $e; } finally { $lock->release(); } diff --git a/module/CLI/test/Command/Api/DisableKeyCommandTest.php b/module/CLI/test/Command/Api/DisableKeyCommandTest.php index ca17aa7a..37629091 100644 --- a/module/CLI/test/Command/Api/DisableKeyCommandTest.php +++ b/module/CLI/test/Command/Api/DisableKeyCommandTest.php @@ -29,7 +29,7 @@ class DisableKeyCommandTest extends TestCase } /** @test */ - public function providedApiKeyIsDisabled() + public function providedApiKeyIsDisabled(): void { $apiKey = 'abcd1234'; $this->apiKeyService->disable($apiKey)->shouldBeCalledOnce(); @@ -43,17 +43,18 @@ class DisableKeyCommandTest extends TestCase } /** @test */ - public function errorIsReturnedIfServiceThrowsException() + public function errorIsReturnedIfServiceThrowsException(): void { $apiKey = 'abcd1234'; - $disable = $this->apiKeyService->disable($apiKey)->willThrow(InvalidArgumentException::class); + $expectedMessage = 'API key "abcd1234" does not exist.'; + $disable = $this->apiKeyService->disable($apiKey)->willThrow(new InvalidArgumentException($expectedMessage)); $this->commandTester->execute([ 'apiKey' => $apiKey, ]); $output = $this->commandTester->getDisplay(); - $this->assertStringContainsString('API key "abcd1234" does not exist.', $output); + $this->assertStringContainsString($expectedMessage, $output); $disable->shouldHaveBeenCalledOnce(); } } diff --git a/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php index 6a036c62..85521835 100644 --- a/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php @@ -58,13 +58,13 @@ class DeleteShortUrlCommandTest extends TestCase { $shortCode = 'abc123'; $deleteByShortCode = $this->service->deleteByShortCode($shortCode, false)->willThrow( - Exception\ShortUrlNotFoundException::class + Exception\ShortUrlNotFoundException::fromNotFoundShortCode($shortCode) ); $this->commandTester->execute(['shortCode' => $shortCode]); $output = $this->commandTester->getDisplay(); - $this->assertStringContainsString(sprintf('Provided short code "%s" could not be found.', $shortCode), $output); + $this->assertStringContainsString(sprintf('No URL found with short code "%s"', $shortCode), $output); $deleteByShortCode->shouldHaveBeenCalledOnce(); } @@ -79,11 +79,11 @@ class DeleteShortUrlCommandTest extends TestCase ): void { $shortCode = 'abc123'; $deleteByShortCode = $this->service->deleteByShortCode($shortCode, Argument::type('bool'))->will( - function (array $args) { + function (array $args) use ($shortCode) { $ignoreThreshold = array_pop($args); if (!$ignoreThreshold) { - throw Exception\DeleteShortUrlException::fromVisitsThreshold(10, ''); + throw Exception\DeleteShortUrlException::fromVisitsThreshold(10, $shortCode); } } ); @@ -93,7 +93,7 @@ class DeleteShortUrlCommandTest extends TestCase $output = $this->commandTester->getDisplay(); $this->assertStringContainsString(sprintf( - 'It was not possible to delete the short URL with short code "%s" because it has more than 10 visits.', + 'Impossible to delete short URL with short code "%s" since it has more than "10" visits.', $shortCode ), $output); $this->assertStringContainsString($expectedMessage, $output); @@ -112,7 +112,7 @@ class DeleteShortUrlCommandTest extends TestCase { $shortCode = 'abc123'; $deleteByShortCode = $this->service->deleteByShortCode($shortCode, false)->willThrow( - Exception\DeleteShortUrlException::fromVisitsThreshold(10, '') + Exception\DeleteShortUrlException::fromVisitsThreshold(10, $shortCode) ); $this->commandTester->setInputs(['no']); @@ -120,7 +120,7 @@ class DeleteShortUrlCommandTest extends TestCase $output = $this->commandTester->getDisplay(); $this->assertStringContainsString(sprintf( - 'It was not possible to delete the short URL with short code "%s" because it has more than 10 visits.', + 'Impossible to delete short URL with short code "%s" since it has more than "10" visits.', $shortCode ), $output); $this->assertStringContainsString('Short URL was not deleted.', $output); diff --git a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php index d83bd042..abae0fe6 100644 --- a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php @@ -59,21 +59,22 @@ class GenerateShortUrlCommandTest extends TestCase /** @test */ public function exceptionWhileParsingLongUrlOutputsError(): void { - $this->urlShortener->urlToShortCode(Argument::cetera())->willThrow(new InvalidUrlException()) + $url = 'http://domain.com/invalid'; + $this->urlShortener->urlToShortCode(Argument::cetera())->willThrow(InvalidUrlException::fromUrl($url)) ->shouldBeCalledOnce(); - $this->commandTester->execute(['longUrl' => 'http://domain.com/invalid']); + $this->commandTester->execute(['longUrl' => $url]); $output = $this->commandTester->getDisplay(); $this->assertEquals(ExitCodes::EXIT_FAILURE, $this->commandTester->getStatusCode()); - $this->assertStringContainsString('Provided URL "http://domain.com/invalid" is invalid.', $output); + $this->assertStringContainsString('Provided URL http://domain.com/invalid is invalid.', $output); } /** @test */ public function providingNonUniqueSlugOutputsError(): void { $urlToShortCode = $this->urlShortener->urlToShortCode(Argument::cetera())->willThrow( - NonUniqueSlugException::class + NonUniqueSlugException::fromSlug('my-slug') ); $this->commandTester->execute(['longUrl' => 'http://domain.com/invalid', '--customSlug' => 'my-slug']); diff --git a/module/CLI/test/Command/ShortUrl/ResolveUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/ResolveUrlCommandTest.php index 23b4ec28..11b549e5 100644 --- a/module/CLI/test/Command/ShortUrl/ResolveUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/ResolveUrlCommandTest.php @@ -52,11 +52,12 @@ class ResolveUrlCommandTest extends TestCase public function incorrectShortCodeOutputsErrorMessage(): void { $shortCode = 'abc123'; - $this->urlShortener->shortCodeToUrl($shortCode, null)->willThrow(ShortUrlNotFoundException::class) - ->shouldBeCalledOnce(); + $this->urlShortener->shortCodeToUrl($shortCode, null) + ->willThrow(ShortUrlNotFoundException::fromNotFoundShortCode($shortCode)) + ->shouldBeCalledOnce(); $this->commandTester->execute(['shortCode' => $shortCode]); $output = $this->commandTester->getDisplay(); - $this->assertStringContainsString(sprintf('Provided short code "%s" could not be found', $shortCode), $output); + $this->assertStringContainsString(sprintf('No URL found with short code "%s"', $shortCode), $output); } } diff --git a/module/CLI/test/Command/Tag/RenameTagCommandTest.php b/module/CLI/test/Command/Tag/RenameTagCommandTest.php index 4fc2aaad..c626e0c0 100644 --- a/module/CLI/test/Command/Tag/RenameTagCommandTest.php +++ b/module/CLI/test/Command/Tag/RenameTagCommandTest.php @@ -34,11 +34,11 @@ class RenameTagCommandTest extends TestCase } /** @test */ - public function errorIsPrintedIfExceptionIsThrown() + public function errorIsPrintedIfExceptionIsThrown(): void { $oldName = 'foo'; $newName = 'bar'; - $renameTag = $this->tagService->renameTag($oldName, $newName)->willThrow(TagNotFoundException::class); + $renameTag = $this->tagService->renameTag($oldName, $newName)->willThrow(TagNotFoundException::fromTag('foo')); $this->commandTester->execute([ 'oldName' => $oldName, @@ -46,12 +46,12 @@ class RenameTagCommandTest extends TestCase ]); $output = $this->commandTester->getDisplay(); - $this->assertStringContainsString('A tag with name "foo" was not found', $output); + $this->assertStringContainsString('Tag with name "foo" could not be found', $output); $renameTag->shouldHaveBeenCalled(); } /** @test */ - public function successIsPrintedIfNoErrorOccurs() + public function successIsPrintedIfNoErrorOccurs(): void { $oldName = 'foo'; $newName = 'bar'; diff --git a/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php index 51c87cb3..70a8cc6f 100644 --- a/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php +++ b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php @@ -12,47 +12,6 @@ use Throwable; class GeolocationDbUpdateFailedExceptionTest extends TestCase { - /** - * @test - * @dataProvider provideOlderDbExists - */ - public function constructCreatesExceptionWithDefaultArgs(bool $olderDbExists): void - { - $e = new GeolocationDbUpdateFailedException($olderDbExists); - - $this->assertEquals($olderDbExists, $e->olderDbExists()); - $this->assertEquals('', $e->getMessage()); - $this->assertEquals(0, $e->getCode()); - $this->assertNull($e->getPrevious()); - } - - public function provideOlderDbExists(): iterable - { - yield 'with older DB' => [true]; - yield 'without older DB' => [false]; - } - - /** - * @test - * @dataProvider provideConstructorArgs - */ - public function constructCreatesException(bool $olderDbExists, string $message, int $code, ?Throwable $prev): void - { - $e = new GeolocationDbUpdateFailedException($olderDbExists, $message, $code, $prev); - - $this->assertEquals($olderDbExists, $e->olderDbExists()); - $this->assertEquals($message, $e->getMessage()); - $this->assertEquals($code, $e->getCode()); - $this->assertEquals($prev, $e->getPrevious()); - } - - public function provideConstructorArgs(): iterable - { - yield [true, 'This is a nice error message', 99, new Exception('prev')]; - yield [false, 'Another message', 0, new RuntimeException('prev')]; - yield [true, 'An yet another message', -50, null]; - } - /** * @test * @dataProvider provideCreateArgs diff --git a/module/Core/src/Exception/NonUniqueSlugException.php b/module/Core/src/Exception/NonUniqueSlugException.php index bcd40268..51beff82 100644 --- a/module/Core/src/Exception/NonUniqueSlugException.php +++ b/module/Core/src/Exception/NonUniqueSlugException.php @@ -17,7 +17,7 @@ class NonUniqueSlugException extends InvalidArgumentException implements Problem private const TITLE = 'Invalid custom slug'; private const TYPE = 'INVALID_SLUG'; - public static function fromSlug(string $slug, ?string $domain): self + public static function fromSlug(string $slug, ?string $domain = null): self { $suffix = $domain === null ? '' : sprintf(' for domain "%s"', $domain); $e = new self(sprintf('Provided slug "%s" is already in use%s.', $slug, $suffix));