Fixed merge conflicts

CHANGELOG.md

@@ -35,6 +35,31 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 * *Nothing*
 
 
+## 1.20.2 - 2019-12-06
+
+#### Added
+
+* *Nothing*
+
+#### Changed
+
+* *Nothing*
+
+#### Deprecated
+
+* *Nothing*
+
+#### Removed
+
+* *Nothing*
+
+#### Fixed
+
+* [#561](https://github.com/shlinkio/shlink/issues/561) Fixed `db:migrate` command failing because yaml extension is not installed, which makes config file not to be readable.
+* [#562](https://github.com/shlinkio/shlink/issues/562) Fixed internal server error being returned when renaming a tag to another tag's name. Now a meaningful API error with status 409 is returned.
+* [#555](https://github.com/shlinkio/shlink/issues/555) Fixed internal server error being returned when invalid dates are provided for new short URLs. Now a 400 is returned, as intended.
+
+
 ## 1.20.1 - 2019-11-17
 
 #### Added

migrations.php

@@ -0,0 +1,11 @@
+<?php
+
+declare(strict_types=1);
+
+return [
+    'name' => 'ShlinkMigrations',
+    'migrations_namespace' => 'ShlinkMigrations',
+    'table_name' => 'migrations',
+    'migrations_directory' => 'data/migrations',
+    'custom_template' => 'data/migrations_template.txt',
+];

migrations.yml

@@ -1,5 +0,0 @@
-name: ShlinkMigrations
-migrations_namespace: ShlinkMigrations
-table_name: migrations
-migrations_directory: data/migrations
-custom_template: data/migrations_template.txt

module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php

@@ -4,7 +4,6 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\CLI\Command\ShortUrl;
 
-use Cake\Chronos\Chronos;
 use Shlinkio\Shlink\CLI\Util\ExitCodes;
 use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
 use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException;
@@ -127,8 +126,8 @@ class GenerateShortUrlCommand extends Command
                 new Uri($longUrl),
                 $tags,
                 ShortUrlMeta::createFromParams(
-                    $this->getOptionalDate($input, 'validSince'),
+                    $input->getOption('validSince'),
-                    $this->getOptionalDate($input, 'validUntil'),
+                    $input->getOption('validUntil'),
                     $customSlug,
                     $maxVisits !== null ? (int) $maxVisits : null,
                     $input->getOption('findIfExists'),
@@ -146,10 +145,4 @@ class GenerateShortUrlCommand extends Command
             return ExitCodes::EXIT_FAILURE;
         }
     }
-
-    private function getOptionalDate(InputInterface $input, string $fieldName): ?Chronos
-    {
-        $since = $input->getOption($fieldName);
-        return $since !== null ? Chronos::parse($since) : null;
-    }
 }

module/CLI/src/Command/Tag/RenameTagCommand.php

@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\CLI\Command\Tag;
 
 use Shlinkio\Shlink\CLI\Util\ExitCodes;
+use Shlinkio\Shlink\Core\Exception\TagConflictException;
 use Shlinkio\Shlink\Core\Exception\TagNotFoundException;
 use Shlinkio\Shlink\Core\Service\Tag\TagServiceInterface;
 use Symfony\Component\Console\Command\Command;
@@ -45,7 +46,7 @@ class RenameTagCommand extends Command
             $this->tagService->renameTag($oldName, $newName);
             $io->success('Tag properly renamed.');
             return ExitCodes::EXIT_SUCCESS;
-        } catch (TagNotFoundException $e) {
+        } catch (TagNotFoundException | TagConflictException $e) {
             $io->error($e->getMessage());
             return ExitCodes::EXIT_FAILURE;
         }

module/Core/src/Exception/TagConflictException.php

@@ -0,0 +1,35 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Exception;
+
+use Fig\Http\Message\StatusCodeInterface;
+use Zend\ProblemDetails\Exception\CommonProblemDetailsExceptionTrait;
+use Zend\ProblemDetails\Exception\ProblemDetailsExceptionInterface;
+
+use function sprintf;
+
+class TagConflictException extends RuntimeException implements ProblemDetailsExceptionInterface
+{
+    use CommonProblemDetailsExceptionTrait;
+
+    private const TITLE = 'Tag conflict';
+    private const TYPE = 'TAG_CONFLICT';
+
+    public static function fromExistingTag(string $oldName, string $newName): self
+    {
+        $e = new self(sprintf('You cannot rename tag %s to %s, because it already exists', $oldName, $newName));
+
+        $e->detail = $e->getMessage();
+        $e->title = self::TITLE;
+        $e->type = self::TYPE;
+        $e->status = StatusCodeInterface::STATUS_CONFLICT;
+        $e->additional = [
+            'oldName' => $oldName,
+            'newName' => $newName,
+        ];
+
+        return $e;
+    }
+}

module/Core/src/Model/ShortUrlMeta.php

