davedevelopment / phpmig

Simple migrations system for php
Other
569 stars 92 forks source link

start to providing reconnection ability #124

Open Zxurian opened 7 years ago

Zxurian commented 7 years ago

So this is a start to the issue raised in #123 about MySQL going away from phpmig itself. I modified it so that you can provide a function to ['phpmig.adapter'] instead of the actual adapter, which will return the Adapter itself.

It's expandable so that each adapter can have it's own test to see if it's still connected or not (only wrote one for MySQL for now).

davedevelopment commented 7 years ago

Do you think we could come up with a separate interface for adapters that might not be connected? I don't want to have to bump a major version for this? That way the check could be...


if ($adaptor instanceof SomethingInterface && !$adaptor->isConnected()) {
    // ...
}
Zxurian commented 7 years ago

A separate interface would end up being more work I would think, since your adapters implement the interface, but the adapters themselves would still be the same. If you had a second interface, you'd have to have two copies of every adapter, one for each interface.

With the implementation I started, the interface just adds one method isConnected(), which can be implemented in whatever way is best for the adapter. IE, in MySQL, a simple SELECT 1, is sufficient, the other adapters might have different ways of seeing if the connection is still connected.

Since the actual code for reconnecting is built into the getAdapter() of the Migrator, for adapters that don't have it implemented can just return true; for their isConnected() function so it doesn't break backwards compatability.

This also doesn't break backwards compatability (once isConnected() { return true; } is added to the other adapters) since you can still provide an adapter directly, or provide a function to ['phpmig.adapter'] and it'll take either

davedevelopment commented 7 years ago

A separate interface would end up being more work I would think, since your adapters implement the interface, but the adapters themselves would still be the same. If you had a second interface, you'd have to have two copies of every adapter, one for each interface.

Classes can implement more than one interface, so that wouldn't be a problem.

This also doesn't break backwards compatability (once isConnected() { return true; } is added to the other adapters) since you can still provide an adapter directly, or provide a function to ['phpmig.adapter'] and it'll take either

It does break BC, because other people who have created custom adapters etc would have their migration process broken.

Zxurian commented 7 years ago

the multiple inheritance I didn't see as an issue so much as how it would be implemented, ex: here's my current adapter initialization

<?php

use Phpmig\Adapter;
use Zend\Mvc\Application;

$container = new ArrayObject();
$container['zend'] = Zend\Mvc\Application::init(require 'config/application.config.php');

$config = $container['zend']->getServiceManager()->get('config');
$container['phpmig.adapter'] = function() use ($config) {
    $pdo = new PDO($config['database']['dsn'], $config['database']['username'], $config['database']['password']);
    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    return new Adapter\PDO\Sql($pdo, 'migrations');
};
$container['phpmig.migrations_path'] = __DIR__ . DIRECTORY_SEPARATOR . 'migrations';

return $container;

You're right in that I could've written a separate custom adapter instead of modifying Adapter\PDO\Sql, however it seems like every adapter type might suffer the same type of timeout issues. With the default adapters having a built-in isConnected() method, it would allow a check to be made against each of their platforms.

The other kink is that because by default ['phpmig.adapter'] takes an Adapter object itself, there's no built in way to have a safety check to make sure the Adapter is still connected the next time you goto run it, hence the modifications to getAdapter() to use in conjuction with isConnected() to then re-build the Adapter when necessary.

I agree that changing ['phpmig.adapter'] to only accept a function instead of a direct adapter would break backwards compatability, hence the option to provide either. The other way might be to have another option of ['phpmig.adapterFunction'] as well