kohana / minion

Everyone loves having a minion they can boss around
113 stars 76 forks source link

The migrations config is unnecessary and misusing the db groups fucntionality #6

Closed zeelot closed 13 years ago

zeelot commented 13 years ago

Database groups are not for providing connections for multiple environments. They are for providing the application with multiple databases to connect to (default, sessions, wordpress, etc)

Instead, I suggest adding a db_group() method to the Minion_Migration_Base class that allows the migration writer to specify a group to use for that migration. This allows module writers to overwrite said method and provide logic for the application to change the group a migration runs on.

public function db_group()
{
    return Database::$default;
}

Now when I create Module X and add a migration, I can add some logic like so:

public function db_group()
{
    return Kohana::config('module_x')->db_group;
}

which allows anyone that uses my module to overwrite the config in order for the migration to run on a different DB group.

BRMatt commented 13 years ago

IMO the method should first check if there's a default group defined for the migration's location and if so return that, else return the global default.

BRMatt commented 13 years ago

Quick clarification, I meant that in a config file such as minion/migrations.php there'd be a mapping of location => db group

zeelot commented 13 years ago

I'm not sure the location should dictate the db group, I think the migration (each file) should do so. Because a location could run migrations on multiple groups, couldn't they?

BRMatt commented 13 years ago

Well in situations where you don't control the end system (e.g. you're the maintainer of a 3rd party module) then you'd need some way to provide a default group and also allow devs to change the group.

Doing it this way would allow you to set a db group for all migrations in a module, plus you don't need to re-invent the wheel every time.

zeelot commented 13 years ago

Alright, sounds fine. As long as the environment stuff is removed because that should be controlled by the application.

BRMatt commented 13 years ago

Yeah, it's slightly redundent when you can change the environment using the KOHANA_ENV constant

BRMatt commented 13 years ago

This has been implemented, by default it checks if there's an override in the minion config file, else it uses the default

https://github.com/BRMatt/kohana-minion/commit/959d1306695a68ae8b749f4858a1694d9d098eb1