@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\Core\Model;
 
 use Cake\Chronos\Chronos;
+use DateTimeInterface;
 use Shlinkio\Shlink\Core\Exception\ValidationException;
 use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter;
 
@@ -96,7 +97,7 @@ final class ShortUrlMeta
     }
 
     /**
-     * @param string|Chronos|null $date
+     * @param string|DateTimeInterface|Chronos|null $date
      */
     private function parseDateField($date): ?Chronos
     {
@@ -104,6 +105,10 @@ final class ShortUrlMeta
             return $date;
         }
 
+        if ($date instanceof DateTimeInterface) {
+            return Chronos::instance($date);
+        }
+
         return Chronos::parse($date);
     }
 

module/Core/src/Service/Tag/TagService.php

@@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\Service\Tag;
 use Doctrine\Common\Collections\Collection;
 use Doctrine\ORM;
 use Shlinkio\Shlink\Core\Entity\Tag;
+use Shlinkio\Shlink\Core\Exception\TagConflictException;
 use Shlinkio\Shlink\Core\Exception\TagNotFoundException;
 use Shlinkio\Shlink\Core\Repository\TagRepository;
 use Shlinkio\Shlink\Core\Util\TagManagerTrait;
@@ -60,15 +61,24 @@ class TagService implements TagServiceInterface
 
     /**
      * @throws TagNotFoundException
+     * @throws TagConflictException
      */
     public function renameTag(string $oldName, string $newName): Tag
     {
+        /** @var TagRepository $repo */
+        $repo = $this->em->getRepository(Tag::class);
+
         /** @var Tag|null $tag */
-        $tag = $this->em->getRepository(Tag::class)->findOneBy(['name' => $oldName]);
+        $tag = $repo->findOneBy(['name' => $oldName]);
         if ($tag === null) {
             throw TagNotFoundException::fromTag($oldName);
         }
 
+        $newNameExists = $newName !== $oldName && $repo->count(['name' => $newName]) > 0;
+        if ($newNameExists) {
+            throw TagConflictException::fromExistingTag($oldName, $newName);
+        }
+
         $tag->rename($newName);
         $this->em->flush();
 

module/Core/src/Service/Tag/TagServiceInterface.php

@@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Service\Tag;
 
 use Doctrine\Common\Collections\Collection;
 use Shlinkio\Shlink\Core\Entity\Tag;
+use Shlinkio\Shlink\Core\Exception\TagConflictException;
 use Shlinkio\Shlink\Core\Exception\TagNotFoundException;
 
 interface TagServiceInterface
@@ -28,6 +29,7 @@ interface TagServiceInterface
 
     /**
      * @throws TagNotFoundException
+     * @throws TagConflictException
      */
     public function renameTag(string $oldName, string $newName): Tag;
 }

module/Core/test/Model/ShortUrlMetaTest.php

@@ -9,6 +9,7 @@ use PHPUnit\Framework\TestCase;
 use Shlinkio\Shlink\Core\Exception\ValidationException;
 use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
 use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter;
+use stdClass;
 
 class ShortUrlMetaTest extends TestCase
 {
@@ -35,6 +36,14 @@ class ShortUrlMetaTest extends TestCase
             ShortUrlMetaInputFilter::VALID_SINCE => '2017',
             ShortUrlMetaInputFilter::MAX_VISITS => 5,
         ]];
+        yield [[
+            ShortUrlMetaInputFilter::VALID_SINCE => new stdClass(),
+            ShortUrlMetaInputFilter::VALID_UNTIL => 'foo',
+        ]];
+        yield [[
+            ShortUrlMetaInputFilter::VALID_UNTIL => 500,
+            ShortUrlMetaInputFilter::DOMAIN => 4,
+        ]];
     }
 
     /** @test */

module/Core/test/Service/Tag/TagServiceTest.php

@@ -10,6 +10,7 @@ use PHPUnit\Framework\TestCase;
 use Prophecy\Argument;
 use Prophecy\Prophecy\ObjectProphecy;
 use Shlinkio\Shlink\Core\Entity\Tag;
+use Shlinkio\Shlink\Core\Exception\TagConflictException;
 use Shlinkio\Shlink\Core\Exception\TagNotFoundException;
 use Shlinkio\Shlink\Core\Repository\TagRepository;
 use Shlinkio\Shlink\Core\Service\Tag\TagService;
@@ -88,22 +89,51 @@ class TagServiceTest extends TestCase
         $this->service->renameTag('foo', 'bar');
     }
 
