Merge pull request #618 from shlinkio/develop

Release v2.0.2

CHANGELOG.md

@@ -4,6 +4,55 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org).
 
+## 2.0.2 - 2020-01-12
+
+#### Added
+
+* *Nothing*
+
+#### Changed
+
+* *Nothing*
+
+#### Deprecated
+
+* *Nothing*
+
+#### Removed
+
+* *Nothing*
+
+#### Fixed
+
+* [#614](https://github.com/shlinkio/shlink/issues/614) Fixed `OPTIONS` requests including the `Origin` header not always returning an empty body with status 2xx.
+* [#615](https://github.com/shlinkio/shlink/issues/615) Fixed query args with no value being lost from the long URL when users are redirected.
+
+
+## 2.0.1 - 2020-01-10
+
+#### Added
+
+* *Nothing*
+
+#### Changed
+
+* *Nothing*
+
+#### Deprecated
+
+* *Nothing*
+
+#### Removed
+
+* *Nothing*
+
+#### Fixed
+
+* [#607](https://github.com/shlinkio/shlink/issues/607) Added missing info on UPGRADE.md doc.
+* [#610](https://github.com/shlinkio/shlink/issues/610) Fixed use of hardcoded quotes on a database migration which makes it fail on postgres.
+* [#605](https://github.com/shlinkio/shlink/issues/605) Fixed crashes occurring when migrating from old Shlink versions with nullable DB columns that are assigned to non-nullable entity typed props.
+
+
 ## 2.0.0 - 2020-01-08
 
 #### Added

UPGRADE.md

@@ -2,6 +2,14 @@
 
 ## From v1.x to v2.x
 
+### PHP 7.4 required
+
+This new version takes advantage of several new features introduced in PHP 7.4.
+
+Thanks to that, the code is more reliable and robust, and easier to maintain and improve.
+
+However, that means that any previous PHP version is no longer supported.
+
 ### Preview generation
 
 The ability to generate website previews has been completely removed and has no replacement.
@@ -43,6 +51,16 @@ Endpoints need to provide a version in the path now. Previously, not providing a
 
 The only exception is the `/rest/health` endpoint, which will continue working without the version.
 
+### API errors
+
+Shlink v1.21.0 introduced support for API errors using the Problem Details format, as well as the v2 of the API.
+
+For backwards compatibility reasons, requests performed to v1 continued to return the old `error` and `message` properties.
+
+Starting with Shlink v2.0.0, both versions of the API will no longer return those two properties.
+
+As a replacement, use `type` instead of `error`, and `detail` instead of `message`.
+
 ### Changes in models
 
 The next REST API models have changed:

composer.json

@@ -97,7 +97,7 @@
         ],
         "cs": "phpcs",
         "cs:fix": "phpcbf",
-        "stan": "phpstan analyse module/*/src/ module/*/config config docker/config --level=5 -c phpstan.neon",
+        "stan": "phpstan analyse module/*/src/ module/*/config config docker/config --level=6",
         "test": [
             "@test:unit",
             "@test:db",

config/autoload/common.local.php.dist

@@ -1,4 +1,5 @@
 <?php
+
 declare(strict_types=1);
 
 use Laminas\ConfigAggregator\ConfigAggregator;

config/autoload/router.local.php.dist

@@ -1,9 +1,13 @@
 <?php
+
+declare(strict_types=1);
+
 use Mezzio\Router\FastRouteRouter;
 
 return [
 
     'router' => [
+//        'base_path' => '',
         'fastroute' => [
             FastRouteRouter::CONFIG_CACHE_ENABLED => false,
         ],

config/autoload/swoole.local.php.dist

@@ -1,8 +1,9 @@
 <?php
+
 declare(strict_types=1);
 
-use Mezzio\Swoole\HotCodeReload\FileWatcher\InotifyFileWatcher;
 use Laminas\ServiceManager\Factory\InvokableFactory;
+use Mezzio\Swoole\HotCodeReload\FileWatcher\InotifyFileWatcher;
 
 return [
 

data/migrations/Version20200105165647.php

@@ -9,18 +9,35 @@ use Doctrine\DBAL\Schema\Schema;
 use Doctrine\DBAL\Types\Types;
 use Doctrine\Migrations\AbstractMigration;
 
+use function Functional\some;
+
 final class Version20200105165647 extends AbstractMigration
 {
     private const COLUMNS = ['lat' => 'latitude', 'lon' => 'longitude'];
 
+    /**
+     * @throws DBALException
+     */
     public function preUp(Schema $schema): void
     {
+        $visitLocations = $schema->getTable('visit_locations');
+        $this->skipIf(some(
+            self::COLUMNS,
+            fn (string $v, string $newColName) => $visitLocations->hasColumn($newColName),
+        ), 'New columns already exist');
+
         foreach (self::COLUMNS as $columnName) {
             $qb = $this->connection->createQueryBuilder();
             $qb->update('visit_locations')
-               ->set($columnName, '"0"')
-               ->where($columnName . '=""')
-               ->orWhere($columnName . ' IS NULL')
+               ->set($columnName, ':zeroValue')
+               ->where($qb->expr()->orX(
+                   $qb->expr()->eq($columnName, ':emptyString'),
+                   $qb->expr()->isNull($columnName),
+               ))
+               ->setParameters([
+                   'zeroValue' => '0',
+                   'emptyString' => '',
+               ])
                ->execute();
         }
     }
@@ -33,16 +50,24 @@ final class Version20200105165647 extends AbstractMigration
         $visitLocations = $schema->getTable('visit_locations');
 
         foreach (self::COLUMNS as $newName => $oldName) {
-            $visitLocations->addColumn($newName, Types::FLOAT);
+            $visitLocations->addColumn($newName, Types::FLOAT, [
+                'default' => '0.0',
+            ]);
         }
     }
 
+    /**
+     * @throws DBALException
+     */
     public function postUp(Schema $schema): void
     {
+        $platformName = $this->connection->getDatabasePlatform()->getName();
+        $castType = $platformName === 'postgres' ? 'DOUBLE PRECISION' : 'DECIMAL(9,2)';
+
         foreach (self::COLUMNS as $newName => $oldName) {
             $qb = $this->connection->createQueryBuilder();
             $qb->update('visit_locations')
-               ->set($newName, $oldName)
+               ->set($newName, 'CAST(' . $oldName . ' AS ' . $castType . ')')
                ->execute();
         }
     }

data/migrations/Version20200106215144.php

@@ -9,6 +9,8 @@ use Doctrine\DBAL\Schema\Schema;
 use Doctrine\DBAL\Types\Types;
 use Doctrine\Migrations\AbstractMigration;
 
+use function Functional\none;
+
 final class Version20200106215144 extends AbstractMigration
 {
     private const COLUMNS = ['latitude', 'longitude'];
@@ -19,6 +21,10 @@ final class Version20200106215144 extends AbstractMigration
     public function up(Schema $schema): void
     {
         $visitLocations = $schema->getTable('visit_locations');
+        $this->skipIf(none(
+            self::COLUMNS,
+            fn (string $oldColName) => $visitLocations->hasColumn($oldColName),
+        ), 'Old columns do not exist');
 
         foreach (self::COLUMNS as $colName) {
             $visitLocations->dropColumn($colName);

data/migrations/Version20200110182849.php

@@ -0,0 +1,53 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkMigrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+use function Functional\each;
+use function Functional\partial_left;
+
+final class Version20200110182849 extends AbstractMigration
+{
+    private const DEFAULT_EMPTY_VALUE = '';
+    private const COLUMN_DEFAULTS_MAP = [
+        'visits' => [
+            'referer',
+            'user_agent',
+        ],
+        'visit_locations' => [
+            'timezone',
+            'country_code',
+            'country_name',
+            'region_name',
+            'city_name',
+        ],
+    ];
+
+    public function up(Schema $schema): void
+    {
+        each(
+            self::COLUMN_DEFAULTS_MAP,
+            fn (array $columns, string $tableName) =>
+                each($columns, partial_left([$this, 'setDefaultValueForColumnInTable'], $tableName)),
+        );
+    }
+
+    public function setDefaultValueForColumnInTable(string $tableName, string $columnName): void
+    {
+        $qb = $this->connection->createQueryBuilder();
+        $qb->update($tableName)
+           ->set($columnName, ':emptyValue')
+           ->setParameter('emptyValue', self::DEFAULT_EMPTY_VALUE)
+           ->where($qb->expr()->isNull($columnName))
+           ->execute();
+    }
+
+    public function down(Schema $schema): void
+    {
+        // No need (and no way) to undo this migration
+    }
+}

module/Core/src/Action/AbstractTrackingAction.php

@@ -20,8 +20,8 @@ use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface;
 
 use function array_key_exists;
 use function array_merge;
+use function GuzzleHttp\Psr7\build_query;
 use function GuzzleHttp\Psr7\parse_query;
-use function http_build_query;
 
 abstract class AbstractTrackingAction implements MiddlewareInterface
 {
@@ -42,12 +42,6 @@ abstract class AbstractTrackingAction implements MiddlewareInterface
         $this->logger = $logger ?: new NullLogger();
     }
 
-    /**
-     * Process an incoming server request and return a response, optionally delegating
-     * to the next middleware component to create the response.
-     *
-     *
-     */
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
         $shortCode = $request->getAttribute('shortCode', '');
@@ -79,7 +73,7 @@ abstract class AbstractTrackingAction implements MiddlewareInterface
         }
         $mergedQuery = array_merge($hardcodedQuery, $currentQuery);
 
-        return (string) $uri->withQuery(http_build_query($mergedQuery));
+        return (string) $uri->withQuery(build_query($mergedQuery));
     }
 
     abstract protected function createSuccessResp(string $longUrl): ResponseInterface;

module/Core/src/Entity/ShortUrl.php

@@ -29,9 +29,9 @@ class ShortUrl extends AbstractEntity
     private Collection $visits;
     /** @var Collection|Tag[] */
     private Collection $tags;
-    private ?Chronos $validSince;
-    private ?Chronos $validUntil;
-    private ?int $maxVisits;
+    private ?Chronos $validSince = null;
+    private ?Chronos $validUntil = null;
+    private ?int $maxVisits = null;
     private ?Domain $domain;
     private bool $customSlugWasProvided;
 

module/Core/src/Entity/Visit.php

@@ -15,10 +15,10 @@ use Shlinkio\Shlink\Core\Visit\Model\VisitLocationInterface;
 
 class Visit extends AbstractEntity implements JsonSerializable
 {
-    private string $referer = '';
+    private string $referer;
     private Chronos $date;
     private ?string $remoteAddr = null;
-    private string $userAgent = '';
+    private string $userAgent;
     private ShortUrl $shortUrl;
     private ?VisitLocation $visitLocation = null;
 

module/Core/test/Action/RedirectActionTest.php

@@ -64,6 +64,7 @@ class RedirectActionTest extends TestCase
     {
         yield ['http://domain.com/foo/bar?some=thing', []];
         yield ['http://domain.com/foo/bar?some=thing', ['foobar' => 'notrack']];
+        yield ['http://domain.com/foo/bar?some=thing&else', ['else' => null]];
         yield ['http://domain.com/foo/bar?some=thing&foo=bar', ['foo' => 'bar']];
         yield ['http://domain.com/foo/bar?some=overwritten&foo=bar', ['foo' => 'bar', 'some' => 'overwritten']];
         yield ['http://domain.com/foo/bar?some=overwritten', ['foobar' => 'notrack', 'some' => 'overwritten']];

module/Rest/src/Middleware/CrossDomainMiddleware.php

@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\Rest\Middleware;
 
 use Fig\Http\Message\RequestMethodInterface;
+use Laminas\Diactoros\Response\EmptyResponse;
 use Mezzio\Router\RouteResult;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
@@ -12,6 +13,7 @@ use Psr\Http\Server\MiddlewareInterface;
 use Psr\Http\Server\RequestHandlerInterface;
 use Shlinkio\Shlink\Rest\Authentication;
 
+use function array_merge;
 use function implode;
 
 class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterface
@@ -53,10 +55,7 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa
             'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'),
         ];
 
-        foreach ($corsHeaders as $key => $value) {
-            $response = $response->withHeader($key, $value);
-        }
-
-        return $response;
+        // Options requests should always be empty and have a 204 status code
+        return EmptyResponse::withHeaders(array_merge($response->getHeaders(), $corsHeaders));
     }
 }

module/Rest/test/Middleware/CrossDomainMiddlewareTest.php

@@ -31,14 +31,14 @@ class CrossDomainMiddlewareTest extends TestCase
     /** @test */
     public function nonCrossDomainRequestsAreNotAffected(): void
     {
-        $originalResponse = new Response();
+        $originalResponse = (new Response())->withStatus(404);
         $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce();
 
         $response = $this->middleware->process(new ServerRequest(), $this->handler->reveal());
-        $this->assertSame($originalResponse, $response);
-
         $headers = $response->getHeaders();
 
+        $this->assertSame($originalResponse, $response);
+        $this->assertEquals(404, $response->getStatusCode());
         $this->assertArrayNotHasKey('Access-Control-Allow-Origin', $headers);
         $this->assertArrayNotHasKey('Access-Control-Expose-Headers', $headers);
         $this->assertArrayNotHasKey('Access-Control-Allow-Methods', $headers);
@@ -93,6 +93,7 @@ class CrossDomainMiddlewareTest extends TestCase
         $this->assertArrayHasKey('Access-Control-Allow-Methods', $headers);
         $this->assertEquals('1000', $response->getHeaderLine('Access-Control-Max-Age'));
         $this->assertEquals('foo, bar, baz', $response->getHeaderLine('Access-Control-Allow-Headers'));
+        $this->assertEquals(204, $response->getStatusCode());
     }
 
     /**
@@ -112,6 +113,7 @@ class CrossDomainMiddlewareTest extends TestCase
         $response = $this->middleware->process($request, $this->handler->reveal());
 
         $this->assertEquals($response->getHeaderLine('Access-Control-Allow-Methods'), $expectedAllowedMethods);
+        $this->assertEquals(204, $response->getStatusCode());
     }
 
     public function provideRouteResults(): iterable
@@ -126,4 +128,42 @@ class CrossDomainMiddlewareTest extends TestCase
             'DELETE,PATCH,PUT',
         ];
     }
+
+    /**
+     * @test
+     * @dataProvider provideMethods
+     */
+    public function expectedStatusCodeIsReturnDependingOnRequestMethod(
+        string $method,
+        int $status,
+        int $expectedStatus
+    ): void {
+        $originalResponse = (new Response())->withStatus($status);
+        $request = (new ServerRequest())->withMethod($method)
+                                        ->withHeader('Origin', 'local');
+        $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce();
+
+        $response = $this->middleware->process($request, $this->handler->reveal());
+
+        $this->assertEquals($expectedStatus, $response->getStatusCode());
+    }
+
+    public function provideMethods(): iterable
+    {
+        yield 'POST 200' => ['POST', 200, 200];
+        yield 'POST 400' => ['POST', 400, 400];
+        yield 'POST 500' => ['POST', 500, 500];
+        yield 'GET 200' => ['GET', 200, 200];
+        yield 'GET 400' => ['GET', 400, 400];
+        yield 'GET 500' => ['GET', 500, 500];
+        yield 'PATCH 200' => ['PATCH', 200, 200];
+        yield 'PATCH 400' => ['PATCH', 400, 400];
+        yield 'PATCH 500' => ['PATCH', 500, 500];
+        yield 'DELETE 200' => ['DELETE', 200, 200];
+        yield 'DELETE 400' => ['DELETE', 400, 400];
+        yield 'DELETE 500' => ['DELETE', 500, 500];
+        yield 'OPTIONS 200' => ['OPTIONS', 200, 204];
+        yield 'OPTIONS 400' => ['OPTIONS', 400, 204];
+        yield 'OPTIONS 500' => ['OPTIONS', 500, 204];
+    }
 }

phpstan.neon

@@ -1,4 +1,6 @@
 parameters:
+	checkMissingIterableValueType: false
+	checkGenericClassInNonGenericObjectType: false
 	ignoreErrors:
 		- '#Undefined variable: \$metadata#'
 		- '#AbstractQuery::setParameters()#'