DoctrineMigrationsBundle icon indicating copy to clipboard operation
DoctrineMigrationsBundle copied to clipboard

Add service migrations (POC)

Open HypeMC opened this issue 2 years ago • 12 comments

Inspired by https://github.com/doctrine/DoctrineMigrationsBundle/issues/521#issuecomment-1855593824

This is a proposed implementation for registering migrations as services.

To get this working Symfony needs to be able to autoload migrations:

    "autoload": {
        "psr-4": {
            "App\\": "src/",
            "DoctrineMigrations\\": "migrations/"
        }
    },

Then, import the migrations as services:

    DoctrineMigrations\:
        resource: '../migrations/'

Of course, an alternative approach would be to move the migrations folder to src and change the namespace to App\Migrations.

Once that is done, all you need to do is override the constructor and inject the desired services.

final class Version20240121015756 extends AbstractMigration
{
    public function __construct(
        Connection              $connection,
        LoggerInterface         $logger,
        private RouterInterface $router,
    ) {
        parent::__construct($connection, $logger);
    }

    public function up(Schema $schema): void
    {
        dump($this->router);

        // ...
    }

    // ...
}

I haven't added any tests yet, as I was hoping to get some feedback on my approach first.

HypeMC avatar Jan 21 '24 17:01 HypeMC

Here you can find a similar but much simpler implementation. My solution works without extending the autoloader. https://github.com/doctrine/DoctrineMigrationsBundle/compare/3.3.x...connorhu:DoctrineMigrationsBundle:3.3.x

connorhu avatar Feb 03 '24 19:02 connorhu

This PR will require to register migrations as services, right? If we go down that road, why would I need them to be service subscribers? I mean, I could just inject all dependencies directly, couldn't I?

derrabus avatar Feb 04 '24 17:02 derrabus

@derrabus The reason for my approach are the standard migration arguments: Connection $connection, LoggerInterface $logger. Connection would get autowired to the connection in Doctrine bundle, not the result of doctrine.migrations.dependency_factory::getConnection(), same goes for the logger. Perhaps I could use bindings instead :thinking: . I'll give it a try.

HypeMC avatar Feb 04 '24 17:02 HypeMC

@derrabus I've updated the implementation to use bound arguments instead of service locators.

HypeMC avatar Feb 26 '24 01:02 HypeMC

@greg0ire any plan when this will be merged?

AlexOstrovsky avatar Apr 02 '24 10:04 AlexOstrovsky

No

greg0ire avatar Apr 02 '24 11:04 greg0ire

@greg0ire what is blocking this from beeing merged?

AlexOstrovsky avatar Apr 03 '24 07:04 AlexOstrovsky

For starters:

  • [x] There are conflicts.
  • [x] The CI should be green.
  • [ ] The conversation with @derrabus must be resolved.

greg0ire avatar Apr 03 '24 07:04 greg0ire

@greg0ire There doesn't seem to be a branch 3.6.x.

HypeMC avatar Apr 03 '24 08:04 HypeMC

Oh right sorry, I got confused with another project (DoctrineFixturesBundle). 3.4.x is correct.

greg0ire avatar Apr 03 '24 08:04 greg0ire

I know it's just a POC for now, but since I'm asked what's blocking, I'd add that docs are probably necessary for this sort of thing.

greg0ire avatar Apr 03 '24 08:04 greg0ire

I started testing this PR with real world code. You should work on making this unnecessary:

    DoctrineMigrations\:
        resource: '../migrations/'

connorhu avatar Jun 05 '24 08:06 connorhu