-    /** @test */
+    /**
-    public function renameValidTagChangesItsName(): void
+     * @test
+     * @dataProvider provideValidRenames
+     */
+    public function renameValidTagChangesItsName(string $oldName, string $newName, int $count): void
     {
         $expected = new Tag('foo');
 
         $repo = $this->prophesize(TagRepository::class);
         $find = $repo->findOneBy(Argument::cetera())->willReturn($expected);
+        $countTags = $repo->count(Argument::cetera())->willReturn($count);
         $getRepo = $this->em->getRepository(Tag::class)->willReturn($repo->reveal());
         $flush = $this->em->flush()->willReturn(null);
 
-        $tag = $this->service->renameTag('foo', 'bar');
+        $tag = $this->service->renameTag($oldName, $newName);
 
         $this->assertSame($expected, $tag);
-        $this->assertEquals('bar', (string) $tag);
+        $this->assertEquals($newName, (string) $tag);
         $find->shouldHaveBeenCalled();
         $getRepo->shouldHaveBeenCalled();
         $flush->shouldHaveBeenCalled();
+        $countTags->shouldHaveBeenCalledTimes($count > 0 ? 0 : 1);
+    }
+
+    public function provideValidRenames(): iterable
+    {
+        yield 'same names' => ['foo', 'foo', 1];
+        yield 'different names names' => ['foo', 'bar', 0];
+    }
+
+    /** @test */
+    public function renameTagToAnExistingNameThrowsException(): void
+    {
+        $repo = $this->prophesize(TagRepository::class);
+        $find = $repo->findOneBy(Argument::cetera())->willReturn(new Tag('foo'));
+        $countTags = $repo->count(Argument::cetera())->willReturn(1);
+        $getRepo = $this->em->getRepository(Tag::class)->willReturn($repo->reveal());
+        $flush = $this->em->flush(Argument::any())->willReturn(null);
+
+        $find->shouldBeCalled();
+        $getRepo->shouldBeCalled();
+        $countTags->shouldBeCalled();
+        $flush->shouldNotBeCalled();
+        $this->expectException(TagConflictException::class);
+
+        $this->service->renameTag('foo', 'bar');
     }
 }

module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php

@@ -4,7 +4,6 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Rest\Action\ShortUrl;
 
-use Cake\Chronos\Chronos;
 use Psr\Http\Message\ServerRequestInterface as Request;
 use Shlinkio\Shlink\Core\Exception\ValidationException;
 use Shlinkio\Shlink\Core\Model\CreateShortUrlData;
@@ -31,8 +30,8 @@ class CreateShortUrlAction extends AbstractCreateShortUrlAction
         }
 
         $meta = ShortUrlMeta::createFromParams(
-            $this->getOptionalDate($postData, 'validSince'),
+            $postData['validSince'] ?? null,
-            $this->getOptionalDate($postData, 'validUntil'),
+            $postData['validUntil'] ?? null,
             $postData['customSlug'] ?? null,
             $postData['maxVisits'] ?? null,
             $postData['findIfExists'] ?? null,
@@ -41,9 +40,4 @@ class CreateShortUrlAction extends AbstractCreateShortUrlAction
 
         return new CreateShortUrlData(new Uri($postData['longUrl']), (array) ($postData['tags'] ?? []), $meta);
     }
-
-    private function getOptionalDate(array $postData, string $fieldName): ?Chronos
-    {
-        return isset($postData[$fieldName]) ? Chronos::parse($postData[$fieldName]) : null;
-    }
 }

module/Rest/test-api/Action/UpdateTagActionTest.php

@@ -55,4 +55,35 @@ class UpdateTagActionTest extends ApiTestCase
         $this->assertEquals($expectedDetail, $payload['message']); // Deprecated
         $this->assertEquals('Tag not found', $payload['title']);
     }
+
+    /** @test */
+    public function errorIsThrownWhenTryingToRenameTagToAnotherTagName(): void
+    {
+        $expectedDetail = 'You cannot rename tag foo to bar, because it already exists';
+
+        $resp = $this->callApiWithKey(self::METHOD_PUT, '/tags', [RequestOptions::JSON => [
+            'oldName' => 'foo',
+            'newName' => 'bar',
+        ]]);
+        $payload = $this->getJsonResponsePayload($resp);
+
+        $this->assertEquals(self::STATUS_CONFLICT, $resp->getStatusCode());
+        $this->assertEquals(self::STATUS_CONFLICT, $payload['status']);
+        $this->assertEquals('TAG_CONFLICT', $payload['type']);
+        $this->assertEquals('TAG_CONFLICT', $payload['error']); // Deprecated
+        $this->assertEquals($expectedDetail, $payload['detail']);
+        $this->assertEquals($expectedDetail, $payload['message']); // Deprecated
+        $this->assertEquals('Tag conflict', $payload['title']);
+    }
+
+    /** @test */
+    public function tagIsProperlyRenamedWhenRenamingToItself(): void
+    {
+        $resp = $this->callApiWithKey(self::METHOD_PUT, '/tags', [RequestOptions::JSON => [
+            'oldName' => 'foo',
+            'newName' => 'foo',
+        ]]);
+
+        $this->assertEquals(self::STATUS_NO_CONTENT, $resp->getStatusCode());
+    }
 }