diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b5a485a..6b38e908 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#1294](https://github.com/shlinkio/shlink/issues/1294) Allowed to specify a specific domain when importing URLs from YOURLS. * [#1416](https://github.com/shlinkio/shlink/issues/1416) Added support to import URLs from Kutt.it. * [#1418](https://github.com/shlinkio/shlink/issues/1418) Added support to customize the timezone used by Shlink, falling back to the default one set in PHP config. +* [#1309](https://github.com/shlinkio/shlink/issues/1309) Improved URL importing, ensuring individual errors do not make the whole process fail, and instead, failing URLs are skipped. ### Changed * [#1359](https://github.com/shlinkio/shlink/issues/1359) Hidden database commands. diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index 2ed57527..6b33c586 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -15,7 +15,9 @@ use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Shlinkio\Shlink\Importer\Params\ImportParams; use Shlinkio\Shlink\Importer\Sources\ImportSources; +use Symfony\Component\Console\Style\OutputStyle; use Symfony\Component\Console\Style\StyleInterface; +use Throwable; use function sprintf; @@ -33,7 +35,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface } /** - * @param iterable|ImportedShlinkUrl[] $shlinkUrls + * @param iterable $shlinkUrls */ public function process(StyleInterface $io, iterable $shlinkUrls, ImportParams $params): void { @@ -43,22 +45,26 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface /** @var ImportedShlinkUrl $importedUrl */ foreach ($iterable as $importedUrl) { - $skipOnShortCodeConflict = static function () use ($io, $importedUrl): bool { - $action = $io->choice(sprintf( - 'Failed to import URL "%s" because its short-code "%s" is already in use. Do you want to generate ' - . 'a new one or skip it?', - $importedUrl->longUrl(), - $importedUrl->shortCode(), - ), ['Generate new short-code', 'Skip'], 1); - - return $action === 'Skip'; - }; + $skipOnShortCodeConflict = static fn (): bool => $io->choice(sprintf( + 'Failed to import URL "%s" because its short-code "%s" is already in use. Do you want to generate ' + . 'a new one or skip it?', + $importedUrl->longUrl(), + $importedUrl->shortCode(), + ), ['Generate new short-code', 'Skip'], 1) === 'Skip'; $longUrl = $importedUrl->longUrl(); try { $shortUrlImporting = $this->resolveShortUrl($importedUrl, $importShortCodes, $skipOnShortCodeConflict); } catch (NonUniqueSlugException) { $io->text(sprintf('%s: Error', $longUrl)); + continue; + } catch (Throwable $e) { + $io->text(sprintf('%s: Skipped. Reason: %s.', $longUrl, $e->getMessage())); + + if ($io instanceof OutputStyle && $io->isVerbose()) { + $io->text($e->__toString()); + } + continue; } diff --git a/module/Core/src/Importer/ShortUrlImporting.php b/module/Core/src/Importer/ShortUrlImporting.php index a925c5d5..e1680517 100644 --- a/module/Core/src/Importer/ShortUrlImporting.php +++ b/module/Core/src/Importer/ShortUrlImporting.php @@ -29,7 +29,7 @@ final class ShortUrlImporting } /** - * @param iterable|ImportedShlinkVisit[] $visits + * @param iterable $visits */ public function importVisits(iterable $visits, EntityManagerInterface $em): string { diff --git a/module/Core/test/Importer/ImportedLinksProcessorTest.php b/module/Core/test/Importer/ImportedLinksProcessorTest.php index 4760448c..341b32bc 100644 --- a/module/Core/test/Importer/ImportedLinksProcessorTest.php +++ b/module/Core/test/Importer/ImportedLinksProcessorTest.php @@ -11,6 +11,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; +use RuntimeException; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Importer\ImportedLinksProcessor; @@ -81,6 +82,37 @@ class ImportedLinksProcessorTest extends TestCase $this->io->text(Argument::type('string'))->shouldHaveBeenCalledTimes($expectedCalls); } + /** @test */ + public function newUrlsWithErrorsAreSkipped(): void + { + $urls = [ + new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo', null), + new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar', 'foo'), + new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz', null), + ]; + + $importedUrlExists = $this->repo->findOneByImportedUrl(Argument::cetera())->willReturn(null); + $ensureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true); + $persist = $this->em->persist(Argument::type(ShortUrl::class))->will(function (array $args): void { + /** @var ShortUrl $shortUrl */ + [$shortUrl] = $args; + + if ($shortUrl->getShortCode() === 'baz') { + throw new RuntimeException('Whatever error'); + } + }); + + $this->processor->process($this->io->reveal(), $urls, $this->buildParams()); + + $importedUrlExists->shouldHaveBeenCalledTimes(3); + $ensureUniqueness->shouldHaveBeenCalledTimes(3); + $persist->shouldHaveBeenCalledTimes(3); + $this->io->text(Argument::containingString('Imported'))->shouldHaveBeenCalledTimes(2); + $this->io->text( + Argument::containingString('Skipped. Reason: Whatever error'), + )->shouldHaveBeenCalledOnce(); + } + /** @test */ public function alreadyImportedUrlsAreSkipped(): void {