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

start to providing reconnection ability #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zxurian
Copy link

@Zxurian Zxurian commented Mar 2, 2017

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
Copy link
Owner

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
Copy link
Author

Zxurian commented Mar 3, 2017

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
Copy link
Owner

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
Copy link
Author

Zxurian commented Mar 3, 2017

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

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

Successfully merging this pull request may close these issues.

2 participants