Merge pull request #437 from acelaya/feature/decorate-em

Feature/decorate em

CHANGELOG.md

@@ -57,7 +57,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 
 * [#416](https://github.com/shlinkio/shlink/issues/416) Fixed error thrown when trying to locate visits after the GeoLite2 DB is downloaded for the first time.
 * [#424](https://github.com/shlinkio/shlink/issues/424) Updated wkhtmltoimage to version 0.12.5
-* [#427](https://github.com/shlinkio/shlink/issues/427) Fixed shlink being unusable after a database error on swoole contexts.
+* [#427](https://github.com/shlinkio/shlink/issues/427) and [#434](https://github.com/shlinkio/shlink/issues/434) Fixed shlink being unusable after a database error on swoole contexts.
 
 
 ## 1.17.0 - 2019-05-13

config/autoload/entity-manager.global.php

@@ -1,7 +1,7 @@
 <?php
 declare(strict_types=1);
 
-use function Shlinkio\Shlink\Common\env;
+namespace Shlinkio\Shlink\Common;
 
 return [
 

module/Common/config/dependencies.config.php

@@ -4,7 +4,6 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\Common;
 
 use Doctrine\Common\Cache\Cache;
-use Doctrine\ORM\EntityManager;
 use GeoIp2\Database\Reader;
 use GuzzleHttp\Client as GuzzleClient;
 use Monolog\Logger;
@@ -20,7 +19,6 @@ return [
 
     'dependencies' => [
         'factories' => [
-            EntityManager::class => Factory\EntityManagerFactory::class,
             GuzzleClient::class => InvokableFactory::class,
             Cache::class => Factory\CacheFactory::class,
             Filesystem::class => InvokableFactory::class,
@@ -45,7 +43,6 @@ return [
             Service\PreviewGenerator::class => ConfigAbstractFactory::class,
         ],
         'aliases' => [
-            'em' => EntityManager::class,
             'httpClient' => GuzzleClient::class,
             'translator' => Translator::class,
 

module/Common/config/doctrine.config.php

@@ -0,0 +1,32 @@
+<?php
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Common;
+
+use Doctrine\ORM\EntityManager;
+
+return [
+
+    'entity_manager' => [
+        'orm' => [
+            'types' => [
+                Type\ChronosDateTimeType::CHRONOS_DATETIME => Type\ChronosDateTimeType::class,
+            ],
+        ],
+    ],
+
+    'dependencies' => [
+        'factories' => [
+            EntityManager::class => Doctrine\EntityManagerFactory::class,
+        ],
+        'aliases' => [
+            'em' => EntityManager::class,
+        ],
+        'delegators' => [
+            EntityManager::class => [
+                Doctrine\ReopeningEntityManagerDelegator::class,
+            ],
+        ],
+    ],
+
+];

module/Common/src/Doctrine/EntityManagerFactory.php

@@ -1,7 +1,7 @@
 <?php
 declare(strict_types=1);
 
-namespace Shlinkio\Shlink\Common\Factory;
+namespace Shlinkio\Shlink\Common\Doctrine;
 
 use Doctrine\Common\Cache\ArrayCache;
 use Doctrine\Common\Cache\Cache;
@@ -11,36 +11,42 @@ use Doctrine\DBAL\Types\Type;
 use Doctrine\ORM\EntityManager;
 use Doctrine\ORM\ORMException;
 use Doctrine\ORM\Tools\Setup;
-use Interop\Container\ContainerInterface;
-use Shlinkio\Shlink\Common\Type\ChronosDateTimeType;
-use Zend\ServiceManager\Exception\ServiceNotCreatedException;
-use Zend\ServiceManager\Exception\ServiceNotFoundException;
-use Zend\ServiceManager\Factory\FactoryInterface;
+use Psr\Container\ContainerInterface;
 
-class EntityManagerFactory implements FactoryInterface
+class EntityManagerFactory
 {
     /**
-     * @throws ServiceNotFoundException if unable to resolve the service.
-     * @throws ServiceNotCreatedException if an exception is raised when creating a service.
      * @throws ORMException
      * @throws DBALException
      */
-    public function __invoke(ContainerInterface $container, $requestedName, ?array $options = null)
+    public function __invoke(ContainerInterface $container): EntityManager
     {
         $globalConfig = $container->get('config');
-        $isDevMode = isset($globalConfig['debug']) ? ((bool) $globalConfig['debug']) : false;
+        $isDevMode = (bool) ($globalConfig['debug'] ?? false);
         $cache = $container->has(Cache::class) ? $container->get(Cache::class) : new ArrayCache();
         $emConfig = $globalConfig['entity_manager'] ?? [];
         $connectionConfig = $emConfig['connection'] ?? [];
         $ormConfig = $emConfig['orm'] ?? [];
 
-        if (! Type::hasType(ChronosDateTimeType::CHRONOS_DATETIME)) {
-            Type::addType(ChronosDateTimeType::CHRONOS_DATETIME, ChronosDateTimeType::class);
-        }
+        $this->registerTypes($ormConfig);
 
         $config = Setup::createConfiguration($isDevMode, $ormConfig['proxies_dir'] ?? null, $cache);
         $config->setMetadataDriverImpl(new PHPDriver($ormConfig['entities_mappings'] ?? []));
 
         return EntityManager::create($connectionConfig, $config);
     }
+
+    /**
+     * @throws DBALException
+     */
+    private function registerTypes(array $ormConfig): void
+    {
+        $types = $ormConfig['types'] ?? [];
+
+        foreach ($types as $name => $className) {
+            if (! Type::hasType($name)) {
+                Type::addType($name, $className);
+            }
+        }
+    }
 }

module/Common/src/Doctrine/ReopeningEntityManager.php

@@ -0,0 +1,57 @@
+<?php
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Common\Doctrine;
+
+use Doctrine\ORM\Decorator\EntityManagerDecorator;
+use Doctrine\ORM\EntityManagerInterface;
+
+class ReopeningEntityManager extends EntityManagerDecorator
+{
+    /** @var callable */
+    private $emFactory;
+
+    public function __construct(EntityManagerInterface $wrapped, callable $emFactory)
+    {
+        parent::__construct($wrapped);
+        $this->emFactory = $emFactory;
+    }
+
+    protected function getWrappedEntityManager(): EntityManagerInterface
+    {
+        if (! $this->wrapped->isOpen()) {
+            $this->wrapped = ($this->emFactory)(
+                $this->wrapped->getConnection(),
+                $this->wrapped->getConfiguration(),
+                $this->wrapped->getEventManager()
+            );
+        }
+
+        return $this->wrapped;
+    }
+
+    public function flush($entity = null): void
+    {
+        $this->getWrappedEntityManager()->flush($entity);
+    }
+
+    public function persist($object): void
+    {
+        $this->getWrappedEntityManager()->persist($object);
+    }
+
+    public function remove($object): void
+    {
+        $this->getWrappedEntityManager()->remove($object);
+    }
+
+    public function refresh($object): void
+    {
+        $this->getWrappedEntityManager()->refresh($object);
+    }
+
+    public function merge($object)
+    {
+        return $this->getWrappedEntityManager()->merge($object);
+    }
+}

module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php

@@ -0,0 +1,15 @@
+<?php
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Common\Doctrine;
+
+use Doctrine\ORM\EntityManager;
+use Psr\Container\ContainerInterface;
+
+class ReopeningEntityManagerDelegator
+{
+    public function __invoke(ContainerInterface $container, string $name, callable $callback): ReopeningEntityManager
+    {
+        return new ReopeningEntityManager($callback(), [EntityManager::class, 'create']);
+    }
+}

module/Common/src/Middleware/CloseDbConnectionMiddleware.php

@@ -3,13 +3,11 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Common\Middleware;
 
-use Doctrine\ORM\EntityManager;
 use Doctrine\ORM\EntityManagerInterface;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\MiddlewareInterface;
 use Psr\Http\Server\RequestHandlerInterface;
-use Throwable;
 
 class CloseDbConnectionMiddleware implements MiddlewareInterface
 {
@@ -25,16 +23,6 @@ class CloseDbConnectionMiddleware implements MiddlewareInterface
     {
         try {
             return $handler->handle($request);
-        } catch (Throwable $e) {
-            // FIXME Mega ugly hack to avoid a closed EntityManager to make shlink fail forever on swoole contexts
-            //       Should be fixed with request-shared EntityManagers, which is not supported by the ServiceManager
-            if (! $this->em->isOpen()) {
-                (function () {
-                    $this->closed = false;
-                })->bindTo($this->em, EntityManager::class)();
-            }
-
-            throw $e;
         } finally {
             $this->em->getConnection()->close();
             $this->em->clear();

module/Common/test/Doctrine/EntityManagerFactoryTest.php

@@ -1,11 +1,12 @@
 <?php
 declare(strict_types=1);
 
-namespace ShlinkioTest\Shlink\Common\Factory;
+namespace ShlinkioTest\Shlink\Common\Doctrine;
 
 use Doctrine\ORM\EntityManager;
 use PHPUnit\Framework\TestCase;
-use Shlinkio\Shlink\Common\Factory\EntityManagerFactory;
+use Shlinkio\Shlink\Common\Doctrine\EntityManagerFactory;
+use Shlinkio\Shlink\Common\Type\ChronosDateTimeType;
 use Zend\ServiceManager\ServiceManager;
 
 class EntityManagerFactoryTest extends TestCase
@@ -19,12 +20,17 @@ class EntityManagerFactoryTest extends TestCase
     }
 
     /** @test */
-    public function serviceIsCreated()
+    public function serviceIsCreated(): void
     {
         $sm = new ServiceManager(['services' => [
             'config' => [
                 'debug' => true,
                 'entity_manager' => [
+                    'orm' => [
+                        'types' => [
+                            ChronosDateTimeType::CHRONOS_DATETIME => ChronosDateTimeType::class,
+                        ],
+                    ],
                     'connection' => [
                         'driver' => 'pdo_sqlite',
                     ],
@@ -32,7 +38,7 @@ class EntityManagerFactoryTest extends TestCase
             ],
         ]]);
 
-        $em = $this->factory->__invoke($sm, EntityManager::class);
+        $em = ($this->factory)($sm, EntityManager::class);
         $this->assertInstanceOf(EntityManager::class, $em);
     }
 }

module/Common/test/Doctrine/ReopeningEntityManagerDelegatorTest.php

@@ -0,0 +1,28 @@
+<?php
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Common\Doctrine;
+
+use Doctrine\ORM\EntityManagerInterface;
+use PHPUnit\Framework\TestCase;
+use ReflectionObject;
+use Shlinkio\Shlink\Common\Doctrine\ReopeningEntityManagerDelegator;
+use Zend\ServiceManager\ServiceManager;
+
+class ReopeningEntityManagerDelegatorTest extends TestCase
+{
+    /** @test */
+    public function decoratesEntityManagerFromCallback(): void
+    {
+        $em = $this->prophesize(EntityManagerInterface::class)->reveal();
+        $result = (new ReopeningEntityManagerDelegator())(new ServiceManager(), '', function () use ($em) {
+            return $em;
+        });
+
+        $ref = new ReflectionObject($result);
+        $prop = $ref->getProperty('wrapped');
+        $prop->setAccessible(true);
+
+        $this->assertSame($em, $prop->getValue($result));
+    }
+}

module/Common/test/Doctrine/ReopeningEntityManagerTest.php

@@ -0,0 +1,80 @@
+<?php
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Common\Doctrine;
+
+use Doctrine\Common\EventManager;
+use Doctrine\DBAL\Connection;
+use Doctrine\ORM\Configuration;
+use Doctrine\ORM\EntityManagerInterface;
+use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
+use Prophecy\Prophecy\ObjectProphecy;
+use Shlinkio\Shlink\Common\Doctrine\ReopeningEntityManager;
+use stdClass;
+
+class ReopeningEntityManagerTest extends TestCase
+{
+    /** @var ReopeningEntityManager */
+    private $decoratorEm;
+    /** @var ObjectProphecy */
+    private $wrapped;
+
+    public function setUp(): void
+    {
+        $this->wrapped = $this->prophesize(EntityManagerInterface::class);
+        $this->wrapped->getConnection()->willReturn($this->prophesize(Connection::class));
+        $this->wrapped->getConfiguration()->willReturn($this->prophesize(Configuration::class));
+        $this->wrapped->getEventManager()->willReturn($this->prophesize(EventManager::class));
+
+        $wrappedMock = $this->wrapped->reveal();
+        $this->decoratorEm = new ReopeningEntityManager($wrappedMock, function () use ($wrappedMock) {
+            return $wrappedMock;
+        });
+    }
+
+    /**
+     * @test
+     * @dataProvider provideMethodNames
+     */
+    public function wrappedInstanceIsTransparentlyCalledWhenItIsNotClosed(string $methodName): void
+    {
+        $method = $this->wrapped->__call($methodName, [Argument::cetera()])->willReturnArgument();
+        $isOpen = $this->wrapped->isOpen()->willReturn(true);
+
+        $this->decoratorEm->{$methodName}(new stdClass());
+
+        $method->shouldHaveBeenCalledOnce();
+        $isOpen->shouldHaveBeenCalledOnce();
+        $this->wrapped->getConnection()->shouldNotHaveBeenCalled();
+        $this->wrapped->getConfiguration()->shouldNotHaveBeenCalled();
+        $this->wrapped->getEventManager()->shouldNotHaveBeenCalled();
+    }
+
+    /**
+     * @test
+     * @dataProvider provideMethodNames
+     */
+    public function wrappedInstanceIsRecreatedWhenItIsClosed(string $methodName): void
+    {
+        $method = $this->wrapped->__call($methodName, [Argument::cetera()])->willReturnArgument();
+        $isOpen = $this->wrapped->isOpen()->willReturn(false);
+
+        $this->decoratorEm->{$methodName}(new stdClass());
+
+        $method->shouldHaveBeenCalledOnce();
+        $isOpen->shouldHaveBeenCalledOnce();
+        $this->wrapped->getConnection()->shouldHaveBeenCalledOnce();
+        $this->wrapped->getConfiguration()->shouldHaveBeenCalledOnce();
+        $this->wrapped->getEventManager()->shouldHaveBeenCalledOnce();
+    }
+
+    public function provideMethodNames(): iterable
+    {
+        yield 'flush' => ['flush'];
+        yield 'persist' => ['persist'];
+        yield 'remove' => ['remove'];
+        yield 'refresh' => ['refresh'];
+        yield 'merge' => ['merge'];
+    }
+}

module/Common/test/Middleware/CloseDbConnectionMiddlewareTest.php

@@ -10,7 +10,6 @@ use Prophecy\Prophecy\ObjectProphecy;
 use Psr\Http\Server\RequestHandlerInterface;
 use RuntimeException;
 use Shlinkio\Shlink\Common\Middleware\CloseDbConnectionMiddleware;
-use Throwable;
 use Zend\Diactoros\Response;
 use Zend\Diactoros\ServerRequest;
 
@@ -35,7 +34,6 @@ class CloseDbConnectionMiddlewareTest extends TestCase
         $this->em->getConnection()->willReturn($this->conn->reveal());
         $this->em->clear()->will(function () {
         });
-        $this->em->isOpen()->willReturn(true);
 
         $this->middleware = new CloseDbConnectionMiddleware($this->em->reveal());
     }
@@ -71,31 +69,4 @@ class CloseDbConnectionMiddlewareTest extends TestCase
 
         $this->middleware->process($req, $this->handler->reveal());
     }
-
-    /**
-     * @test
-     * @dataProvider provideClosed
-     */
-    public function entityManagerIsReopenedAfterAnExceptionWhichClosedIt(bool $closed): void
-    {
-        $req = new ServerRequest();
-        $expectedError = new RuntimeException();
-        $this->handler->handle($req)->willThrow($expectedError)
-                                    ->shouldBeCalledOnce();
-        $this->em->closed = $closed;
-        $this->em->isOpen()->willReturn(false);
-
-        try {
-            $this->middleware->process($req, $this->handler->reveal());
-            $this->fail('Expected exception to be thrown but it did not.');
-        } catch (Throwable $e) {
-            $this->assertSame($expectedError, $e);
-            $this->assertFalse($this->em->closed);
-        }
-    }
-
-    public function provideClosed(): iterable
-    {
-        return [[true, false]];
-    }
 }

phpstan.neon

@@ -3,6 +3,3 @@ parameters:
 		- '#League\\Plates\\callback#'
 		- '#is not subtype of Throwable#'
 		- '#ObjectManager::flush()#'
-		-
-			message: '#Access to an undefined property#'
-			path: %currentWorkingDirectory%/module/Common/src/Middleware/CloseDbConnectionMiddleware.php