From bdc93a45b5c4fe0c6ccd021961e4a20ff55d3703 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 2 Aug 2019 19:28:10 +0200 Subject: [PATCH 1/4] Created EntityManagerDecorator to handle the automatic reopening, and removed this behavior from ClosDbConnectionMiddleware --- config/autoload/entity-manager.global.php | 2 +- module/Common/config/dependencies.config.php | 3 -- module/Common/config/doctrine.config.php | 32 +++++++++++++ .../EntityManagerFactory.php | 34 +++++++------ .../src/Doctrine/ReopeningEntityManager.php | 48 +++++++++++++++++++ .../ReopeningEntityManagerDelegator.php | 17 +++++++ .../CloseDbConnectionMiddleware.php | 12 ----- .../EntityManagerFactoryTest.php | 14 ++++-- .../CloseDbConnectionMiddlewareTest.php | 29 ----------- 9 files changed, 128 insertions(+), 63 deletions(-) create mode 100644 module/Common/config/doctrine.config.php rename module/Common/src/{Factory => Doctrine}/EntityManagerFactory.php (52%) create mode 100644 module/Common/src/Doctrine/ReopeningEntityManager.php create mode 100644 module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php rename module/Common/test/{Factory => Doctrine}/EntityManagerFactoryTest.php (60%) diff --git a/config/autoload/entity-manager.global.php b/config/autoload/entity-manager.global.php index 951c909f..04f999d9 100644 --- a/config/autoload/entity-manager.global.php +++ b/config/autoload/entity-manager.global.php @@ -1,7 +1,7 @@ [ '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, diff --git a/module/Common/config/doctrine.config.php b/module/Common/config/doctrine.config.php new file mode 100644 index 00000000..66d678af --- /dev/null +++ b/module/Common/config/doctrine.config.php @@ -0,0 +1,32 @@ + [ + '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, + ], + ], + ], + +]; diff --git a/module/Common/src/Factory/EntityManagerFactory.php b/module/Common/src/Doctrine/EntityManagerFactory.php similarity index 52% rename from module/Common/src/Factory/EntityManagerFactory.php rename to module/Common/src/Doctrine/EntityManagerFactory.php index b1de186b..43847645 100644 --- a/module/Common/src/Factory/EntityManagerFactory.php +++ b/module/Common/src/Doctrine/EntityManagerFactory.php @@ -1,7 +1,7 @@ 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); + } + } + } } diff --git a/module/Common/src/Doctrine/ReopeningEntityManager.php b/module/Common/src/Doctrine/ReopeningEntityManager.php new file mode 100644 index 00000000..134dcd22 --- /dev/null +++ b/module/Common/src/Doctrine/ReopeningEntityManager.php @@ -0,0 +1,48 @@ +wrapped->isOpen()) { + $this->wrapped= EntityManager::create( + $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); + } +} diff --git a/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php b/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php new file mode 100644 index 00000000..2de8b651 --- /dev/null +++ b/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php @@ -0,0 +1,17 @@ +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(); diff --git a/module/Common/test/Factory/EntityManagerFactoryTest.php b/module/Common/test/Doctrine/EntityManagerFactoryTest.php similarity index 60% rename from module/Common/test/Factory/EntityManagerFactoryTest.php rename to module/Common/test/Doctrine/EntityManagerFactoryTest.php index 70a969f9..ade8d84d 100644 --- a/module/Common/test/Factory/EntityManagerFactoryTest.php +++ b/module/Common/test/Doctrine/EntityManagerFactoryTest.php @@ -1,11 +1,12 @@ [ '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); } } diff --git a/module/Common/test/Middleware/CloseDbConnectionMiddlewareTest.php b/module/Common/test/Middleware/CloseDbConnectionMiddlewareTest.php index 34d141ad..2bf93a85 100644 --- a/module/Common/test/Middleware/CloseDbConnectionMiddlewareTest.php +++ b/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]]; - } } From f99053d25154d2ee96d8c2daec21e8d276a6f805 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 2 Aug 2019 19:33:31 +0200 Subject: [PATCH 2/4] Created ReopeningEntityManagerDelegatorTest --- .../src/Doctrine/ReopeningEntityManager.php | 5 ++-- .../ReopeningEntityManagerDelegator.php | 5 +--- .../ReopeningEntityManagerDelegatorTest.php | 28 +++++++++++++++++++ 3 files changed, 32 insertions(+), 6 deletions(-) create mode 100644 module/Common/test/Doctrine/ReopeningEntityManagerDelegatorTest.php diff --git a/module/Common/src/Doctrine/ReopeningEntityManager.php b/module/Common/src/Doctrine/ReopeningEntityManager.php index 134dcd22..e040b978 100644 --- a/module/Common/src/Doctrine/ReopeningEntityManager.php +++ b/module/Common/src/Doctrine/ReopeningEntityManager.php @@ -5,13 +5,14 @@ namespace Shlinkio\Shlink\Common\Doctrine; use Doctrine\ORM\Decorator\EntityManagerDecorator; use Doctrine\ORM\EntityManager; +use Doctrine\ORM\EntityManagerInterface; class ReopeningEntityManager extends EntityManagerDecorator { - protected function getWrappedEntityManager(): EntityManager + protected function getWrappedEntityManager(): EntityManagerInterface { if (! $this->wrapped->isOpen()) { - $this->wrapped= EntityManager::create( + $this->wrapped = EntityManager::create( $this->wrapped->getConnection(), $this->wrapped->getConfiguration(), $this->wrapped->getEventManager() diff --git a/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php b/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php index 2de8b651..3128b0cf 100644 --- a/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php +++ b/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php @@ -3,15 +3,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Common\Doctrine; -use Doctrine\ORM\EntityManagerInterface; use Psr\Container\ContainerInterface; class ReopeningEntityManagerDelegator { public function __invoke(ContainerInterface $container, string $name, callable $callback): ReopeningEntityManager { - /** @var EntityManagerInterface $em */ - $em = $callback(); - return new ReopeningEntityManager($em); + return new ReopeningEntityManager($callback()); } } diff --git a/module/Common/test/Doctrine/ReopeningEntityManagerDelegatorTest.php b/module/Common/test/Doctrine/ReopeningEntityManagerDelegatorTest.php new file mode 100644 index 00000000..1193541b --- /dev/null +++ b/module/Common/test/Doctrine/ReopeningEntityManagerDelegatorTest.php @@ -0,0 +1,28 @@ +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)); + } +} From bfd2ce782c9f181047f07530ee2c528868025b6e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 2 Aug 2019 19:53:19 +0200 Subject: [PATCH 3/4] Created ReopeningEntityManagerTest --- .../src/Doctrine/ReopeningEntityManager.php | 12 ++- .../ReopeningEntityManagerDelegator.php | 3 +- .../Doctrine/ReopeningEntityManagerTest.php | 80 +++++++++++++++++++ 3 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 module/Common/test/Doctrine/ReopeningEntityManagerTest.php diff --git a/module/Common/src/Doctrine/ReopeningEntityManager.php b/module/Common/src/Doctrine/ReopeningEntityManager.php index e040b978..d9a80e2e 100644 --- a/module/Common/src/Doctrine/ReopeningEntityManager.php +++ b/module/Common/src/Doctrine/ReopeningEntityManager.php @@ -4,15 +4,23 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Common\Doctrine; use Doctrine\ORM\Decorator\EntityManagerDecorator; -use Doctrine\ORM\EntityManager; 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 = EntityManager::create( + $this->wrapped = ($this->emFactory)( $this->wrapped->getConnection(), $this->wrapped->getConfiguration(), $this->wrapped->getEventManager() diff --git a/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php b/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php index 3128b0cf..3ea6a2f1 100644 --- a/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php +++ b/module/Common/src/Doctrine/ReopeningEntityManagerDelegator.php @@ -3,12 +3,13 @@ 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()); + return new ReopeningEntityManager($callback(), [EntityManager::class, 'create']); } } diff --git a/module/Common/test/Doctrine/ReopeningEntityManagerTest.php b/module/Common/test/Doctrine/ReopeningEntityManagerTest.php new file mode 100644 index 00000000..74e8d260 --- /dev/null +++ b/module/Common/test/Doctrine/ReopeningEntityManagerTest.php @@ -0,0 +1,80 @@ +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']; + } +} From 3cba3f7a4bff07ee31c2bbf80ea98aef08b6480c Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 2 Aug 2019 19:55:21 +0200 Subject: [PATCH 4/4] Removed error which no longer needs to be supressed from phpstan --- CHANGELOG.md | 2 +- phpstan.neon | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2853c00..509bcee2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/phpstan.neon b/phpstan.neon index c735bfec..7756bc14 100644 --- a/phpstan.neon +++ b/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