davedevelopment / phpmig

Simple migrations system for php
Other
568 stars 93 forks source link

Resolved a small issue with validation of migration setup #97

Closed eman1986 closed 8 years ago

aik099 commented 8 years ago

Can you please describe more specifically what this small issue is in PR description?

eman1986 commented 8 years ago

The issue was it was validating for every method of migration whether you use all of them or not. I made it so that if it detects a method being used it'll then validate for that method. I found this issue happened when trying to use the phpmig set method.

eman1986 commented 8 years ago

The way I wrote it makes it a bit more smarter by checking for things if its actually using a certain migration method

aik099 commented 8 years ago

I'm still not getting what user facing issue this PR solves. There migrations that are already executed and the ones are not. The missing migrations are executed. So what is the bug in current process?

aik099 commented 8 years ago

Looking at code changes made I only see, that you're moved checks around, but the code change doesn't tell why.

eman1986 commented 8 years ago

So if you were to setup a migration with sets it errors out stating you must have phpmig.migration and phpmig.migrations which is false. My fix makes it so it doesn't say you need all of of them.I hit this issue every time I use the set method of migration which is how I'm using it. If you really look at the existing check its looking to see if every way possible is set which isn't needed as a few lines later it already determines that so its not really adding anything to the user except over validation that's not needed

aik099 commented 8 years ago

Could you post content of configuration file, that will allow to replicate the problem?

Also since tests exists in this library, then I guess you need to update existing ones that check for proper exceptions being thrown due invalid configuration.

eman1986 commented 8 years ago
$container['phpmig.sets'] = $container->share(function ($container) {
    return [
        'pages' => [
            'adapter' => new Phpmig\Adapter\Illuminate\Database($container['db'], 'migrations_pages'),
            'migrations_path' => __DIR__ . '/App/Migrations/pages'
        ],
        'user' => [
            'adapter' => new Phpmig\Adapter\Illuminate\Database($container['db'], 'migrations_users'),
            'migrations_path' => __DIR__ . '/App/Migrations/user'
        ]
    ];
});

if you run phpmig with just that you'll get the following error

phpmig.php must return container with array at phpmig.migrations or migrations default path at phpmig.migrations_path or migrations default path at phpmig.sets

if I took that out and just used phpmig.migrations or phpmig.migrations_path it'll work no problem.

I can write up a test case if you want me to.

davedevelopment commented 8 years ago

It's all good, I understand the problem. Thanks!