Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependency injection into migrations #2183

Open
dakujem opened this issue Mar 13, 2023 · 6 comments
Open

Dependency injection into migrations #2183

dakujem opened this issue Mar 13, 2023 · 6 comments

Comments

@dakujem
Copy link

dakujem commented Mar 13, 2023

Phinx has a serious design issue IMHO.

As far as I can tell, there is no way to inject anything to migrations. At least there is a way to do so with the seeders (using the undocumented 'container' option), which would not be the best solution for the migrations, but would do the job.
Obviously, defining dependencies for every migration would be tedious, unless something with magic (like PHP-DI) is used to construct them.
And there would be the problem of inlining the dynamic arguments ($migration = new $class($environment, $version, $this->getInput(), $this->getOutput());).

Currently, I am able to set my base migration class, which might have the option setManager which would allow to call something like $this->getManager()->getContainer() inside the migrations and to fetch my dependencies from there.
But I am not able to cofigure it in any way, so I can't call the fictional setManager method.

Suggestions

I suggest the following solutions

  1. create an interface allowing us to decorate the migration classes
  2. allow us to set a migration factory (for seeders as well?)
  3. introduce hooks to call after a migration has been instantiated enabling decoration (for seeders as well?)

1. Interface

A simple interface like

interface ManagerAwareMigrationInterface {
  public function setManager(Manager $manager);
}

would do the job.

Inside Manager class, right after instantiating a migration, add this code

if ($migration instanceof ManagerAwareMigrationInterface ){
  $migration->setManager($this);
}

Add Manager::getContainer public method to enable access to the container from within the migrations.

Or do something similar directly using the container

if (if $this->container && $migration instanceof ContainerAwareMigrationInterface ){
  $migration->setContainer($this->container);
}

2. Migration factory

$phinxOptions = [
  'migration_factory' => $container->get(MyMigrationFactory::class),
];

as the possible values, use a generic callable with signature same as the AbstractMigration's constructor (plus the class name) or an interface with a method with the same signature...

// a generic callable
$factory = fn(string $className, string $environment, int $version, ?InputInterface $input = null, ?OutputInterface $output = null): AbstractMigration => WhateverHere();

// an interface
interface MigrationFactory{
  public function createMigration(string $className, string $environment, int $version, ?InputInterface $input = null, ?OutputInterface $output = null): AbstractMigration;
}

3. Decoration hook

$phinxOptions = [
  'migration_decorator' => $container->get(MyMigrationFactory::class)->makeDecorator(),
];
// Manager.php
$migration = new $class($environment, $version, $this->getInput(), $this->getOutput());
if ($this->migrationFactoryHook){
  $migration = ($this->migrationFactoryHook)($migration) ?? $migration;
}

Conclusion

All the above options are simple to implement and will do the job.
I could provide a PR if you wanted me to, but let me know which approach to take.

@rquadling
Copy link
Collaborator

You could extend the base template and add your own DI management and then refer to the new template in your phinx.php or equivalent.

Setting 'templates' => ['file' => '<your new template filename>'] should then work with no changes needed.

Not everyone needs DI.

@rquadling
Copy link
Collaborator

In terms of accessing input and output, the following methods are available:

\Phinx\Migration\AbstractMigration::getOutput()
\Phinx\Migration\AbstractMigration::getInput()

So you can use them in your migrations already via $this->getOutput()->write(). One thing I have in our AbstractMigration class is a writeln() like this:

   protected function writeln($message, bool $newline = true, bool $addPadding = true)
    {
        if (!is_array($message)) {
            $message = [$message];
        }
        $message = array_map(
            function ($message) use ($addPadding) {
                return ($addPadding ? $this->commentPadding : '').$message;
            },
            $message
        );

        $this->getOutput()->write($message, $newline);
    }

So, all the output from a migration is nicely indented and so when you run a migration you get output like:

 == 20230309080316 Bc1068RemoveEnableTodayScreenSetting: migrating
                   Processing : 10 - Delete Setting
 == 20230309080316 Bc1068RemoveEnableTodayScreenSetting: migrated 0.1686s

 == 20230321103317 Bc1115CstMassUpdateBookings: migrating
                   Updating orders...
                   Success log file of this migration can be found here: 2023-03-22-16-24-15_up_Bc1115CstMassUpdateBookings_success.txt
 == 20230321103317 Bc1115CstMassUpdateBookings: migrated 1.7315s

This does make things nice and easy to read for us.

@dakujem
Copy link
Author

