From 430c407106d0dfb7474580caa54835f3745f91cb Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 2 Feb 2021 20:21:48 +0100 Subject: [PATCH 01/18] Added support for an optional title field in short URLs --- data/migrations/Version20210202181026.php | 32 +++++++++++++++++++ .../Shlinkio.Shlink.Core.Entity.ShortUrl.php | 6 ++++ module/Core/src/Entity/ShortUrl.php | 7 ++++ module/Core/src/Model/ShortUrlMeta.php | 7 ++++ module/Core/src/Model/ShortUrlsOrdering.php | 7 ++-- .../Transformer/ShortUrlDataTransformer.php | 1 + .../src/Validation/ShortUrlInputFilter.php | 5 +++ .../Mercure/MercureUpdatesGeneratorTest.php | 13 +++++--- 8 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 data/migrations/Version20210202181026.php diff --git a/data/migrations/Version20210202181026.php b/data/migrations/Version20210202181026.php new file mode 100644 index 00000000..00e82a40 --- /dev/null +++ b/data/migrations/Version20210202181026.php @@ -0,0 +1,32 @@ +getTable('short_urls'); + $this->skipIf($shortUrls->hasColumn(self::TITLE)); + + $shortUrls->addColumn(self::TITLE, Types::STRING, [ + 'notnull' => false, + 'length' => 512, + ]); + } + + public function down(Schema $schema): void + { + $shortUrls = $schema->getTable('short_urls'); + $this->skipIf(! $shortUrls->hasColumn(self::TITLE)); + $shortUrls->dropColumn(self::TITLE); + } +} diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php index da4506af..df17ad95 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php @@ -84,4 +84,10 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->build(); $builder->addUniqueConstraint(['short_code', 'domain_id'], 'unique_short_code_plus_domain'); + + $builder->createField('title', Types::STRING) + ->columnName('title') + ->length(512) + ->nullable() + ->build(); }; diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index c41d506e..fd061996 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -38,6 +38,7 @@ class ShortUrl extends AbstractEntity private ?string $importSource = null; private ?string $importOriginalShortCode = null; private ?ApiKey $authorApiKey = null; + private ?string $title = null; private function __construct() { @@ -72,6 +73,7 @@ class ShortUrl extends AbstractEntity $instance->shortCode = $meta->getCustomSlug() ?? generateRandomShortCode($instance->shortCodeLength); $instance->domain = $relationResolver->resolveDomain($meta->getDomain()); $instance->authorApiKey = $meta->getApiKey(); + $instance->title = $meta->getTitle(); return $instance; } @@ -157,6 +159,11 @@ class ShortUrl extends AbstractEntity return $this->maxVisits; } + public function getTitle(): ?string + { + return $this->title; + } + public function update( ShortUrlEdit $shortUrlEdit, ?ShortUrlRelationResolverInterface $relationResolver = null diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index 13f36362..65ff5e1e 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -28,6 +28,7 @@ final class ShortUrlMeta private ?bool $validateUrl = null; private ?ApiKey $apiKey = null; private array $tags = []; + private ?string $title = null; private function __construct() { @@ -76,6 +77,7 @@ final class ShortUrlMeta ) ?? DEFAULT_SHORT_CODES_LENGTH; $this->apiKey = $inputFilter->getValue(ShortUrlInputFilter::API_KEY); $this->tags = $inputFilter->getValue(ShortUrlInputFilter::TAGS); + $this->title = $inputFilter->getValue(ShortUrlInputFilter::TITLE); } public function getLongUrl(): string @@ -160,4 +162,9 @@ final class ShortUrlMeta { return $this->tags; } + + public function getTitle(): ?string + { + return $this->title; + } } diff --git a/module/Core/src/Model/ShortUrlsOrdering.php b/module/Core/src/Model/ShortUrlsOrdering.php index e1708a86..b59435ca 100644 --- a/module/Core/src/Model/ShortUrlsOrdering.php +++ b/module/Core/src/Model/ShortUrlsOrdering.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Model; use Shlinkio\Shlink\Core\Exception\ValidationException; +use function array_pad; use function explode; use function is_array; use function is_string; @@ -50,9 +51,9 @@ final class ShortUrlsOrdering /** @var string|array $orderBy */ if (! $isArray) { - $parts = explode('-', $orderBy); - $this->orderField = $parts[0]; - $this->orderDirection = $parts[1] ?? self::DEFAULT_ORDER_DIRECTION; + [$field, $dir] = array_pad(explode('-', $orderBy), 2, null); + $this->orderField = $field; + $this->orderDirection = $dir ?? self::DEFAULT_ORDER_DIRECTION; } else { $this->orderField = key($orderBy); $this->orderDirection = $orderBy[$this->orderField]; diff --git a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php index 462178a0..ce459714 100644 --- a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php +++ b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php @@ -34,6 +34,7 @@ class ShortUrlDataTransformer implements DataTransformerInterface 'tags' => invoke($shortUrl->getTags(), '__toString'), 'meta' => $this->buildMeta($shortUrl), 'domain' => $shortUrl->getDomain(), + 'title' => $shortUrl->getTitle(), ]; } diff --git a/module/Core/src/Validation/ShortUrlInputFilter.php b/module/Core/src/Validation/ShortUrlInputFilter.php index fa333b49..b5d4fa07 100644 --- a/module/Core/src/Validation/ShortUrlInputFilter.php +++ b/module/Core/src/Validation/ShortUrlInputFilter.php @@ -31,6 +31,7 @@ class ShortUrlInputFilter extends InputFilter public const VALIDATE_URL = 'validateUrl'; public const API_KEY = 'apiKey'; public const TAGS = 'tags'; + public const TITLE = 'title'; private function __construct(array $data, bool $requireLongUrl) { @@ -87,6 +88,8 @@ class ShortUrlInputFilter extends InputFilter $this->add($this->createBooleanInput(self::FIND_IF_EXISTS, false)); + // This cannot be defined as a boolean input because it can actually have 3 values, true, false and null. + // Defining it as boolean will make null fall back to false, which is not the desired behavior. $this->add($this->createInput(self::VALIDATE_URL, false)); $domain = $this->createInput(self::DOMAIN, false); @@ -100,5 +103,7 @@ class ShortUrlInputFilter extends InputFilter $this->add($apiKeyInput); $this->add($this->createTagsInput(self::TAGS, false)); + + $this->add($this->createInput(self::TITLE, false)); } } diff --git a/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php b/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php index c3a8463f..435fb4d8 100644 --- a/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php +++ b/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php @@ -28,9 +28,13 @@ class MercureUpdatesGeneratorTest extends TestCase * @test * @dataProvider provideMethod */ - public function visitIsProperlySerializedIntoUpdate(string $method, string $expectedTopic): void + public function visitIsProperlySerializedIntoUpdate(string $method, string $expectedTopic, ?string $title): void { - $shortUrl = ShortUrl::fromMeta(ShortUrlMeta::fromRawData(['customSlug' => 'foo', 'longUrl' => ''])); + $shortUrl = ShortUrl::fromMeta(ShortUrlMeta::fromRawData([ + 'customSlug' => 'foo', + 'longUrl' => '', + 'title' => $title, + ])); $visit = new Visit($shortUrl, Visitor::emptyInstance()); $update = $this->generator->{$method}($visit); @@ -50,6 +54,7 @@ class MercureUpdatesGeneratorTest extends TestCase 'maxVisits' => null, ], 'domain' => null, + 'title' => $title, ], 'visit' => [ 'referer' => '', @@ -62,7 +67,7 @@ class MercureUpdatesGeneratorTest extends TestCase public function provideMethod(): iterable { - yield 'newVisitUpdate' => ['newVisitUpdate', 'https://shlink.io/new-visit']; - yield 'newShortUrlVisitUpdate' => ['newShortUrlVisitUpdate', 'https://shlink.io/new-visit/foo']; + yield 'newVisitUpdate' => ['newVisitUpdate', 'https://shlink.io/new-visit', 'the cool title']; + yield 'newShortUrlVisitUpdate' => ['newShortUrlVisitUpdate', 'https://shlink.io/new-visit/foo', null]; } } From 356e68ca3e5d9f80604c2e754e6524b649dd67cd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 2 Feb 2021 20:51:28 +0100 Subject: [PATCH 02/18] Documented new title prop in swagger docs --- docs/swagger/definitions/ShortUrl.json | 6 ++++++ docs/swagger/paths/v1_short-urls.json | 13 +++++++++--- docs/swagger/paths/v1_short-urls_shorten.json | 3 ++- .../paths/v1_short-urls_{shortCode}.json | 20 ++++++++++++++----- module/Core/src/Entity/ShortUrl.php | 3 +++ module/Core/src/Model/ShortUrlEdit.php | 14 +++++++++++++ .../test-api/Action/ListShortUrlsTest.php | 6 ++++++ .../test-api/Fixtures/ShortUrlsFixture.php | 1 + 8 files changed, 57 insertions(+), 9 deletions(-) diff --git a/docs/swagger/definitions/ShortUrl.json b/docs/swagger/definitions/ShortUrl.json index 66d20115..3e4c6ead 100644 --- a/docs/swagger/definitions/ShortUrl.json +++ b/docs/swagger/definitions/ShortUrl.json @@ -34,7 +34,13 @@ }, "domain": { "type": "string", + "nullable": true, "description": "The domain in which the short URL was created. Null if it belongs to default domain." + }, + "title": { + "type": "string", + "nullable": true, + "description": "A descriptive title of the short URL." } } } diff --git a/docs/swagger/paths/v1_short-urls.json b/docs/swagger/paths/v1_short-urls.json index 1dc10978..c3db1f05 100644 --- a/docs/swagger/paths/v1_short-urls.json +++ b/docs/swagger/paths/v1_short-urls.json @@ -137,7 +137,8 @@ "validUntil": null, "maxVisits": 100 }, - "domain": null + "domain": null, + "title": "Welcome to Steam" }, { "shortCode": "12Kb3", @@ -153,7 +154,8 @@ "validUntil": null, "maxVisits": null }, - "domain": null + "domain": null, + "title": null }, { "shortCode": "123bA", @@ -167,7 +169,8 @@ "validUntil": null, "maxVisits": null }, - "domain": "example.com" + "domain": "example.com", + "title": null } ], "pagination": { @@ -264,6 +267,10 @@ "validateUrl": { "description": "Tells if the long URL should or should not be validated as a reachable URL. If not provided, it will fall back to app-level config", "type": "boolean" + }, + "title": { + "type": "string", + "description": "A descriptive title of the short URL." } } } diff --git a/docs/swagger/paths/v1_short-urls_shorten.json b/docs/swagger/paths/v1_short-urls_shorten.json index c31c0cd9..b6184d8d 100644 --- a/docs/swagger/paths/v1_short-urls_shorten.json +++ b/docs/swagger/paths/v1_short-urls_shorten.json @@ -73,7 +73,8 @@ "validUntil": null, "maxVisits": 100 }, - "domain": null + "domain": null, + "title": null }, "text/plain": "https://doma.in/abc123" } diff --git a/docs/swagger/paths/v1_short-urls_{shortCode}.json b/docs/swagger/paths/v1_short-urls_{shortCode}.json index 6cfa3c97..2281d9b8 100644 --- a/docs/swagger/paths/v1_short-urls_{shortCode}.json +++ b/docs/swagger/paths/v1_short-urls_{shortCode}.json @@ -53,7 +53,8 @@ "validUntil": null, "maxVisits": 100 }, - "domain": null + "domain": null, + "title": null } } }, @@ -118,15 +119,18 @@ }, "validSince": { "description": "The date (in ISO-8601 format) from which this short code will be valid", - "type": "string" + "type": "string", + "nullable": true }, "validUntil": { "description": "The date (in ISO-8601 format) until which this short code will be valid", - "type": "string" + "type": "string", + "nullable": true }, "maxVisits": { "description": "The maximum number of allowed visits for this short code", - "type": "number" + "type": "number", + "nullable": true }, "validateUrl": { "description": "Tells if the long URL (if provided) should or should not be validated as a reachable URL. If not provided, it will fall back to app-level config", @@ -138,6 +142,11 @@ "type": "string" }, "description": "The list of tags to set to the short URL." + }, + "title": { + "type": "string", + "description": "A descriptive title of the short URL.", + "nullable": true } } } @@ -174,7 +183,8 @@ "validUntil": null, "maxVisits": 100 }, - "domain": null + "domain": null, + "title": "Shlink - The URL shortener" } } }, diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index fd061996..ba7f34f4 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -184,6 +184,9 @@ class ShortUrl extends AbstractEntity $relationResolver = $relationResolver ?? new SimpleShortUrlRelationResolver(); $this->tags = $relationResolver->resolveTags($shortUrlEdit->tags()); } + if ($shortUrlEdit->hasTitle()) { + $this->title = $shortUrlEdit->title(); + } } /** diff --git a/module/Core/src/Model/ShortUrlEdit.php b/module/Core/src/Model/ShortUrlEdit.php index b8cb0e0c..ddb7d317 100644 --- a/module/Core/src/Model/ShortUrlEdit.php +++ b/module/Core/src/Model/ShortUrlEdit.php @@ -25,6 +25,8 @@ final class ShortUrlEdit private ?int $maxVisits = null; private bool $tagsPropWasProvided = false; private array $tags = []; + private bool $titlePropWasProvided = false; + private ?string $title = null; private ?bool $validateUrl = null; private function __construct() @@ -56,6 +58,7 @@ final class ShortUrlEdit $this->validUntilPropWasProvided = array_key_exists(ShortUrlInputFilter::VALID_UNTIL, $data); $this->maxVisitsPropWasProvided = array_key_exists(ShortUrlInputFilter::MAX_VISITS, $data); $this->tagsPropWasProvided = array_key_exists(ShortUrlInputFilter::TAGS, $data); + $this->titlePropWasProvided = array_key_exists(ShortUrlInputFilter::TITLE, $data); $this->longUrl = $inputFilter->getValue(ShortUrlInputFilter::LONG_URL); $this->validSince = parseDateField($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE)); @@ -63,6 +66,7 @@ final class ShortUrlEdit $this->maxVisits = getOptionalIntFromInputFilter($inputFilter, ShortUrlInputFilter::MAX_VISITS); $this->validateUrl = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL); $this->tags = $inputFilter->getValue(ShortUrlInputFilter::TAGS); + $this->title = $inputFilter->getValue(ShortUrlInputFilter::TITLE); } public function longUrl(): ?string @@ -118,6 +122,16 @@ final class ShortUrlEdit return $this->tagsPropWasProvided; } + public function title(): ?string + { + return $this->title; + } + + public function hasTitle(): bool + { + return $this->titlePropWasProvided; + } + public function doValidateUrl(): ?bool { return $this->validateUrl; diff --git a/module/Rest/test-api/Action/ListShortUrlsTest.php b/module/Rest/test-api/Action/ListShortUrlsTest.php index e38374c8..2c7f35ae 100644 --- a/module/Rest/test-api/Action/ListShortUrlsTest.php +++ b/module/Rest/test-api/Action/ListShortUrlsTest.php @@ -25,6 +25,7 @@ class ListShortUrlsTest extends ApiTestCase 'maxVisits' => null, ], 'domain' => null, + 'title' => 'Shlink', ]; private const SHORT_URL_DOCS = [ 'shortCode' => 'ghi789', @@ -39,6 +40,7 @@ class ListShortUrlsTest extends ApiTestCase 'maxVisits' => null, ], 'domain' => null, + 'title' => null, ]; private const SHORT_URL_CUSTOM_SLUG_AND_DOMAIN = [ 'shortCode' => 'custom-with-domain', @@ -53,6 +55,7 @@ class ListShortUrlsTest extends ApiTestCase 'maxVisits' => null, ], 'domain' => 'some-domain.com', + 'title' => null, ]; private const SHORT_URL_META = [ 'shortCode' => 'def456', @@ -69,6 +72,7 @@ class ListShortUrlsTest extends ApiTestCase 'maxVisits' => null, ], 'domain' => null, + 'title' => null, ]; private const SHORT_URL_CUSTOM_SLUG = [ 'shortCode' => 'custom', @@ -83,6 +87,7 @@ class ListShortUrlsTest extends ApiTestCase 'maxVisits' => 2, ], 'domain' => null, + 'title' => null, ]; private const SHORT_URL_CUSTOM_DOMAIN = [ 'shortCode' => 'ghi789', @@ -99,6 +104,7 @@ class ListShortUrlsTest extends ApiTestCase 'maxVisits' => null, ], 'domain' => 'example.com', + 'title' => null, ]; /** diff --git a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php index 93defe90..426bc4f4 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php @@ -34,6 +34,7 @@ class ShortUrlsFixture extends AbstractFixture implements DependentFixtureInterf 'apiKey' => $authorApiKey, 'longUrl' => 'https://shlink.io', 'tags' => ['foo'], + 'title' => 'Shlink', ]), $relationResolver), '2018-05-01', ); From 8b5409829974bd5af02bea8aaaf34238297c5906 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 3 Feb 2021 11:07:47 +0100 Subject: [PATCH 03/18] Added option to automatically resolve url titles --- config/autoload/url-shortener.global.php | 1 + module/Core/src/Model/ShortUrlMeta.php | 13 ++++++++ .../Core/src/Options/UrlShortenerOptions.php | 12 ++++++++ module/Core/src/Service/UrlShortener.php | 10 +++++-- module/Core/src/Util/UrlValidator.php | 30 +++++++++++++++++-- .../Core/src/Util/UrlValidatorInterface.php | 5 ++++ module/Core/test/Service/UrlShortenerTest.php | 4 +-- 7 files changed, 68 insertions(+), 7 deletions(-) diff --git a/config/autoload/url-shortener.global.php b/config/autoload/url-shortener.global.php index f27210af..f4a7966a 100644 --- a/config/autoload/url-shortener.global.php +++ b/config/autoload/url-shortener.global.php @@ -19,6 +19,7 @@ return [ 'default_short_codes_length' => DEFAULT_SHORT_CODES_LENGTH, 'redirect_status_code' => DEFAULT_REDIRECT_STATUS_CODE, 'redirect_cache_lifetime' => DEFAULT_REDIRECT_CACHE_LIFETIME, + 'auto_resolve_titles' => false, // Deprecated value. Default to true with Shlink 3.0.0 ], ]; diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index 65ff5e1e..a069062c 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -167,4 +167,17 @@ final class ShortUrlMeta { return $this->title; } + + public function hasTitle(): bool + { + return $this->title !== null; + } + + public function withResolvedTitle(?string $title): self + { + $copy = clone $this; + $copy->title = $title; + + return $copy; + } } diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php index 92bb7d07..553a160f 100644 --- a/module/Core/src/Options/UrlShortenerOptions.php +++ b/module/Core/src/Options/UrlShortenerOptions.php @@ -18,6 +18,7 @@ class UrlShortenerOptions extends AbstractOptions private bool $validateUrl = true; private int $redirectStatusCode = DEFAULT_REDIRECT_STATUS_CODE; private int $redirectCacheLifetime = DEFAULT_REDIRECT_CACHE_LIFETIME; + private bool $autoResolveTitles = false; // Deprecated value. Default to true with Shlink 3.0.0 public function isUrlValidationEnabled(): bool { @@ -55,4 +56,15 @@ class UrlShortenerOptions extends AbstractOptions ? $redirectCacheLifetime : DEFAULT_REDIRECT_CACHE_LIFETIME; } + + public function autoResolveTitles(): bool + { + return $this->autoResolveTitles; + } + + protected function setAutoResolveTitles(bool $autoResolveTitles): self + { + $this->autoResolveTitles = $autoResolveTitles; + return $this; + } } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index f8125524..aa0908fe 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -13,7 +13,6 @@ use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; -use Throwable; class UrlShortener implements UrlShortenerInterface { @@ -37,7 +36,6 @@ class UrlShortener implements UrlShortenerInterface /** * @throws NonUniqueSlugException * @throws InvalidUrlException - * @throws Throwable */ public function shorten(ShortUrlMeta $meta): ShortUrl { @@ -47,7 +45,13 @@ class UrlShortener implements UrlShortenerInterface return $existingShortUrl; } - $this->urlValidator->validateUrl($meta->getLongUrl(), $meta->doValidateUrl()); + if ($meta->hasTitle()) { + $this->urlValidator->validateUrl($meta->getLongUrl(), $meta->doValidateUrl()); + } else { + $meta = $meta->withResolvedTitle( + $this->urlValidator->validateUrlWithTitle($meta->getLongUrl(), $meta->doValidateUrl()), + ); + } return $this->em->transactional(function () use ($meta) { $shortUrl = ShortUrl::fromMeta($meta, $this->relationResolver); diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php index ccf69dd1..1f590de5 100644 --- a/module/Core/src/Util/UrlValidator.php +++ b/module/Core/src/Util/UrlValidator.php @@ -8,9 +8,12 @@ use Fig\Http\Message\RequestMethodInterface; use GuzzleHttp\ClientInterface; use GuzzleHttp\Exception\GuzzleException; use GuzzleHttp\RequestOptions; +use Psr\Http\Message\ResponseInterface; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; +use function preg_match; + class UrlValidator implements UrlValidatorInterface, RequestMethodInterface { private const MAX_REDIRECTS = 15; @@ -35,13 +38,36 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface return; } + $this->validateUrlAndGetResponse($url, true); + } + + public function validateUrlWithTitle(string $url, ?bool $doValidate): ?string + { + $doValidate = $doValidate ?? $this->options->isUrlValidationEnabled(); + $response = $this->validateUrlAndGetResponse($url, $doValidate); + + if ($response === null || ! $this->options->autoResolveTitles()) { + return null; + } + + $body = $response->getBody()->__toString(); + preg_match('/(.+)<\/title>/i', $body, $matches); + return $matches[1] ?? null; + } + + private function validateUrlAndGetResponse(string $url, bool $throwOnError): ?ResponseInterface + { try { - $this->httpClient->request(self::METHOD_GET, $url, [ + return $this->httpClient->request(self::METHOD_GET, $url, [ RequestOptions::ALLOW_REDIRECTS => ['max' => self::MAX_REDIRECTS], RequestOptions::IDN_CONVERSION => true, ]); } catch (GuzzleException $e) { - throw InvalidUrlException::fromUrl($url, $e); + if ($throwOnError) { + throw InvalidUrlException::fromUrl($url, $e); + } + + return null; } } } diff --git a/module/Core/src/Util/UrlValidatorInterface.php b/module/Core/src/Util/UrlValidatorInterface.php index fdf1e781..f198d301 100644 --- a/module/Core/src/Util/UrlValidatorInterface.php +++ b/module/Core/src/Util/UrlValidatorInterface.php @@ -12,4 +12,9 @@ interface UrlValidatorInterface * @throws InvalidUrlException */ public function validateUrl(string $url, ?bool $doValidate): void; + + /** + * @throws InvalidUrlException + */ + public function validateUrlWithTitle(string $url, ?bool $doValidate): ?string; } diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index a9ba783f..24abf69f 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -31,7 +31,7 @@ class UrlShortenerTest extends TestCase public function setUp(): void { $this->urlValidator = $this->prophesize(UrlValidatorInterface::class); - $this->urlValidator->validateUrl('http://foobar.com/12345/hello?foo=bar', null)->will( + $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', null)->will( function (): void { }, ); @@ -101,7 +101,7 @@ class UrlShortenerTest extends TestCase $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); - $this->urlValidator->validateUrl(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->urlValidator->validateUrlWithTitle(Argument::cetera())->shouldNotHaveBeenCalled(); self::assertSame($expected, $result); } From 71f85350dae68b4d518cd1186ff96c097ebcae89 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya <alejandrocelaya@gmail.com> Date: Wed, 3 Feb 2021 11:28:40 +0100 Subject: [PATCH 04/18] Fixed regex to parse title from URL to consider possible attributes --- module/Core/src/Util/UrlValidator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php index 1f590de5..e1ae159b 100644 --- a/module/Core/src/Util/UrlValidator.php +++ b/module/Core/src/Util/UrlValidator.php @@ -51,7 +51,7 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface } $body = $response->getBody()->__toString(); - preg_match('/<title>(.+)<\/title>/i', $body, $matches); + preg_match('/<title[^>]*>(.*?)<\/title>/i', $body, $matches); return $matches[1] ?? null; } From bfba05c8634a739ffcb49de7be3204523331c6db Mon Sep 17 00:00:00 2001 From: Alejandro Celaya <alejandrocelaya@gmail.com> Date: Wed, 3 Feb 2021 11:53:08 +0100 Subject: [PATCH 05/18] Enhanced UrlValidatorTest --- module/Core/functions/functions.php | 1 + module/Core/src/Util/UrlValidator.php | 7 ++- module/Core/test/Util/UrlValidatorTest.php | 50 ++++++++++++++++++++++ 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index 531f8038..f9a67e3d 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -26,6 +26,7 @@ const DEFAULT_REDIRECT_STATUS_CODE = StatusCodeInterface::STATUS_FOUND; const DEFAULT_REDIRECT_CACHE_LIFETIME = 30; const LOCAL_LOCK_FACTORY = 'Shlinkio\Shlink\LocalLockFactory'; const CUSTOM_SLUGS_REGEXP = '/[^\pL\pN._~]/u'; // Any unicode letter or number, plus ".", "_" and "~" chars +const TITLE_TAG_VALUE = '/<title[^>]*>(.*?)<\/title>/i'; // Matches the value inside an html title tag function generateRandomShortCode(int $length): string { diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php index e1ae159b..8d05cbe6 100644 --- a/module/Core/src/Util/UrlValidator.php +++ b/module/Core/src/Util/UrlValidator.php @@ -13,6 +13,9 @@ use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use function preg_match; +use function trim; + +use const Shlinkio\Shlink\Core\TITLE_TAG_VALUE; class UrlValidator implements UrlValidatorInterface, RequestMethodInterface { @@ -51,8 +54,8 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface } $body = $response->getBody()->__toString(); - preg_match('/<title[^>]*>(.*?)<\/title>/i', $body, $matches); - return $matches[1] ?? null; + preg_match(TITLE_TAG_VALUE, $body, $matches); + return isset($matches[1]) ? trim($matches[1]) : null; } private function validateUrlAndGetResponse(string $url, bool $throwOnError): ?ResponseInterface diff --git a/module/Core/test/Util/UrlValidatorTest.php b/module/Core/test/Util/UrlValidatorTest.php index fab1db1e..7c5f7c55 100644 --- a/module/Core/test/Util/UrlValidatorTest.php +++ b/module/Core/test/Util/UrlValidatorTest.php @@ -9,6 +9,7 @@ use GuzzleHttp\ClientInterface; use GuzzleHttp\Exception\ClientException; use GuzzleHttp\RequestOptions; use Laminas\Diactoros\Response; +use Laminas\Diactoros\Stream; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; @@ -76,10 +77,59 @@ class UrlValidatorTest extends TestCase $request->shouldNotHaveBeenCalled(); } + /** + * @test + * @dataProvider provideDisabledCombinations + */ + public function validateUrlWithTitleReturnsNullWhenRequestFailsAndValidationIsDisabled( + ?bool $doValidate, + bool $validateUrl + ): void { + $request = $this->httpClient->request(Argument::cetera())->willThrow(ClientException::class); + $this->options->validateUrl = $validateUrl; + + $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', $doValidate); + + self::assertNull($result); + $request->shouldHaveBeenCalledOnce(); + } + public function provideDisabledCombinations(): iterable { yield 'config is disabled and no runtime option is provided' => [null, false]; yield 'config is enabled but runtime option is disabled' => [false, true]; yield 'both config and runtime option are disabled' => [false, false]; } + + /** @test */ + public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsDisabled(): void + { + $request = $this->httpClient->request(Argument::cetera())->willReturn($this->respWithTitle()); + $this->options->autoResolveTitles = false; + + $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); + + self::assertNull($result); + $request->shouldHaveBeenCalledOnce(); + } + + /** @test */ + public function validateUrlWithTitleResolvesTitleWhenAutoResolutionIsEnabled(): void + { + $request = $this->httpClient->request(Argument::cetera())->willReturn($this->respWithTitle()); + $this->options->autoResolveTitles = true; + + $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); + + self::assertEquals('Resolved title', $result); + $request->shouldHaveBeenCalledOnce(); + } + + private function respWithTitle(): Response + { + $body = new Stream('php://temp', 'wr'); + $body->write('<title> Resolved title'); + + return new Response($body); + } } From 0ef1e347e764058743dd19a73272323af593c834 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 3 Feb 2021 13:28:51 +0100 Subject: [PATCH 06/18] Enhanced UrlShortenerTest --- module/Core/test/Service/UrlShortenerTest.php | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 24abf69f..beeda044 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -31,10 +31,6 @@ class UrlShortenerTest extends TestCase public function setUp(): void { $this->urlValidator = $this->prophesize(UrlValidatorInterface::class); - $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', null)->will( - function (): void { - }, - ); $this->em = $this->prophesize(EntityManagerInterface::class); $this->em->persist(Argument::any())->will(function ($arguments): void { @@ -63,14 +59,26 @@ class UrlShortenerTest extends TestCase ); } - /** @test */ - public function urlIsProperlyShortened(): void + /** + * @test + * @dataProvider provideTitles + */ + public function urlIsProperlyShortened(?string $title, int $validateWithTitleCallsNum, int $validateCallsNum): void { - $shortUrl = $this->urlShortener->shorten( - ShortUrlMeta::fromRawData(['longUrl' => 'http://foobar.com/12345/hello?foo=bar']), - ); + $longUrl = 'http://foobar.com/12345/hello?foo=bar'; + $shortUrl = $this->urlShortener->shorten(ShortUrlMeta::fromRawData(['longUrl' => $longUrl, 'title' => $title])); self::assertEquals('http://foobar.com/12345/hello?foo=bar', $shortUrl->getLongUrl()); + $this->urlValidator->validateUrlWithTitle($longUrl, null)->shouldHaveBeenCalledTimes( + $validateWithTitleCallsNum, + ); + $this->urlValidator->validateUrl($longUrl, null)->shouldHaveBeenCalledTimes($validateCallsNum); + } + + public function provideTitles(): iterable + { + yield 'no title' => [null, 1, 0]; + yield 'title' => ['link title', 0, 1]; } /** @test */ @@ -101,6 +109,7 @@ class UrlShortenerTest extends TestCase $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->urlValidator->validateUrl(Argument::cetera())->shouldNotHaveBeenCalled(); $this->urlValidator->validateUrlWithTitle(Argument::cetera())->shouldNotHaveBeenCalled(); self::assertSame($expected, $result); } From 1da66f272c58be2bbc925b20e9b681aae50c21ed Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 3 Feb 2021 13:41:37 +0100 Subject: [PATCH 07/18] Added AUTO_RESOLVE_TITLES env var for the docker image --- docker/config/shlink_in_docker.local.php | 1 + 1 file changed, 1 insertion(+) diff --git a/docker/config/shlink_in_docker.local.php b/docker/config/shlink_in_docker.local.php index c6d7f69e..070f0974 100644 --- a/docker/config/shlink_in_docker.local.php +++ b/docker/config/shlink_in_docker.local.php @@ -125,6 +125,7 @@ return [ 'default_short_codes_length' => $helper->getDefaultShortCodesLength(), 'redirect_status_code' => (int) env('REDIRECT_STATUS_CODE', DEFAULT_REDIRECT_STATUS_CODE), 'redirect_cache_lifetime' => (int) env('REDIRECT_CACHE_LIFETIME', DEFAULT_REDIRECT_CACHE_LIFETIME), + 'auto_resolve_titles' => (bool) env('AUTO_RESOLVE_TITLES', false), // Deprecated value. Default to true ], 'not_found_redirects' => $helper->getNotFoundRedirectsConfig(), From 7192480751be9ef142539627b0a445fa07ad7ecd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 3 Feb 2021 18:26:50 +0100 Subject: [PATCH 08/18] Update installer version --- composer.json | 4 ++-- config/autoload/installer.global.php | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 8776d34d..22473890 100644 --- a/composer.json +++ b/composer.json @@ -51,7 +51,7 @@ "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^2.0", "shlinkio/shlink-importer": "^2.1", - "shlinkio/shlink-installer": "^5.3", + "shlinkio/shlink-installer": "dev-develop#1ed5ac8 as 5.4", "shlinkio/shlink-ip-geolocation": "^1.5", "symfony/console": "^5.1", "symfony/filesystem": "^5.1", @@ -64,7 +64,7 @@ "devster/ubench": "^2.1", "dms/phpunit-arraysubset-asserts": "^0.2.1", "eaglewu/swoole-ide-helper": "dev-master", - "infection/infection": "^0.20.2", + "infection/infection": "^0.21.0", "phpspec/prophecy-phpunit": "^2.0", "phpstan/phpstan": "^0.12.64", "phpunit/php-code-coverage": "^9.2", diff --git a/config/autoload/installer.global.php b/config/autoload/installer.global.php index a04d874b..7a355dbe 100644 --- a/config/autoload/installer.global.php +++ b/config/autoload/installer.global.php @@ -40,6 +40,7 @@ return [ Option\UrlShortener\IpAnonymizationConfigOption::class, Option\UrlShortener\RedirectStatusCodeConfigOption::class, Option\UrlShortener\RedirectCacheLifeTimeConfigOption::class, + Option\UrlShortener\AutoResolveTitlesConfigOption::class, ], 'installation_commands' => [ From 7824dddef723eb5aea9477a1a9330b4c77123614 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 3 Feb 2021 19:22:47 +0100 Subject: [PATCH 09/18] Added tracking to tell if short URL titles were autogenerated or not --- data/migrations/Version20210202181026.php | 12 ++++++++++++ .../Shlinkio.Shlink.Core.Entity.ShortUrl.php | 5 +++++ module/Core/src/Entity/ShortUrl.php | 2 ++ module/Core/src/Model/ShortUrlMeta.php | 9 ++++++++- module/Core/src/Service/UrlShortener.php | 19 ++++++++++++------- module/Core/test/Service/UrlShortenerTest.php | 14 ++++++++++++++ 6 files changed, 53 insertions(+), 8 deletions(-) diff --git a/data/migrations/Version20210202181026.php b/data/migrations/Version20210202181026.php index 00e82a40..c964559c 100644 --- a/data/migrations/Version20210202181026.php +++ b/data/migrations/Version20210202181026.php @@ -21,6 +21,9 @@ final class Version20210202181026 extends AbstractMigration 'notnull' => false, 'length' => 512, ]); + $shortUrls->addColumn('title_was_auto_resolved', Types::BOOLEAN, [ + 'default' => false, + ]); } public function down(Schema $schema): void @@ -28,5 +31,14 @@ final class Version20210202181026 extends AbstractMigration $shortUrls = $schema->getTable('short_urls'); $this->skipIf(! $shortUrls->hasColumn(self::TITLE)); $shortUrls->dropColumn(self::TITLE); + $shortUrls->dropColumn('title_was_auto_resolved'); + } + + /** + * @fixme Workaround for https://github.com/doctrine/migrations/issues/1104 + */ + public function isTransactional(): bool + { + return false; } } diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php index df17ad95..751e513c 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php @@ -90,4 +90,9 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->length(512) ->nullable() ->build(); + + $builder->createField('titleWasAutoResolved', Types::BOOLEAN) + ->columnName('title_was_auto_resolved') + ->option('default', false) + ->build(); }; diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index ba7f34f4..84c74874 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -39,6 +39,7 @@ class ShortUrl extends AbstractEntity private ?string $importOriginalShortCode = null; private ?ApiKey $authorApiKey = null; private ?string $title = null; + private bool $titleWasAutoResolved = false; private function __construct() { @@ -74,6 +75,7 @@ class ShortUrl extends AbstractEntity $instance->domain = $relationResolver->resolveDomain($meta->getDomain()); $instance->authorApiKey = $meta->getApiKey(); $instance->title = $meta->getTitle(); + $instance->titleWasAutoResolved = $meta->titleWasAutoResolved(); return $instance; } diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index a069062c..a741f78c 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -29,6 +29,7 @@ final class ShortUrlMeta private ?ApiKey $apiKey = null; private array $tags = []; private ?string $title = null; + private bool $titleWasAutoResolved = false; private function __construct() { @@ -173,10 +174,16 @@ final class ShortUrlMeta return $this->title !== null; } - public function withResolvedTitle(?string $title): self + public function titleWasAutoResolved(): bool + { + return $this->titleWasAutoResolved; + } + + public function withResolvedTitle(string $title): self { $copy = clone $this; $copy->title = $title; + $copy->titleWasAutoResolved = true; return $copy; } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index aa0908fe..6083e1f4 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -45,13 +45,7 @@ class UrlShortener implements UrlShortenerInterface return $existingShortUrl; } - if ($meta->hasTitle()) { - $this->urlValidator->validateUrl($meta->getLongUrl(), $meta->doValidateUrl()); - } else { - $meta = $meta->withResolvedTitle( - $this->urlValidator->validateUrlWithTitle($meta->getLongUrl(), $meta->doValidateUrl()), - ); - } + $meta = $this->processTitleAndValidateUrl($meta); return $this->em->transactional(function () use ($meta) { $shortUrl = ShortUrl::fromMeta($meta, $this->relationResolver); @@ -88,4 +82,15 @@ class UrlShortener implements UrlShortenerInterface throw NonUniqueSlugException::fromSlug($shortUrlToBeCreated->getShortCode(), $domainAuthority); } } + + private function processTitleAndValidateUrl(ShortUrlMeta $meta): ShortUrlMeta + { + if ($meta->hasTitle()) { + $this->urlValidator->validateUrl($meta->getLongUrl(), $meta->doValidateUrl()); + return $meta; + } + + $title = $this->urlValidator->validateUrlWithTitle($meta->getLongUrl(), $meta->doValidateUrl()); + return $title === null ? $meta : $meta->withResolvedTitle($title); + } } diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index beeda044..9fbf82ed 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -75,6 +75,20 @@ class UrlShortenerTest extends TestCase $this->urlValidator->validateUrl($longUrl, null)->shouldHaveBeenCalledTimes($validateCallsNum); } + /** + * @test + * @dataProvider provideTitles + */ + public function urlIsProperlyShortenedWithExpectedResolvedTitle(?string $title): void + { + $validateWithTitle = $this->urlValidator->validateUrlWithTitle(Argument::cetera())->willReturn($title); + + $shortUrl = $this->urlShortener->shorten(ShortUrlMeta::fromRawData(['longUrl' => 'foo'])); + + self::assertEquals($title, $shortUrl->getTitle()); + $validateWithTitle->shouldHaveBeenCalledOnce(); + } + public function provideTitles(): iterable { yield 'no title' => [null, 1, 0]; From 2640c7b43c6d85442ff81834627ce517aea38736 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 4 Feb 2021 15:24:27 +0100 Subject: [PATCH 10/18] Updated to a shlink-importer version that supports titles --- composer.json | 2 +- module/Core/src/Entity/ShortUrl.php | 1 + .../Repository/ShortUrlRepositoryTest.php | 2 +- module/Core/test/Entity/ShortUrlTest.php | 2 +- .../Importer/ImportedLinksProcessorTest.php | 26 +++++++++---------- 5 files changed, 17 insertions(+), 16 deletions(-) diff --git a/composer.json b/composer.json index 22473890..3f314a2c 100644 --- a/composer.json +++ b/composer.json @@ -50,7 +50,7 @@ "shlinkio/shlink-common": "dev-main#b889f5d as 3.5", "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^2.0", - "shlinkio/shlink-importer": "^2.1", + "shlinkio/shlink-importer": "dev-main#b6fc81f as 2.2", "shlinkio/shlink-installer": "dev-develop#1ed5ac8 as 5.4", "shlinkio/shlink-ip-geolocation": "^1.5", "symfony/console": "^5.1", diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 84c74874..53215a68 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -89,6 +89,7 @@ class ShortUrl extends AbstractEntity ShortUrlInputFilter::LONG_URL => $url->longUrl(), ShortUrlInputFilter::DOMAIN => $url->domain(), ShortUrlInputFilter::TAGS => $url->tags(), + ShortUrlInputFilter::TITLE => $url->title(), ShortUrlInputFilter::VALIDATE_URL => false, ]; if ($importShortCode) { diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index cf38d5a6..29694867 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -418,7 +418,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase public function importedShortUrlsAreSearchedAsExpected(): void { $buildImported = static fn (string $shortCode, ?String $domain = null) => - new ImportedShlinkUrl('', 'foo', [], Chronos::now(), $domain, $shortCode); + new ImportedShlinkUrl('', 'foo', [], Chronos::now(), $domain, $shortCode, null); $shortUrlWithoutDomain = ShortUrl::fromImport($buildImported('my-cool-slug'), true); $this->getEntityManager()->persist($shortUrlWithoutDomain); diff --git a/module/Core/test/Entity/ShortUrlTest.php b/module/Core/test/Entity/ShortUrlTest.php index 3cd607da..fceba3e2 100644 --- a/module/Core/test/Entity/ShortUrlTest.php +++ b/module/Core/test/Entity/ShortUrlTest.php @@ -64,7 +64,7 @@ class ShortUrlTest extends TestCase { yield 'no custom slug' => [ShortUrl::createEmpty()]; yield 'imported with custom slug' => [ - ShortUrl::fromImport(new ImportedShlinkUrl('', '', [], Chronos::now(), null, 'custom-slug'), true), + ShortUrl::fromImport(new ImportedShlinkUrl('', '', [], Chronos::now(), null, 'custom-slug', null), true), ]; } diff --git a/module/Core/test/Importer/ImportedLinksProcessorTest.php b/module/Core/test/Importer/ImportedLinksProcessorTest.php index 174e9afc..c294ffe5 100644 --- a/module/Core/test/Importer/ImportedLinksProcessorTest.php +++ b/module/Core/test/Importer/ImportedLinksProcessorTest.php @@ -58,9 +58,9 @@ class ImportedLinksProcessorTest extends TestCase public function newUrlsWithNoErrorsAreAllPersisted(): void { $urls = [ - new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo'), - new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar'), - new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz'), + new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo', null), + new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar', 'foo'), + new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz', null), ]; $expectedCalls = count($urls); @@ -80,11 +80,11 @@ class ImportedLinksProcessorTest extends TestCase public function alreadyImportedUrlsAreSkipped(): void { $urls = [ - new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo'), - new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar'), - new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz'), - new ImportedShlinkUrl('', 'baz2', [], Chronos::now(), null, 'baz2'), - new ImportedShlinkUrl('', 'baz3', [], Chronos::now(), null, 'baz3'), + new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo', null), + new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar', null), + new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz', null), + new ImportedShlinkUrl('', 'baz2', [], Chronos::now(), null, 'baz2', null), + new ImportedShlinkUrl('', 'baz3', [], Chronos::now(), null, 'baz3', null), ]; $contains = fn (string $needle) => fn (string $text) => str_contains($text, $needle); @@ -110,11 +110,11 @@ class ImportedLinksProcessorTest extends TestCase public function nonUniqueShortCodesAreAskedToUser(): void { $urls = [ - new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo'), - new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar'), - new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz'), - new ImportedShlinkUrl('', 'baz2', [], Chronos::now(), null, 'baz2'), - new ImportedShlinkUrl('', 'baz3', [], Chronos::now(), null, 'baz3'), + new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo', null), + new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar', null), + new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz', 'foo'), + new ImportedShlinkUrl('', 'baz2', [], Chronos::now(), null, 'baz2', null), + new ImportedShlinkUrl('', 'baz3', [], Chronos::now(), null, 'baz3', 'bar'), ]; $contains = fn (string $needle) => fn (string $text) => str_contains($text, $needle); From 16873201e93b909dc7dbaa15089be0dfebbadf50 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 4 Feb 2021 21:27:16 +0100 Subject: [PATCH 11/18] Added support to search short URLs by title --- bin/test/run-api-tests.sh | 2 +- .../src/Repository/ShortUrlRepository.php | 1 + .../test-api/Action/ListShortUrlsTest.php | 23 +++++++++++-------- .../test-api/Fixtures/ShortUrlsFixture.php | 2 +- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/bin/test/run-api-tests.sh b/bin/test/run-api-tests.sh index 06708d18..07b36881 100755 --- a/bin/test/run-api-tests.sh +++ b/bin/test/run-api-tests.sh @@ -1,6 +1,6 @@ #!/usr/bin/env sh export APP_ENV=test -export DB_DRIVER=mysql +export DB_DRIVER=postgres export TEST_ENV=api # Try to stop server just in case it hanged in last execution diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index d7ab2d66..3830cdaf 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -120,6 +120,7 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU ->andWhere($qb->expr()->orX( $qb->expr()->like('s.longUrl', ':searchPattern'), $qb->expr()->like('s.shortCode', ':searchPattern'), + $qb->expr()->like('s.title', ':searchPattern'), $qb->expr()->like('t.name', ':searchPattern'), $qb->expr()->like('d.authority', ':searchPattern'), )) diff --git a/module/Rest/test-api/Action/ListShortUrlsTest.php b/module/Rest/test-api/Action/ListShortUrlsTest.php index 2c7f35ae..f7182fe5 100644 --- a/module/Rest/test-api/Action/ListShortUrlsTest.php +++ b/module/Rest/test-api/Action/ListShortUrlsTest.php @@ -12,7 +12,7 @@ use function count; class ListShortUrlsTest extends ApiTestCase { - private const SHORT_URL_SHLINK = [ + private const SHORT_URL_SHLINK_WITH_TITLE = [ 'shortCode' => 'abc123', 'shortUrl' => 'http://doma.in/abc123', 'longUrl' => 'https://shlink.io', @@ -25,7 +25,7 @@ class ListShortUrlsTest extends ApiTestCase 'maxVisits' => null, ], 'domain' => null, - 'title' => 'Shlink', + 'title' => 'My cool title', ]; private const SHORT_URL_DOCS = [ 'shortCode' => 'ghi789', @@ -128,7 +128,7 @@ class ListShortUrlsTest extends ApiTestCase public function provideFilteredLists(): iterable { yield [[], [ - self::SHORT_URL_SHLINK, + self::SHORT_URL_SHLINK_WITH_TITLE, self::SHORT_URL_DOCS, self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, self::SHORT_URL_META, @@ -136,7 +136,7 @@ class ListShortUrlsTest extends ApiTestCase self::SHORT_URL_CUSTOM_DOMAIN, ], 'valid_api_key']; yield [['orderBy' => 'shortCode'], [ - self::SHORT_URL_SHLINK, + self::SHORT_URL_SHLINK_WITH_TITLE, self::SHORT_URL_CUSTOM_SLUG, self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, self::SHORT_URL_META, @@ -149,7 +149,7 @@ class ListShortUrlsTest extends ApiTestCase self::SHORT_URL_META, self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_SHLINK, + self::SHORT_URL_SHLINK_WITH_TITLE, ], 'valid_api_key']; yield [['orderBy' => 'shortCode-DESC'], [ self::SHORT_URL_DOCS, @@ -157,7 +157,7 @@ class ListShortUrlsTest extends ApiTestCase self::SHORT_URL_META, self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_SHLINK, + self::SHORT_URL_SHLINK_WITH_TITLE, ], 'valid_api_key']; yield [['startDate' => Chronos::parse('2018-12-01')->toAtomString()], [ self::SHORT_URL_META, @@ -165,12 +165,12 @@ class ListShortUrlsTest extends ApiTestCase self::SHORT_URL_CUSTOM_DOMAIN, ], 'valid_api_key']; yield [['endDate' => Chronos::parse('2018-12-01')->toAtomString()], [ - self::SHORT_URL_SHLINK, + self::SHORT_URL_SHLINK_WITH_TITLE, self::SHORT_URL_DOCS, self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, ], 'valid_api_key']; yield [['tags' => ['foo']], [ - self::SHORT_URL_SHLINK, + self::SHORT_URL_SHLINK_WITH_TITLE, self::SHORT_URL_META, self::SHORT_URL_CUSTOM_DOMAIN, ], 'valid_api_key']; @@ -178,17 +178,20 @@ class ListShortUrlsTest extends ApiTestCase self::SHORT_URL_META, ], 'valid_api_key']; yield [['tags' => ['foo'], 'endDate' => Chronos::parse('2018-12-01')->toAtomString()], [ - self::SHORT_URL_SHLINK, + self::SHORT_URL_SHLINK_WITH_TITLE, ], 'valid_api_key']; yield [['searchTerm' => 'alejandro'], [ self::SHORT_URL_META, self::SHORT_URL_CUSTOM_DOMAIN, ], 'valid_api_key']; + yield [['searchTerm' => 'cool'], [ + self::SHORT_URL_SHLINK_WITH_TITLE, + ], 'valid_api_key']; yield [['searchTerm' => 'example.com'], [ self::SHORT_URL_CUSTOM_DOMAIN, ], 'valid_api_key']; yield [[], [ - self::SHORT_URL_SHLINK, + self::SHORT_URL_SHLINK_WITH_TITLE, self::SHORT_URL_META, self::SHORT_URL_CUSTOM_SLUG, ], 'author_api_key']; diff --git a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php index 426bc4f4..bfc65aa0 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php @@ -34,7 +34,7 @@ class ShortUrlsFixture extends AbstractFixture implements DependentFixtureInterf 'apiKey' => $authorApiKey, 'longUrl' => 'https://shlink.io', 'tags' => ['foo'], - 'title' => 'Shlink', + 'title' => 'My cool title', ]), $relationResolver), '2018-05-01', ); From 4330a09793ff227931851b53699e0f66102d2a33 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 4 Feb 2021 21:33:26 +0100 Subject: [PATCH 12/18] Removed use of deprecated approach for ordering in ListShort --- .../src/Command/ShortUrl/ListShortUrlsCommand.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index 21beecaa..c0a99f02 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php @@ -21,8 +21,8 @@ use Symfony\Component\Console\Style\SymfonyStyle; use function array_flip; use function array_intersect_key; +use function array_pad; use function array_values; -use function count; use function explode; use function implode; use function sprintf; @@ -79,7 +79,8 @@ class ListShortUrlsCommand extends AbstractWithDateRangeCommand 'order-by', 'o', InputOption::VALUE_REQUIRED, - 'The field from which we want to order by. Pass ASC or DESC separated by a comma.', + 'The field from which you want to order by. ' + . 'Define ordering dir by passing ASC or DESC after "," or "-".', ) ->addOptionWithDeprecatedFallback( 'show-tags', @@ -178,17 +179,14 @@ class ListShortUrlsCommand extends AbstractWithDateRangeCommand return $result; } - /** - * @return array|string|null - */ - private function processOrderBy(InputInterface $input) + private function processOrderBy(InputInterface $input): ?string { $orderBy = $this->getOptionWithDeprecatedFallback($input, 'order-by'); if (empty($orderBy)) { return null; } - $orderBy = explode(',', $orderBy); - return count($orderBy) === 1 ? $orderBy[0] : [$orderBy[0] => $orderBy[1]]; + [$field, $dir] = array_pad(explode(',', $orderBy), 2, null); + return $dir === null ? $field : sprintf('%s-%s', $field, $dir); } } From ed18f10b94d53b52ce392d116638a221d56cf38c Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 4 Feb 2021 22:07:54 +0100 Subject: [PATCH 13/18] Added support to order short URLs by title --- docs/swagger/paths/v1_short-urls.json | 4 +++- .../Command/ShortUrl/ListShortUrlsCommand.php | 17 +++++++++-------- .../Core/src/Repository/ShortUrlRepository.php | 4 +++- .../Rest/test-api/Action/ListShortUrlsTest.php | 8 ++++++++ 4 files changed, 23 insertions(+), 10 deletions(-) diff --git a/docs/swagger/paths/v1_short-urls.json b/docs/swagger/paths/v1_short-urls.json index c3db1f05..b034dcf3 100644 --- a/docs/swagger/paths/v1_short-urls.json +++ b/docs/swagger/paths/v1_short-urls.json @@ -64,7 +64,9 @@ "dateCreated-ASC", "dateCreated-DESC", "visits-ASC", - "visits-DESC" + "visits-DESC", + "title-ASC", + "title-DESC" ] } }, diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index c0a99f02..24689bcb 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php @@ -19,11 +19,9 @@ use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; -use function array_flip; -use function array_intersect_key; use function array_pad; -use function array_values; use function explode; +use function Functional\map; use function implode; use function sprintf; @@ -32,12 +30,16 @@ class ListShortUrlsCommand extends AbstractWithDateRangeCommand use PagerfantaUtilsTrait; public const NAME = 'short-url:list'; - private const COLUMNS_WHITELIST = [ + private const COLUMNS_TO_SHOW = [ 'shortCode', + 'title', 'shortUrl', 'longUrl', 'dateCreated', 'visitsCount', + ]; + private const COLUMNS_TO_SHOW_WITH_TAGS = [ + ...self::COLUMNS_TO_SHOW, 'tags', ]; @@ -154,21 +156,20 @@ class ListShortUrlsCommand extends AbstractWithDateRangeCommand { $result = $this->shortUrlService->listShortUrls($params); - $headers = ['Short code', 'Short URL', 'Long URL', 'Date created', 'Visits count']; + $headers = ['Short code', 'Title', 'Short URL', 'Long URL', 'Date created', 'Visits count']; if ($showTags) { $headers[] = 'Tags'; } $rows = []; foreach ($result as $row) { + $columnsToShow = $showTags ? self::COLUMNS_TO_SHOW_WITH_TAGS : self::COLUMNS_TO_SHOW; $shortUrl = $this->transformer->transform($row); if ($showTags) { $shortUrl['tags'] = implode(', ', $shortUrl['tags']); - } else { - unset($shortUrl['tags']); } - $rows[] = array_values(array_intersect_key($shortUrl, array_flip(self::COLUMNS_WHITELIST))); + $rows[] = map($columnsToShow, fn (string $prop) => $shortUrl[$prop]); } ShlinkTable::fromOutput($output)->render($headers, $rows, $all ? null : $this->formatCurrentPageMessage( diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index 3830cdaf..f7a089b7 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -55,6 +55,7 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU $fieldName = $orderBy->orderField(); $order = $orderBy->orderDirection(); + // visitsCount and visitCount are deprecated. Only visits should work if (contains(['visits', 'visitsCount', 'visitCount'], $fieldName)) { $qb->addSelect('COUNT(DISTINCT v) AS totalVisits') ->leftJoin('s.visits', 'v') @@ -66,10 +67,11 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU // Map public field names to column names $fieldNameMap = [ - 'originalUrl' => 'longUrl', + 'originalUrl' => 'longUrl', // Deprecated 'longUrl' => 'longUrl', 'shortCode' => 'shortCode', 'dateCreated' => 'dateCreated', + 'title' => 'title', ]; if (array_key_exists($fieldName, $fieldNameMap)) { $qb->orderBy('s.' . $fieldNameMap[$fieldName], $order); diff --git a/module/Rest/test-api/Action/ListShortUrlsTest.php b/module/Rest/test-api/Action/ListShortUrlsTest.php index f7182fe5..f81524ae 100644 --- a/module/Rest/test-api/Action/ListShortUrlsTest.php +++ b/module/Rest/test-api/Action/ListShortUrlsTest.php @@ -159,6 +159,14 @@ class ListShortUrlsTest extends ApiTestCase self::SHORT_URL_CUSTOM_SLUG, self::SHORT_URL_SHLINK_WITH_TITLE, ], 'valid_api_key']; + yield [['orderBy' => 'title-DESC'], [ + self::SHORT_URL_META, + self::SHORT_URL_CUSTOM_SLUG, + self::SHORT_URL_DOCS, + self::SHORT_URL_CUSTOM_DOMAIN, + self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, + self::SHORT_URL_SHLINK_WITH_TITLE, + ], 'valid_api_key']; yield [['startDate' => Chronos::parse('2018-12-01')->toAtomString()], [ self::SHORT_URL_META, self::SHORT_URL_CUSTOM_SLUG, From 71e91a541fa24ff985857f53227c604391b37c71 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 4 Feb 2021 23:02:26 +0100 Subject: [PATCH 14/18] Allowed to resolve title during short URL edition if it has to --- module/Core/src/Entity/ShortUrl.php | 19 ++++++++----- module/Core/src/Model/ShortUrlEdit.php | 27 ++++++++++++++----- module/Core/src/Service/ShortUrlService.php | 15 +++++++++-- .../Core/test/Service/ShortUrlServiceTest.php | 2 +- 4 files changed, 47 insertions(+), 16 deletions(-) diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 53215a68..810281fa 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -171,24 +171,29 @@ class ShortUrl extends AbstractEntity ShortUrlEdit $shortUrlEdit, ?ShortUrlRelationResolverInterface $relationResolver = null ): void { - if ($shortUrlEdit->hasValidSince()) { + if ($shortUrlEdit->validSinceWasProvided()) { $this->validSince = $shortUrlEdit->validSince(); } - if ($shortUrlEdit->hasValidUntil()) { + if ($shortUrlEdit->validUntilWasProvided()) { $this->validUntil = $shortUrlEdit->validUntil(); } - if ($shortUrlEdit->hasMaxVisits()) { + if ($shortUrlEdit->maxVisitsWasProvided()) { $this->maxVisits = $shortUrlEdit->maxVisits(); } - if ($shortUrlEdit->hasLongUrl()) { - $this->longUrl = $shortUrlEdit->longUrl(); + if ($shortUrlEdit->longUrlWasProvided()) { + $this->longUrl = $shortUrlEdit->longUrl() ?? $this->longUrl; } - if ($shortUrlEdit->hasTags()) { + if ($shortUrlEdit->tagsWereProvided()) { $relationResolver = $relationResolver ?? new SimpleShortUrlRelationResolver(); $this->tags = $relationResolver->resolveTags($shortUrlEdit->tags()); } - if ($shortUrlEdit->hasTitle()) { + if ( + $this->title === null + || $shortUrlEdit->titleWasProvided() + || ($this->titleWasAutoResolved && $shortUrlEdit->titleWasAutoResolved()) + ) { $this->title = $shortUrlEdit->title(); + $this->titleWasAutoResolved = $shortUrlEdit->titleWasAutoResolved(); } } diff --git a/module/Core/src/Model/ShortUrlEdit.php b/module/Core/src/Model/ShortUrlEdit.php index ddb7d317..c2f97eed 100644 --- a/module/Core/src/Model/ShortUrlEdit.php +++ b/module/Core/src/Model/ShortUrlEdit.php @@ -27,6 +27,7 @@ final class ShortUrlEdit private array $tags = []; private bool $titlePropWasProvided = false; private ?string $title = null; + private bool $titleWasAutoResolved = false; private ?bool $validateUrl = null; private function __construct() @@ -74,7 +75,7 @@ final class ShortUrlEdit return $this->longUrl; } - public function hasLongUrl(): bool + public function longUrlWasProvided(): bool { return $this->longUrlPropWasProvided && $this->longUrl !== null; } @@ -84,7 +85,7 @@ final class ShortUrlEdit return $this->validSince; } - public function hasValidSince(): bool + public function validSinceWasProvided(): bool { return $this->validSincePropWasProvided; } @@ -94,7 +95,7 @@ final class ShortUrlEdit return $this->validUntil; } - public function hasValidUntil(): bool + public function validUntilWasProvided(): bool { return $this->validUntilPropWasProvided; } @@ -104,7 +105,7 @@ final class ShortUrlEdit return $this->maxVisits; } - public function hasMaxVisits(): bool + public function maxVisitsWasProvided(): bool { return $this->maxVisitsPropWasProvided; } @@ -117,7 +118,7 @@ final class ShortUrlEdit return $this->tags; } - public function hasTags(): bool + public function tagsWereProvided(): bool { return $this->tagsPropWasProvided; } @@ -127,11 +128,25 @@ final class ShortUrlEdit return $this->title; } - public function hasTitle(): bool + public function titleWasProvided(): bool { return $this->titlePropWasProvided; } + public function titleWasAutoResolved(): bool + { + return $this->titleWasAutoResolved; + } + + public function withResolvedTitle(string $title): self + { + $copy = clone $this; + $copy->title = $title; + $copy->titleWasAutoResolved = true; + + return $copy; + } + public function doValidateUrl(): ?bool { return $this->validateUrl; diff --git a/module/Core/src/Service/ShortUrlService.php b/module/Core/src/Service/ShortUrlService.php index 70606219..e412b63b 100644 --- a/module/Core/src/Service/ShortUrlService.php +++ b/module/Core/src/Service/ShortUrlService.php @@ -61,8 +61,8 @@ class ShortUrlService implements ShortUrlServiceInterface ShortUrlEdit $shortUrlEdit, ?ApiKey $apiKey = null ): ShortUrl { - if ($shortUrlEdit->hasLongUrl()) { - $this->urlValidator->validateUrl($shortUrlEdit->longUrl(), $shortUrlEdit->doValidateUrl()); + if ($shortUrlEdit->longUrlWasProvided()) { + $shortUrlEdit = $this->processTitleAndValidateUrl($shortUrlEdit); } $shortUrl = $this->urlResolver->resolveShortUrl($identifier, $apiKey); @@ -72,4 +72,15 @@ class ShortUrlService implements ShortUrlServiceInterface return $shortUrl; } + + private function processTitleAndValidateUrl(ShortUrlEdit $shortUrlEdit): ShortUrlEdit + { + if ($shortUrlEdit->titleWasProvided()) { + $this->urlValidator->validateUrl($shortUrlEdit->longUrl(), $shortUrlEdit->doValidateUrl()); + return $shortUrlEdit; + } + + $title = $this->urlValidator->validateUrlWithTitle($shortUrlEdit->longUrl(), $shortUrlEdit->doValidateUrl()); + return $title === null ? $shortUrlEdit : $shortUrlEdit->withResolvedTitle($title); + } } diff --git a/module/Core/test/Service/ShortUrlServiceTest.php b/module/Core/test/Service/ShortUrlServiceTest.php index 178561f0..d4920f27 100644 --- a/module/Core/test/Service/ShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrlServiceTest.php @@ -102,7 +102,7 @@ class ShortUrlServiceTest extends TestCase self::assertEquals($shortUrlEdit->longUrl() ?? $originalLongUrl, $shortUrl->getLongUrl()); $findShortUrl->shouldHaveBeenCalled(); $flush->shouldHaveBeenCalled(); - $this->urlValidator->validateUrl( + $this->urlValidator->validateUrlWithTitle( $shortUrlEdit->longUrl(), $shortUrlEdit->doValidateUrl(), )->shouldHaveBeenCalledTimes($expectedValidateCalls); From 608742c2e2f12d7317c2229de96a5ccf64a782cf Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 5 Feb 2021 17:59:34 +0100 Subject: [PATCH 15/18] Added helper service to avoid code duplication when resolving short URLs titles --- module/Core/config/dependencies.config.php | 6 ++- module/Core/src/Model/ShortUrlEdit.php | 13 ++++- module/Core/src/Model/ShortUrlMeta.php | 3 +- module/Core/src/Service/ShortUrlService.php | 22 +++------ module/Core/src/Service/UrlShortener.php | 22 +++------ .../Helper/ShortUrlTitleResolutionHelper.php | 28 +++++++++++ ...ShortUrlTitleResolutionHelperInterface.php | 15 ++++++ .../Helper/TitleResolutionModelInterface.php | 16 ++++++ .../Core/test/Service/ShortUrlServiceTest.php | 17 ++++--- module/Core/test/Service/UrlShortenerTest.php | 49 +++++-------------- .../ShortUrlTitleResolutionHelperTest.php | 49 +++++++++++++++++++ 11 files changed, 159 insertions(+), 81 deletions(-) create mode 100644 module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php create mode 100644 module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php create mode 100644 module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php create mode 100644 module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 48166aeb..e742ad43 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -44,6 +44,7 @@ return [ ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ConfigAbstractFactory::class, ShortUrl\Helper\ShortUrlStringifier::class => ConfigAbstractFactory::class, + ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => ConfigAbstractFactory::class, ShortUrl\Transformer\ShortUrlDataTransformer::class => ConfigAbstractFactory::class, Mercure\MercureUpdatesGenerator::class => ConfigAbstractFactory::class, @@ -69,7 +70,7 @@ return [ Options\UrlShortenerOptions::class => ['config.url_shortener'], Service\UrlShortener::class => [ - Util\UrlValidator::class, + ShortUrl\Helper\ShortUrlTitleResolutionHelper::class, 'em', ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class, Service\ShortUrl\ShortCodeHelper::class, @@ -82,7 +83,7 @@ return [ Service\ShortUrlService::class => [ 'em', Service\ShortUrl\ShortUrlResolver::class, - Util\UrlValidator::class, + ShortUrl\Helper\ShortUrlTitleResolutionHelper::class, ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class, ], Visit\VisitLocator::class => ['em'], @@ -122,6 +123,7 @@ return [ ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ['em'], ShortUrl\Helper\ShortUrlStringifier::class => ['config.url_shortener.domain', 'config.router.base_path'], + ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => [Util\UrlValidator::class], ShortUrl\Transformer\ShortUrlDataTransformer::class => [ShortUrl\Helper\ShortUrlStringifier::class], Mercure\MercureUpdatesGenerator::class => [ShortUrl\Transformer\ShortUrlDataTransformer::class], diff --git a/module/Core/src/Model/ShortUrlEdit.php b/module/Core/src/Model/ShortUrlEdit.php index c2f97eed..3327aad4 100644 --- a/module/Core/src/Model/ShortUrlEdit.php +++ b/module/Core/src/Model/ShortUrlEdit.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Model; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\ShortUrl\Helper\TitleResolutionModelInterface; use Shlinkio\Shlink\Core\Validation\ShortUrlInputFilter; use function array_key_exists; @@ -13,7 +14,7 @@ use function Shlinkio\Shlink\Core\getOptionalBoolFromInputFilter; use function Shlinkio\Shlink\Core\getOptionalIntFromInputFilter; use function Shlinkio\Shlink\Core\parseDateField; -final class ShortUrlEdit +final class ShortUrlEdit implements TitleResolutionModelInterface { private bool $longUrlPropWasProvided = false; private ?string $longUrl = null; @@ -75,6 +76,11 @@ final class ShortUrlEdit return $this->longUrl; } + public function getLongUrl(): string + { + return $this->longUrl() ?? ''; + } + public function longUrlWasProvided(): bool { return $this->longUrlPropWasProvided && $this->longUrl !== null; @@ -133,6 +139,11 @@ final class ShortUrlEdit return $this->titlePropWasProvided; } + public function hasTitle(): bool + { + return $this->titleWasProvided(); + } + public function titleWasAutoResolved(): bool { return $this->titleWasAutoResolved; diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index a741f78c..df25735c 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Model; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\ShortUrl\Helper\TitleResolutionModelInterface; use Shlinkio\Shlink\Core\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -15,7 +16,7 @@ use function Shlinkio\Shlink\Core\parseDateField; use const Shlinkio\Shlink\Core\DEFAULT_SHORT_CODES_LENGTH; -final class ShortUrlMeta +final class ShortUrlMeta implements TitleResolutionModelInterface { private string $longUrl; private ?Chronos $validSince = null; diff --git a/module/Core/src/Service/ShortUrlService.php b/module/Core/src/Service/ShortUrlService.php index e412b63b..dcb1d8cc 100644 --- a/module/Core/src/Service/ShortUrlService.php +++ b/module/Core/src/Service/ShortUrlService.php @@ -15,26 +15,26 @@ use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Paginator\Adapter\ShortUrlRepositoryAdapter; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; +use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlTitleResolutionHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; -use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; class ShortUrlService implements ShortUrlServiceInterface { private ORM\EntityManagerInterface $em; private ShortUrlResolverInterface $urlResolver; - private UrlValidatorInterface $urlValidator; + private ShortUrlTitleResolutionHelperInterface $titleResolutionHelper; private ShortUrlRelationResolverInterface $relationResolver; public function __construct( ORM\EntityManagerInterface $em, ShortUrlResolverInterface $urlResolver, - UrlValidatorInterface $urlValidator, + ShortUrlTitleResolutionHelperInterface $titleResolutionHelper, ShortUrlRelationResolverInterface $relationResolver ) { $this->em = $em; $this->urlResolver = $urlResolver; - $this->urlValidator = $urlValidator; + $this->titleResolutionHelper = $titleResolutionHelper; $this->relationResolver = $relationResolver; } @@ -62,7 +62,8 @@ class ShortUrlService implements ShortUrlServiceInterface ?ApiKey $apiKey = null ): ShortUrl { if ($shortUrlEdit->longUrlWasProvided()) { - $shortUrlEdit = $this->processTitleAndValidateUrl($shortUrlEdit); + /** @var ShortUrlEdit $shortUrlEdit */ + $shortUrlEdit = $this->titleResolutionHelper->processTitleAndValidateUrl($shortUrlEdit); } $shortUrl = $this->urlResolver->resolveShortUrl($identifier, $apiKey); @@ -72,15 +73,4 @@ class ShortUrlService implements ShortUrlServiceInterface return $shortUrl; } - - private function processTitleAndValidateUrl(ShortUrlEdit $shortUrlEdit): ShortUrlEdit - { - if ($shortUrlEdit->titleWasProvided()) { - $this->urlValidator->validateUrl($shortUrlEdit->longUrl(), $shortUrlEdit->doValidateUrl()); - return $shortUrlEdit; - } - - $title = $this->urlValidator->validateUrlWithTitle($shortUrlEdit->longUrl(), $shortUrlEdit->doValidateUrl()); - return $title === null ? $shortUrlEdit : $shortUrlEdit->withResolvedTitle($title); - } } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index 6083e1f4..78064259 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -11,23 +11,23 @@ use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; +use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlTitleResolutionHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; -use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; class UrlShortener implements UrlShortenerInterface { private EntityManagerInterface $em; - private UrlValidatorInterface $urlValidator; + private ShortUrlTitleResolutionHelperInterface $titleResolutionHelper; private ShortUrlRelationResolverInterface $relationResolver; private ShortCodeHelperInterface $shortCodeHelper; public function __construct( - UrlValidatorInterface $urlValidator, + ShortUrlTitleResolutionHelperInterface $titleResolutionHelper, EntityManagerInterface $em, ShortUrlRelationResolverInterface $relationResolver, ShortCodeHelperInterface $shortCodeHelper ) { - $this->urlValidator = $urlValidator; + $this->titleResolutionHelper = $titleResolutionHelper; $this->em = $em; $this->relationResolver = $relationResolver; $this->shortCodeHelper = $shortCodeHelper; @@ -45,7 +45,8 @@ class UrlShortener implements UrlShortenerInterface return $existingShortUrl; } - $meta = $this->processTitleAndValidateUrl($meta); + /** @var ShortUrlMeta $meta */ + $meta = $this->titleResolutionHelper->processTitleAndValidateUrl($meta); return $this->em->transactional(function () use ($meta) { $shortUrl = ShortUrl::fromMeta($meta, $this->relationResolver); @@ -82,15 +83,4 @@ class UrlShortener implements UrlShortenerInterface throw NonUniqueSlugException::fromSlug($shortUrlToBeCreated->getShortCode(), $domainAuthority); } } - - private function processTitleAndValidateUrl(ShortUrlMeta $meta): ShortUrlMeta - { - if ($meta->hasTitle()) { - $this->urlValidator->validateUrl($meta->getLongUrl(), $meta->doValidateUrl()); - return $meta; - } - - $title = $this->urlValidator->validateUrlWithTitle($meta->getLongUrl(), $meta->doValidateUrl()); - return $title === null ? $meta : $meta->withResolvedTitle($title); - } } diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php new file mode 100644 index 00000000..4615e45f --- /dev/null +++ b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php @@ -0,0 +1,28 @@ +urlValidator = $urlValidator; + } + + public function processTitleAndValidateUrl(TitleResolutionModelInterface $data): TitleResolutionModelInterface + { + if ($data->hasTitle()) { + $this->urlValidator->validateUrl($data->getLongUrl(), $data->doValidateUrl()); + return $data; + } + + $title = $this->urlValidator->validateUrlWithTitle($data->getLongUrl(), $data->doValidateUrl()); + return $title === null ? $data : $data->withResolvedTitle($title); + } +} diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php new file mode 100644 index 00000000..50022746 --- /dev/null +++ b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php @@ -0,0 +1,15 @@ +em->flush()->willReturn(null); $this->urlResolver = $this->prophesize(ShortUrlResolverInterface::class); - $this->urlValidator = $this->prophesize(UrlValidatorInterface::class); + $this->titleResolutionHelper = $this->prophesize(ShortUrlTitleResolutionHelperInterface::class); $this->service = new ShortUrlService( $this->em->reveal(), $this->urlResolver->reveal(), - $this->urlValidator->reveal(), + $this->titleResolutionHelper->reveal(), new SimpleShortUrlRelationResolver(), ); } @@ -93,6 +93,10 @@ class ShortUrlServiceTest extends TestCase )->willReturn($shortUrl); $flush = $this->em->flush()->willReturn(null); + $processTitle = $this->titleResolutionHelper->processTitleAndValidateUrl($shortUrlEdit)->willReturn( + $shortUrlEdit, + ); + $result = $this->service->updateShortUrl(new ShortUrlIdentifier('abc123'), $shortUrlEdit, $apiKey); self::assertSame($shortUrl, $result); @@ -102,10 +106,7 @@ class ShortUrlServiceTest extends TestCase self::assertEquals($shortUrlEdit->longUrl() ?? $originalLongUrl, $shortUrl->getLongUrl()); $findShortUrl->shouldHaveBeenCalled(); $flush->shouldHaveBeenCalled(); - $this->urlValidator->validateUrlWithTitle( - $shortUrlEdit->longUrl(), - $shortUrlEdit->doValidateUrl(), - )->shouldHaveBeenCalledTimes($expectedValidateCalls); + $processTitle->shouldHaveBeenCalledTimes($expectedValidateCalls); } public function provideShortUrlEdits(): iterable diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 9fbf82ed..7e319314 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -16,8 +16,8 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\Service\UrlShortener; +use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlTitleResolutionHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\SimpleShortUrlRelationResolver; -use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; class UrlShortenerTest extends TestCase { @@ -25,12 +25,13 @@ class UrlShortenerTest extends TestCase private UrlShortener $urlShortener; private ObjectProphecy $em; - private ObjectProphecy $urlValidator; + private ObjectProphecy $titleResolutionHelper; private ObjectProphecy $shortCodeHelper; public function setUp(): void { - $this->urlValidator = $this->prophesize(UrlValidatorInterface::class); + $this->titleResolutionHelper = $this->prophesize(ShortUrlTitleResolutionHelperInterface::class); + $this->titleResolutionHelper->processTitleAndValidateUrl(Argument::cetera())->willReturnArgument(); $this->em = $this->prophesize(EntityManagerInterface::class); $this->em->persist(Argument::any())->will(function ($arguments): void { @@ -52,47 +53,22 @@ class UrlShortenerTest extends TestCase $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true); $this->urlShortener = new UrlShortener( - $this->urlValidator->reveal(), + $this->titleResolutionHelper->reveal(), $this->em->reveal(), new SimpleShortUrlRelationResolver(), $this->shortCodeHelper->reveal(), ); } - /** - * @test - * @dataProvider provideTitles - */ - public function urlIsProperlyShortened(?string $title, int $validateWithTitleCallsNum, int $validateCallsNum): void + /** @test */ + public function urlIsProperlyShortened(): void { $longUrl = 'http://foobar.com/12345/hello?foo=bar'; - $shortUrl = $this->urlShortener->shorten(ShortUrlMeta::fromRawData(['longUrl' => $longUrl, 'title' => $title])); + $meta = ShortUrlMeta::fromRawData(['longUrl' => $longUrl]); + $shortUrl = $this->urlShortener->shorten($meta); - self::assertEquals('http://foobar.com/12345/hello?foo=bar', $shortUrl->getLongUrl()); - $this->urlValidator->validateUrlWithTitle($longUrl, null)->shouldHaveBeenCalledTimes( - $validateWithTitleCallsNum, - ); - $this->urlValidator->validateUrl($longUrl, null)->shouldHaveBeenCalledTimes($validateCallsNum); - } - - /** - * @test - * @dataProvider provideTitles - */ - public function urlIsProperlyShortenedWithExpectedResolvedTitle(?string $title): void - { - $validateWithTitle = $this->urlValidator->validateUrlWithTitle(Argument::cetera())->willReturn($title); - - $shortUrl = $this->urlShortener->shorten(ShortUrlMeta::fromRawData(['longUrl' => 'foo'])); - - self::assertEquals($title, $shortUrl->getTitle()); - $validateWithTitle->shouldHaveBeenCalledOnce(); - } - - public function provideTitles(): iterable - { - yield 'no title' => [null, 1, 0]; - yield 'title' => ['link title', 0, 1]; + self::assertEquals($longUrl, $shortUrl->getLongUrl()); + $this->titleResolutionHelper->processTitleAndValidateUrl($meta)->shouldHaveBeenCalledOnce(); } /** @test */ @@ -123,8 +99,7 @@ class UrlShortenerTest extends TestCase $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); - $this->urlValidator->validateUrl(Argument::cetera())->shouldNotHaveBeenCalled(); - $this->urlValidator->validateUrlWithTitle(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->titleResolutionHelper->processTitleAndValidateUrl(Argument::cetera())->shouldNotHaveBeenCalled(); self::assertSame($expected, $result); } diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php new file mode 100644 index 00000000..6783303c --- /dev/null +++ b/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php @@ -0,0 +1,49 @@ +urlValidator = $this->prophesize(UrlValidatorInterface::class); + $this->helper = new ShortUrlTitleResolutionHelper($this->urlValidator->reveal()); + } + + /** + * @test + * @dataProvider provideTitles + */ + public function urlIsProperlyShortened(?string $title, int $validateWithTitleCallsNum, int $validateCallsNum): void + { + $longUrl = 'http://foobar.com/12345/hello?foo=bar'; + $this->helper->processTitleAndValidateUrl( + ShortUrlMeta::fromRawData(['longUrl' => $longUrl, 'title' => $title]), + ); + + $this->urlValidator->validateUrlWithTitle($longUrl, null)->shouldHaveBeenCalledTimes( + $validateWithTitleCallsNum, + ); + $this->urlValidator->validateUrl($longUrl, null)->shouldHaveBeenCalledTimes($validateCallsNum); + } + + public function provideTitles(): iterable + { + yield 'no title' => [null, 1, 0]; + yield 'title' => ['link title', 0, 1]; + } +} From d386e1405c8d9a6d5593bb22ed674964c660f4f6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 5 Feb 2021 18:22:54 +0100 Subject: [PATCH 16/18] Ensure request is not performed if both title resolution and URL validation are disabled --- module/Core/src/Util/UrlValidator.php | 7 +++++-- module/Core/test/Util/UrlValidatorTest.php | 5 +++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php index 8d05cbe6..62c2bea5 100644 --- a/module/Core/src/Util/UrlValidator.php +++ b/module/Core/src/Util/UrlValidator.php @@ -47,9 +47,12 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface public function validateUrlWithTitle(string $url, ?bool $doValidate): ?string { $doValidate = $doValidate ?? $this->options->isUrlValidationEnabled(); - $response = $this->validateUrlAndGetResponse($url, $doValidate); + if (! $doValidate && ! $this->options->autoResolveTitles()) { + return null; + } - if ($response === null || ! $this->options->autoResolveTitles()) { + $response = $this->validateUrlAndGetResponse($url, $doValidate); + if ($response === null) { return null; } diff --git a/module/Core/test/Util/UrlValidatorTest.php b/module/Core/test/Util/UrlValidatorTest.php index 7c5f7c55..9ef8e94e 100644 --- a/module/Core/test/Util/UrlValidatorTest.php +++ b/module/Core/test/Util/UrlValidatorTest.php @@ -87,6 +87,7 @@ class UrlValidatorTest extends TestCase ): void { $request = $this->httpClient->request(Argument::cetera())->willThrow(ClientException::class); $this->options->validateUrl = $validateUrl; + $this->options->autoResolveTitles = true; $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', $doValidate); @@ -107,10 +108,10 @@ class UrlValidatorTest extends TestCase $request = $this->httpClient->request(Argument::cetera())->willReturn($this->respWithTitle()); $this->options->autoResolveTitles = false; - $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); + $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', false); self::assertNull($result); - $request->shouldHaveBeenCalledOnce(); + $request->shouldNotHaveBeenCalled(); } /** @test */ From bc632fd644ffb13e3a667494ee8900ce0acac4b9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 5 Feb 2021 18:26:22 +0100 Subject: [PATCH 17/18] Updated changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6079f750..87f8b9d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ## [Unreleased] ### Added * [#856](https://github.com/shlinkio/shlink/issues/856) Added PHP 8.0 support. +* [#941](https://github.com/shlinkio/shlink/issues/856) Added support to provide a title for every short URL. + + The title can also be automatically resolved from the long URL, when no title was explicitly provided, but this option needs to be opted in. ### Changed * [#977](https://github.com/shlinkio/shlink/issues/977) Migrated from `laminas/laminas-paginator` to `pagerfanta/core` to handle pagination. From de4e677f18a0a6bb804b332607cda709ae66bfc8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 5 Feb 2021 18:33:36 +0100 Subject: [PATCH 18/18] Fixed database started for API tests in GitHub workflow --- .github/workflows/ci.yml | 2 +- config/autoload/url-shortener.global.php | 2 +- docker/config/shlink_in_docker.local.php | 2 +- module/Core/src/Options/UrlShortenerOptions.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b591afbd..0674cd2b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -189,7 +189,7 @@ jobs: - name: Checkout code uses: actions/checkout@v2 - name: Start database server - run: docker-compose -f docker-compose.yml -f docker-compose.ci.yml up -d shlink_db + run: docker-compose -f docker-compose.yml -f docker-compose.ci.yml up -d shlink_db_postgres - name: Use PHP uses: shivammathur/setup-php@v2 with: diff --git a/config/autoload/url-shortener.global.php b/config/autoload/url-shortener.global.php index f4a7966a..015d459e 100644 --- a/config/autoload/url-shortener.global.php +++ b/config/autoload/url-shortener.global.php @@ -19,7 +19,7 @@ return [ 'default_short_codes_length' => DEFAULT_SHORT_CODES_LENGTH, 'redirect_status_code' => DEFAULT_REDIRECT_STATUS_CODE, 'redirect_cache_lifetime' => DEFAULT_REDIRECT_CACHE_LIFETIME, - 'auto_resolve_titles' => false, // Deprecated value. Default to true with Shlink 3.0.0 + 'auto_resolve_titles' => false, ], ]; diff --git a/docker/config/shlink_in_docker.local.php b/docker/config/shlink_in_docker.local.php index 070f0974..40173d69 100644 --- a/docker/config/shlink_in_docker.local.php +++ b/docker/config/shlink_in_docker.local.php @@ -125,7 +125,7 @@ return [ 'default_short_codes_length' => $helper->getDefaultShortCodesLength(), 'redirect_status_code' => (int) env('REDIRECT_STATUS_CODE', DEFAULT_REDIRECT_STATUS_CODE), 'redirect_cache_lifetime' => (int) env('REDIRECT_CACHE_LIFETIME', DEFAULT_REDIRECT_CACHE_LIFETIME), - 'auto_resolve_titles' => (bool) env('AUTO_RESOLVE_TITLES', false), // Deprecated value. Default to true + 'auto_resolve_titles' => (bool) env('AUTO_RESOLVE_TITLES', false), ], 'not_found_redirects' => $helper->getNotFoundRedirectsConfig(), diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php index 553a160f..ebedbf97 100644 --- a/module/Core/src/Options/UrlShortenerOptions.php +++ b/module/Core/src/Options/UrlShortenerOptions.php @@ -18,7 +18,7 @@ class UrlShortenerOptions extends AbstractOptions private bool $validateUrl = true; private int $redirectStatusCode = DEFAULT_REDIRECT_STATUS_CODE; private int $redirectCacheLifetime = DEFAULT_REDIRECT_CACHE_LIFETIME; - private bool $autoResolveTitles = false; // Deprecated value. Default to true with Shlink 3.0.0 + private bool $autoResolveTitles = false; public function isUrlValidationEnabled(): bool {