From d294994f63e5201cc1d28b74e9d0a2d9377efaf8 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 22 Jan 2024 23:17:28 -0500 Subject: [PATCH 1/4] Add factory method support to AdapterFactory supporting DI better. --- src/Db/Adapter/AdapterFactory.php | 44 ++++++++----------- .../Db/Adapter/AdapterFactoryTest.php | 17 +++---- 2 files changed, 27 insertions(+), 34 deletions(-) diff --git a/src/Db/Adapter/AdapterFactory.php b/src/Db/Adapter/AdapterFactory.php index 6cdf7563..f102a7d2 100644 --- a/src/Db/Adapter/AdapterFactory.php +++ b/src/Db/Adapter/AdapterFactory.php @@ -8,6 +8,7 @@ namespace Migrations\Db\Adapter; +use Closure; use RuntimeException; /** @@ -39,9 +40,9 @@ public static function instance(): static /** * Class map of database adapters, indexed by PDO::ATTR_DRIVER_NAME. * - * @var array - * @phpstan-var array> - * @psalm-var array> + * @var array + * @phpstan-var array|\Closure> + * @psalm-var array|\Closure> */ protected array $adapters = [ 'mysql' => MysqlAdapter::class, @@ -65,13 +66,15 @@ public static function instance(): static * Register an adapter class with a given name. * * @param string $name Name - * @param string $class Class + * @param \Closure|string $class Class or factory method for the adapter. * @throws \RuntimeException * @return $this */ - public function registerAdapter(string $name, string $class) + public function registerAdapter(string $name, Closure|string $class) { - if (!is_subclass_of($class, AdapterInterface::class)) { + if ( + !($class instanceof Closure || is_subclass_of($class, AdapterInterface::class)) + ) { throw new RuntimeException(sprintf( 'Adapter class "%s" must implement Migrations\\Db\\Adapter\\AdapterInterface', $class @@ -83,14 +86,13 @@ public function registerAdapter(string $name, string $class) } /** - * Get an adapter class by name. + * Get an adapter instance by name. * * @param string $name Name - * @throws \RuntimeException - * @return string - * @phpstan-return class-string<\Migrations\Db\Adapter\AdapterInterface> + * @param array $options Options + * @return \Migrations\Db\Adapter\AdapterInterface */ - protected function getClass(string $name): object|string + public function getAdapter(string $name, array $options): AdapterInterface { if (empty($this->adapters[$name])) { throw new RuntimeException(sprintf( @@ -98,22 +100,12 @@ protected function getClass(string $name): object|string $name )); } + $classOrFactory = $this->adapters[$name]; + if ($classOrFactory instanceof Closure) { + return $classOrFactory($options); + } - return $this->adapters[$name]; - } - - /** - * Get an adapter instance by name. - * - * @param string $name Name - * @param array $options Options - * @return \Migrations\Db\Adapter\AdapterInterface - */ - public function getAdapter(string $name, array $options): AdapterInterface - { - $class = $this->getClass($name); - - return new $class($options); + return new $classOrFactory($options); } /** diff --git a/tests/TestCase/Db/Adapter/AdapterFactoryTest.php b/tests/TestCase/Db/Adapter/AdapterFactoryTest.php index 553a962b..c55ee69d 100644 --- a/tests/TestCase/Db/Adapter/AdapterFactoryTest.php +++ b/tests/TestCase/Db/Adapter/AdapterFactoryTest.php @@ -4,6 +4,7 @@ namespace Migrations\Test\Db\Adapter; use Migrations\Db\Adapter\AdapterFactory; +use Migrations\Db\Adapter\PdoAdapter; use PHPUnit\Framework\TestCase; use ReflectionMethod; use RuntimeException; @@ -27,20 +28,20 @@ protected function tearDown(): void public function testInstanceIsFactory() { - $this->assertInstanceOf('Migrations\Db\Adapter\AdapterFactory', $this->factory); + $this->assertInstanceOf(AdapterFactory::class, $this->factory); } public function testRegisterAdapter() { - // AdapterFactory::getClass is protected, work around it to avoid - // creating unnecessary instances and making the test more complex. - $method = new ReflectionMethod(get_class($this->factory), 'getClass'); - $method->setAccessible(true); - $adapter = $method->invoke($this->factory, 'mysql'); - $this->factory->registerAdapter('test', $adapter); + $mock = $this->getMockForAbstractClass(PdoAdapter::class, [['foo' => 'bar']]); + $this->factory->registerAdapter('test', function (array $options) use ($mock) { + $this->assertEquals('value', $options['key']); + + return $mock; + }); - $this->assertEquals($adapter, $method->invoke($this->factory, 'test')); + $this->assertEquals($mock, $this->factory->getAdapter('test', ['key' => 'value'])); } public function testRegisterAdapterFailure() From 67436e5c7c7fbd957a9394f505c1d3093ba118e2 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Tue, 23 Jan 2024 00:05:04 -0500 Subject: [PATCH 2/4] Get Environment tests passing with migrations engine Using the PhinxAdapter we can maintain compatibility with existing Migrations, and have an API solution for both now as we transition onto the migrations engine and after phinx is removed, the API doesn't change much just the namespaces change. --- src/Migration/Environment.php | 52 +++++++----- tests/TestCase/Migration/EnvironmentTest.php | 86 ++++++-------------- 2 files changed, 55 insertions(+), 83 deletions(-) diff --git a/src/Migration/Environment.php b/src/Migration/Environment.php index 7d916e50..bbb2029d 100644 --- a/src/Migration/Environment.php +++ b/src/Migration/Environment.php @@ -10,6 +10,7 @@ use Migrations\Db\Adapter\AdapterFactory; use Migrations\Db\Adapter\AdapterInterface; +use Migrations\Db\Adapter\PhinxAdapter; use PDO; use Phinx\Migration\MigrationInterface; use Phinx\Seed\SeedInterface; @@ -78,10 +79,11 @@ public function executeMigration(MigrationInterface $migration, string $directio $migration->setMigratingUp($direction === MigrationInterface::UP); $startTime = time(); - // Need to get a phinx interface adapter here. We will need to have a shim - // to bridge the interfaces. Changing the MigrationInterface is tricky - // because of the method names. - $migration->setAdapter($this->getAdapter()); + // Use an adapter shim to bridge between the new migrations + // engine and the Phinx compatible interface + $adapter = $this->getAdapter(); + $phinxShim = new PhinxAdapter($adapter); + $migration->setAdapter($phinxShim); $migration->preFlightCheck(); @@ -90,24 +92,29 @@ public function executeMigration(MigrationInterface $migration, string $directio } // begin the transaction if the adapter supports it - if ($this->getAdapter()->hasTransactions()) { - $this->getAdapter()->beginTransaction(); + if ($adapter->hasTransactions()) { + $adapter->beginTransaction(); } if (!$fake) { // Run the migration if (method_exists($migration, MigrationInterface::CHANGE)) { if ($direction === MigrationInterface::DOWN) { - // Create an instance of the ProxyAdapter so we can record all + // Create an instance of the RecordingAdapter so we can record all // of the migration commands for reverse playback - /** @var \Phinx\Db\Adapter\ProxyAdapter $proxyAdapter */ - $proxyAdapter = AdapterFactory::instance() - ->getWrapper('proxy', $this->getAdapter()); - $migration->setAdapter($proxyAdapter); + /** @var \Migrations\Db\Adapter\RecordingAdapter $recordAdapter */ + $recordAdapter = AdapterFactory::instance() + ->getWrapper('record', $adapter); + + // Wrap the adapter with a phinx shim to maintain contain + $phinxAdapter = new PhinxAdapter($recordAdapter); + $migration->setAdapter($phinxAdapter); + $migration->{MigrationInterface::CHANGE}(); - $proxyAdapter->executeInvertedCommands(); - $migration->setAdapter($this->getAdapter()); + $recordAdapter->executeInvertedCommands(); + + $migration->setAdapter(new PhinxAdapter($this->getAdapter())); } else { $migration->{MigrationInterface::CHANGE}(); } @@ -117,11 +124,11 @@ public function executeMigration(MigrationInterface $migration, string $directio } // Record it in the database - $this->getAdapter()->migrated($migration, $direction, date('Y-m-d H:i:s', $startTime), date('Y-m-d H:i:s', time())); + $adapter->migrated($migration, $direction, date('Y-m-d H:i:s', $startTime), date('Y-m-d H:i:s', time())); // commit the transaction if the adapter supports it - if ($this->getAdapter()->hasTransactions()) { - $this->getAdapter()->commitTransaction(); + if ($adapter->hasTransactions()) { + $adapter->commitTransaction(); } $migration->postFlightCheck(); @@ -135,14 +142,17 @@ public function executeMigration(MigrationInterface $migration, string $directio */ public function executeSeed(SeedInterface $seed): void { - $seed->setAdapter($this->getAdapter()); + $adapter = $this->getAdapter(); + $phinxAdapter = new PhinxAdapter($adapter); + + $seed->setAdapter($phinxAdapter); if (method_exists($seed, SeedInterface::INIT)) { $seed->{SeedInterface::INIT}(); } // begin the transaction if the adapter supports it - if ($this->getAdapter()->hasTransactions()) { - $this->getAdapter()->beginTransaction(); + if ($adapter->hasTransactions()) { + $adapter->beginTransaction(); } // Run the seeder @@ -151,8 +161,8 @@ public function executeSeed(SeedInterface $seed): void } // commit the transaction if the adapter supports it - if ($this->getAdapter()->hasTransactions()) { - $this->getAdapter()->commitTransaction(); + if ($adapter->hasTransactions()) { + $adapter->commitTransaction(); } } diff --git a/tests/TestCase/Migration/EnvironmentTest.php b/tests/TestCase/Migration/EnvironmentTest.php index 60b5994a..f1a8c63e 100644 --- a/tests/TestCase/Migration/EnvironmentTest.php +++ b/tests/TestCase/Migration/EnvironmentTest.php @@ -4,9 +4,14 @@ namespace Test\Phinx\Migration; use Migrations\Db\Adapter\AdapterFactory; +use Migrations\Db\Adapter\PdoAdapter; +use Migrations\Db\Adapter\PhinxAdapter; use Migrations\Migration\Environment; use PDO; +use Phinx\Migration\AbstractMigration; use Phinx\Migration\MigrationInterface; +use Phinx\Seed\AbstractSeed; +use Phinx\Seed\SeedInterface; use PHPUnit\Framework\TestCase; use RuntimeException; use stdClass; @@ -59,42 +64,6 @@ public function testNoAdapter() $this->environment->getAdapter(); } - private function getPdoMock() - { - $pdoMock = $this->getMockBuilder(PDO::class)->disableOriginalConstructor()->getMock(); - $attributes = []; - $pdoMock->method('setAttribute')->will($this->returnCallback(function ($attribute, $value) use (&$attributes) { - $attributes[$attribute] = $value; - - return true; - })); - $pdoMock->method('getAttribute')->will($this->returnCallback(function ($attribute) use (&$attributes) { - return $attributes[$attribute] ?? 'pdomock'; - })); - - return $pdoMock; - } - - public function testGetAdapterWithExistingPdoInstance() - { - $this->markTestIncomplete('Requires a shim adapter to pass.'); - $adapter = $this->getMockForAbstractClass('\Migrations\Db\Adapter\PdoAdapter', [['foo' => 'bar']]); - AdapterFactory::instance()->registerAdapter('pdomock', $adapter); - $this->environment->setOptions(['connection' => $this->getPdoMock()]); - $options = $this->environment->getAdapter()->getOptions(); - $this->assertEquals('pdomock', $options['adapter']); - } - - public function testSetPdoAttributeToErrmodeException() - { - $this->markTestIncomplete('Requires a shim adapter to pass.'); - $adapter = $this->getMockForAbstractClass('\Migrations\Db\Adapter\PdoAdapter', [['foo' => 'bar']]); - AdapterFactory::instance()->registerAdapter('pdomock', $adapter); - $this->environment->setOptions(['connection' => $this->getPdoMock()]); - $options = $this->environment->getAdapter()->getOptions(); - $this->assertEquals(PDO::ERRMODE_EXCEPTION, $options['connection']->getAttribute(PDO::ATTR_ERRMODE)); - } - public function testGetAdapterWithBadExistingPdoInstance() { $this->environment->setOptions(['connection' => new stdClass()]); @@ -115,7 +84,7 @@ public function testSchemaName() public function testCurrentVersion() { - $stub = $this->getMockBuilder('\Migrations\Db\Adapter\PdoAdapter') + $stub = $this->getMockBuilder(PdoAdapter::class) ->setConstructorArgs([[]]) ->getMock(); $stub->expects($this->any()) @@ -130,7 +99,7 @@ public function testCurrentVersion() public function testExecutingAMigrationUp() { // stub adapter - $adapterStub = $this->getMockBuilder('\Migrations\Db\Adapter\PdoAdapter') + $adapterStub = $this->getMockBuilder(PdoAdapter::class) ->setConstructorArgs([[]]) ->getMock(); $adapterStub->expects($this->once()) @@ -140,21 +109,20 @@ public function testExecutingAMigrationUp() $this->environment->setAdapter($adapterStub); // up - $upMigration = $this->getMockBuilder('\Phinx\Migration\AbstractMigration') + $upMigration = $this->getMockBuilder(AbstractMigration::class) ->setConstructorArgs(['mockenv', '20110301080000']) ->addMethods(['up']) ->getMock(); $upMigration->expects($this->once()) ->method('up'); - $this->markTestIncomplete('Requires a shim adapter to pass.'); $this->environment->executeMigration($upMigration, MigrationInterface::UP); } public function testExecutingAMigrationDown() { // stub adapter - $adapterStub = $this->getMockBuilder('\Migrations\Db\Adapter\PdoAdapter') + $adapterStub = $this->getMockBuilder(PdoAdapter::class) ->setConstructorArgs([[]]) ->getMock(); $adapterStub->expects($this->once()) @@ -164,22 +132,20 @@ public function testExecutingAMigrationDown() $this->environment->setAdapter($adapterStub); // down - $downMigration = $this->getMockBuilder('\Phinx\Migration\AbstractMigration') + $downMigration = $this->getMockBuilder(AbstractMigration::class) ->setConstructorArgs(['mockenv', '20110301080000']) ->addMethods(['down']) ->getMock(); $downMigration->expects($this->once()) ->method('down'); - $this->markTestIncomplete('Requires a shim adapter to pass.'); $this->environment->executeMigration($downMigration, MigrationInterface::DOWN); } public function testExecutingAMigrationWithTransactions() { - $this->markTestIncomplete('Requires a shim adapter to pass.'); // stub adapter - $adapterStub = $this->getMockBuilder('\Migrations\Db\Adapter\PdoAdapter') + $adapterStub = $this->getMockBuilder(PdoAdapter::class) ->setConstructorArgs([[]]) ->getMock(); $adapterStub->expects($this->once()) @@ -195,7 +161,7 @@ public function testExecutingAMigrationWithTransactions() $this->environment->setAdapter($adapterStub); // migrate - $migration = $this->getMockBuilder('\Phinx\Migration\AbstractMigration') + $migration = $this->getMockBuilder(AbstractMigration::class) ->setConstructorArgs(['mockenv', '20110301080000']) ->addMethods(['up']) ->getMock(); @@ -207,9 +173,8 @@ public function testExecutingAMigrationWithTransactions() public function testExecutingAChangeMigrationUp() { - $this->markTestIncomplete('Requires a shim adapter to pass.'); // stub adapter - $adapterStub = $this->getMockBuilder('\Migrations\Db\Adapter\PdoAdapter') + $adapterStub = $this->getMockBuilder(PdoAdapter::class) ->setConstructorArgs([[]]) ->getMock(); $adapterStub->expects($this->once()) @@ -219,7 +184,7 @@ public function testExecutingAChangeMigrationUp() $this->environment->setAdapter($adapterStub); // migration - $migration = $this->getMockBuilder('\Phinx\Migration\AbstractMigration') + $migration = $this->getMockBuilder(AbstractMigration::class) ->setConstructorArgs(['mockenv', '20130301080000']) ->addMethods(['change']) ->getMock(); @@ -231,9 +196,8 @@ public function testExecutingAChangeMigrationUp() public function testExecutingAChangeMigrationDown() { - $this->markTestIncomplete('Requires a shim adapter to pass.'); // stub adapter - $adapterStub = $this->getMockBuilder('\Migrations\Db\Adapter\PdoAdapter') + $adapterStub = $this->getMockBuilder(PdoAdapter::class) ->setConstructorArgs([[]]) ->getMock(); $adapterStub->expects($this->once()) @@ -243,7 +207,7 @@ public function testExecutingAChangeMigrationDown() $this->environment->setAdapter($adapterStub); // migration - $migration = $this->getMockBuilder('\Phinx\Migration\AbstractMigration') + $migration = $this->getMockBuilder(AbstractMigration::class) ->setConstructorArgs(['mockenv', '20130301080000']) ->addMethods(['change']) ->getMock(); @@ -255,9 +219,8 @@ public function testExecutingAChangeMigrationDown() public function testExecutingAFakeMigration() { - $this->markTestIncomplete('Requires a shim adapter to pass.'); // stub adapter - $adapterStub = $this->getMockBuilder('\Migrations\Db\Adapter\PdoAdapter') + $adapterStub = $this->getMockBuilder(PdoAdapter::class) ->setConstructorArgs([[]]) ->getMock(); $adapterStub->expects($this->once()) @@ -267,7 +230,7 @@ public function testExecutingAFakeMigration() $this->environment->setAdapter($adapterStub); // migration - $migration = $this->getMockBuilder('\Phinx\Migration\AbstractMigration') + $migration = $this->getMockBuilder(AbstractMigration::class) ->setConstructorArgs(['mockenv', '20130301080000']) ->addMethods(['change']) ->getMock(); @@ -288,9 +251,8 @@ public function testGettingInputObject() public function testExecuteMigrationCallsInit() { - $this->markTestIncomplete('Requires a shim adapter to pass.'); // stub adapter - $adapterStub = $this->getMockBuilder('\Migrations\Db\Adapter\PdoAdapter') + $adapterStub = $this->getMockBuilder(PdoAdapter::class) ->setConstructorArgs([[]]) ->getMock(); $adapterStub->expects($this->once()) @@ -300,7 +262,7 @@ public function testExecuteMigrationCallsInit() $this->environment->setAdapter($adapterStub); // up - $upMigration = $this->getMockBuilder('\Phinx\Migration\AbstractMigration') + $upMigration = $this->getMockBuilder(AbstractMigration::class) ->setConstructorArgs(['mockenv', '20110301080000']) ->addMethods(['up', 'init']) ->getMock(); @@ -314,17 +276,17 @@ public function testExecuteMigrationCallsInit() public function testExecuteSeedInit() { - $this->markTestIncomplete('Requires a shim adapter to pass.'); // stub adapter - $adapterStub = $this->getMockBuilder('\Migrations\Db\Adapter\PdoAdapter') + $adapterStub = $this->getMockBuilder(PdoAdapter::class) ->setConstructorArgs([[]]) ->getMock(); $this->environment->setAdapter($adapterStub); // up - $seed = $this->getMockBuilder('\Migrations\AbstractSeed') - ->onlyMethods(['run', 'init']) + $seed = $this->getMockBuilder(AbstractSeed::class) + ->addMethods(['init']) + ->onlyMethods(['run']) ->getMock(); $seed->expects($this->once()) From e827ae6e2d0825bdfabd790064d9fbc051497030 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Tue, 23 Jan 2024 00:31:49 -0500 Subject: [PATCH 3/4] Update phpcs, psalm, phpstan --- phpstan-baseline.neon | 15 --------------- psalm-baseline.xml | 10 +++++----- tests/TestCase/Migration/EnvironmentTest.php | 4 ---- 3 files changed, 5 insertions(+), 24 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index d4891e5e..eebf9dfe 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -15,11 +15,6 @@ parameters: count: 1 path: src/Command/BakeMigrationSnapshotCommand.php - - - message: "#^Method Migrations\\\\Db\\\\Adapter\\\\AdapterFactory\\:\\:getAdapter\\(\\) should return Migrations\\\\Db\\\\Adapter\\\\AdapterInterface but returns object\\.$#" - count: 1 - path: src/Db/Adapter/AdapterFactory.php - - message: "#^Unsafe usage of new static\\(\\)\\.$#" count: 1 @@ -75,16 +70,6 @@ parameters: count: 2 path: src/Db/Adapter/SqlserverAdapter.php - - - message: "#^Parameter \\#1 \\$adapter of method Phinx\\\\Migration\\\\MigrationInterface\\:\\:setAdapter\\(\\) expects Phinx\\\\Db\\\\Adapter\\\\AdapterInterface, Migrations\\\\Db\\\\Adapter\\\\AdapterInterface given\\.$#" - count: 2 - path: src/Migration/Environment.php - - - - message: "#^Parameter \\#1 \\$adapter of method Phinx\\\\Seed\\\\SeedInterface\\:\\:setAdapter\\(\\) expects Phinx\\\\Db\\\\Adapter\\\\AdapterInterface, Migrations\\\\Db\\\\Adapter\\\\AdapterInterface given\\.$#" - count: 1 - path: src/Migration/Environment.php - - message: "#^Possibly invalid array key type Cake\\\\Database\\\\Schema\\\\TableSchemaInterface\\|string\\.$#" count: 2 diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 7c1cf486..b1e9de84 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -15,6 +15,11 @@ output)]]> + + + adapters]]> + + getQueryBuilder @@ -69,11 +74,6 @@ - - getAdapter()]]> - getAdapter()]]> - getAdapter()]]> - adapter)]]> diff --git a/tests/TestCase/Migration/EnvironmentTest.php b/tests/TestCase/Migration/EnvironmentTest.php index f1a8c63e..fdfac5b8 100644 --- a/tests/TestCase/Migration/EnvironmentTest.php +++ b/tests/TestCase/Migration/EnvironmentTest.php @@ -3,15 +3,11 @@ namespace Test\Phinx\Migration; -use Migrations\Db\Adapter\AdapterFactory; use Migrations\Db\Adapter\PdoAdapter; -use Migrations\Db\Adapter\PhinxAdapter; use Migrations\Migration\Environment; -use PDO; use Phinx\Migration\AbstractMigration; use Phinx\Migration\MigrationInterface; use Phinx\Seed\AbstractSeed; -use Phinx\Seed\SeedInterface; use PHPUnit\Framework\TestCase; use RuntimeException; use stdClass; From ac088cefca9a1bf6c6328380fc73eb45d904d9e4 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Fri, 26 Jan 2024 00:36:20 -0500 Subject: [PATCH 4/4] Fix phpcs --- tests/TestCase/Db/Adapter/AdapterFactoryTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/TestCase/Db/Adapter/AdapterFactoryTest.php b/tests/TestCase/Db/Adapter/AdapterFactoryTest.php index c55ee69d..6fc41e16 100644 --- a/tests/TestCase/Db/Adapter/AdapterFactoryTest.php +++ b/tests/TestCase/Db/Adapter/AdapterFactoryTest.php @@ -33,7 +33,6 @@ public function testInstanceIsFactory() public function testRegisterAdapter() { - $mock = $this->getMockForAbstractClass(PdoAdapter::class, [['foo' => 'bar']]); $this->factory->registerAdapter('test', function (array $options) use ($mock) { $this->assertEquals('value', $options['key']);