dakujem commented Mar 24, 2023

@rquadling I'm sorry, but you misunderstood the issue completely. I don't care about the output or the console commands.
By "decoration" I meant the decorator design pattern.

What I want to be able to do is to call service code inside my migrations' up and down methods, like this:

final class GroupsType extends Migration
{
    public function up(): void
    {
        $rates = $this->container()->get(ConversionService::class)->getRates();
        // or just $this->conversionService()->getRates()
        
        // Now use the rates for the migration code here...
    }
}

And I do want that service to come from my service container, not conjured using static stuff, as that's what I'm currently doing to overcome the issue (using a static facade).

Let me elaborate to prevent further confusion.

The following is my phinx.php used as the configuration:

use My\Database\Migration;

return (function () {
    $container = (new Bootstrap)->container();
    
    // This patches the issue of not having access to the container in the migrations.
    PhinxCrutch::bind(new PhinxCrutch($container));
    
    $settings = $container->get(Settings::class);
    $root = $settings->get('paths.root');
    return [
        'container' => $container, // This only works for fetching the seeders at the time of writing (v13.4).
        'paths' => [
            'migrations' => $root . 'db/migrations',
            'seeds' => $root . 'db/seeds',
        ],
        'migration_base_class' => Migration::class,
        // ...
    ];
})();

And the following is my migration base class

abstract class Migration extends AbstractMigration
{
    public function schema(): SchemaBuilder
    {
        return $this->container()->get(SchemaBuilder::class);
    }

    public function eq(): EloquentConnection
    {
        return $this->container()->get(EloquentConnection::class);
    }

    public function db(): DibiConnection
    {
        return $this->container()->get(DibiConnection::class);
    }

    public function capsule(): Capsule
    {
        return $this->container()->get(Capsule::class);
    }

    protected function container(): Container
    {
        try {
            // This is a facade-like approach to patching the issue of not having a service container injected.
            return PhinxCrutch::instance()->container();
        } catch (LogicFault $e) {
            throw new LogicFault(
                message: 'Unable to fetch DI container for Phinx migrations.',
                previous: $e ?? null,
            );
        }
    }
}

This allows our developers to use whatever DB tech they are accustomed to,
plus gives them access to services like Settings, currency conversions, third party service connections, Redis etc. which occasionally need to be used in the migrations.

@rquadling
Copy link
Collaborator

The example I gave was a bit ultra simplistic.

We use PHP-DI setup in our AbstractMigration class to do all of this.

We then use @DelayedInject which is processed as part of the preFlight() method. This allows migrations to have DI dependencies, without them being instantiated until execution, so NOT when you run phinx status, but only when running phinx migrate and only on the migrations actually running, and not those already run.

This is similar to using @Inject but we effectively delay the injection to a point in time that's more appropriate.

What gets injected is defined by the di-config, so if there's rules about what to use, then great.

My example was that you can create your own AbstractMigration and set this to be the default AbstractMigration for all subsequent migrations created by phinx create.

If the you then add your DI handler in the pre-flight, then, effectively, you've setup whatever processing you need as part of the migration's up() / down() or change() (We prefer the former pattern so have disabled change() and enforced up() / down().

Does this help?

@rquadling
Copy link
Collaborator

The question of is there anything extra that Phinx needs to support is probably more along the lines of what you are asking.

I'd say that whilst the need for both of us to have Phinx support DI for the migrations is business critical, it isn't for everyone.

Implementing DI is relatively trivial with the feature set already present in Phinx.

@dakujem
Copy link
Author

dakujem commented Mar 24, 2023

Okay, I get the point where one does not need to use dependency injection for all the migrations upon creation. This can also be solved using the same patterns I described in the first post.

But if anyone was to do what you suggest, one would have to at least

  • override Manager::executeMigration to decorate the migrations there
  • or override Environment::executeMigration and set the altered environment to the manager
  • or override each of the migration commands to use an altered Manager class
  • and then override the whole PhinxApplication::__construct or create his own application to inject the configured manager into each of commands (or use overridden commands), because the new Manager is hardwired into the abstract command base class and created at the moment the command runs (the issue here being the i/o not being available at configuration time)
  • and then write a new phinx.php script and binary, because the ones provided by the package aren't using the altered ones

So yes, it is doable, but at what cost? It is then much more sane to just use a static facade.

And the need for these measures can be mitigated at the cost of adding a couple of interfaces or hooks and a couple of instanceof or is_null ifs directly into the package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants