Feature Request #2753

Database::set_default($name)

Added by Rick Jolly over 4 years ago. Updated over 2 years ago.

Status:ClosedStart date:03/27/2010
Priority:NormalDue date:
Assignee:Woody Gilk% Done:

0%

Category:Modules:Database
Target version:v3.0.5
Resolution:duplicate Points:

Description

There is some confusion about how to set the default database when switching between different environments where you can't have a static "default" configuration in the config file:
http://forum.kohanaphp.com/comments.php?DiscussionID=3149
http://forum.kohanaphp.com/comments.php?DiscussionID=4215
http://forum.kohanaphp.com/comments.php?DiscussionID=5178

One solution is to put logic in the database config file. Another is to set the default database before Kohana does (maybe in a parent model). How about a built-in method like so:

public static function set_default($config)
{
   if (! is_array($config))
   {
      $config = Kohana::config('database')->$config;
   }

   if ( ! isset($config['type']))
   {
      throw new Kohana_Exception('Database type not defined in configuration');
   }

   // Set the driver class name
   $driver = 'Database_'.ucfirst($config['type']);

   // Create the database connection instance
   new $driver('default', $config);
}


Related issues

Related to Kohana v3.x - Feature Request #2849: Improved consistency when initializing classes with confi... Closed 05/04/2010

History

#1 Updated by Isaiah DeRose-Wilson over 4 years ago

Honestly, I think adding another option for changing the default database will just make things more confusing. Also your set_default() function looks a lot like Database::instance()... I'm not really sure what you are trying to do with it. I think the best way to solve this confusion is with documentation.

#2 Updated by Rick Jolly over 4 years ago

Isaiah, yes the code is almost exactly like Database::instance().

I think the main advantages of this method are:

1. Clarity: By looking at the api, it's clear how to set the default database and perhaps there wouldn't be the confusion that is evident in the threads linked above.
2. Ease of use: It's easier to use "set_default" than "instance" as shown below:

Database::instance('default', Kohana::config('database')->development);
Database::set_default('development');

#3 Updated by Rick Jolly over 4 years ago

Also, I might be alone on this but, I don't think Kohana should ever set the default database on its own. It's dangerous if the developer ever forgets to set the default before a query. I guess the best place to set the default is in the bootstrap to avoid that possibility.

#4 Updated by Isaiah DeRose-Wilson over 4 years ago

Actually the instance() function should be used like this:

// default database
$db = Database::instance();
// production database
$db = Database::instance('production');

The second 'config' argument is only used if you want to change one of the configuration values when you are creating the instance of the database, you don't use it when loading them from a config file.

To me set_default() is confusing, because it's not changing the default database. All it does is create an instance of the database using the configuration group you specified (exactly the same as Database::instance()).

#5 Updated by Rick Jolly over 4 years ago

Isaiah DeRose-Wilson wrote:

Actually the instance() function should be used like this:
[...]

The second 'config' argument is only used if you want to change one of the configuration values when you are creating the instance of the database, you don't use it when loading them from a config file.

To me set_default() is confusing, because it's not changing the default database. All it does is create an instance of the database using the configuration group you specified (exactly the same as Database::instance()).

Isaiah, Database::instance() does return an instance, but before it does, it also sets the instance in an internal array. If the default database isn't yet set, then you actually can set it using the Database::instance() method. The default database isn't loaded by Kohana until a the query execute() method is called without a database specified, and the default database hasn't yet been created. So by setting the default instance before any queries are run, we can use our own without messing with logic in the config file.

The fact that you and I weren't initially aware of this kind of makes my point. It's hard to know how to override the default database based on the current environment. Database::instance() isn't a clean or intuitive way to do this.

#6 Updated by Isaiah DeRose-Wilson over 4 years ago

I'm not really sure what you are talking about. Database::instance() keeps an internal array of all the database 'group' instances loaded and if you request the same group twice the second request will return the existing instance (it's a quasi singleton class). I'm not really sure what any of this has to do with setting a default database group though. Are you talking about overwriting the existing default configuration by passing in additional configuration data? That doesn't make much sense to me.

It looks like set_default() is just a factory function? I'm confused!

#7 Updated by Rick Jolly over 4 years ago

Sorry, I've tried to explain as best I can. Maybe instead of focusing on the solution, understanding the problem will help.

I use the DB class for all my queries. If I don't pass a database to the query execute() method, then Kohana uses the default database. That is a good thing as long as I can control what the default database is. Because I have a different database for development and production, I need to be able to control which one I set as default.

I'm curious how others work around this problem.

#8 Updated by Michael Peters over 4 years ago

Use a different config file

#9 Updated by Isaiah DeRose-Wilson over 4 years ago

Yes understanding the problem always helps :)

The way I see it is you have three options. (1) Use different config files (loading in modules), (2) put logic in your database config file to change the default configuration, or (3) let your models decide and always pass a database instance in when executing your queries. I normally do options 1 and 2. Sometimes I don't want to commit my local config files to public repos, so putting the config files in different modules works best. However when this isn't an issue, putting logic in your config files is simple and there is nothing wrong with it.

#10 Updated by Lorenzo Pisani over 4 years ago

I don't think the database config groups are for production vs development. That's what the environment is for. The default group is your main database, you create other groups if you have other databases you want to connect to (to migrate data, or to work with an external application somewhere else)

When you are in 'development' you should have a system to change out all the database group names that have a development version (usually at least the default group). We usually keep an entirely different config file for development that has all the groups in it. The default group is our development server and so our models continue to function with the same DB group switching over to the live one when we deploy automatically.

You should not have a Development and Live or Production group in your database config file... this makes things much more complicated in my opinion... 99% of your applications will probably only need a default group.

Hope that's helpful =D

#11 Updated by Rick Jolly over 4 years ago

Thanks guys.

I disagree that logic belongs in a config file. That isn't consistent with Kohana or any other software that I know of. With most software, config files are static text (xml, ini, etc.) so logic isn't even an option.

The one suggestion I hadn't considered was different config files. I don't use an automatic deployment script and I don't care if others with access to the source see my development database config. So the best way to switch configs would seem to be having different modules just for configuration. Then in the bootstrap, load the appropriate module based on the environment. Not a bad approach, but I do think it's asking too much of the developer.

#12 Updated by Rick Jolly over 4 years ago

I think this is important:

As I've mentioned, I don't like how Kohana automatically loads the default config group. If you look at routes and modules, they are loaded explicitly for good reason - so that the developer can make decisions about what is loaded dynamically. Would anyone suggest that Kohana should blindly load those from a config file? So, for example, we couldn't load different modules based on environment? Should database be different? I see no reason why.

#13 Updated by Lorenzo Pisani over 4 years ago

the difference is that 99% of the time, an application will only have 1 database to connect to...

#14 Updated by Rick Jolly over 4 years ago

2 databases Lorenzo - development and production. If it were one static database I wouldn't have filed this feature request. I don't think you understand the issue.

It's ironic that switching modules for database config based on environment wouldn't be possible if Kohana blindly loaded modules from a config - at least not without the hacks we've been discussing.

#15 Updated by Michael Peters over 4 years ago

Rick Jolly wrote:

I disagree that logic belongs in a config file. That isn't consistent with Kohana or any other software that I know of. With most software, config files are static text (xml, ini, etc.) so logic isn't even an option.

So because other software doesn't let you do it, you shouldn't ever have logic in your config files? Seems like a weak argument...

Do whatever is easiest for you. If extending Database to have a set_default() works for you, great. But it's something that is not needed 99% of the time, and there are other (perhaps better) ways of achieving what you are trying to do, so there is no sense in bloating the Kohana core with it.

#16 Updated by Rick Jolly over 4 years ago

Michael Peters wrote:

So because other software doesn't let you do it, you shouldn't ever have logic in your config files? Seems like a weak argument...

No, other software doesn't do it because it is bad practice.

Do whatever is easiest for you. If extending Database to have a set_default() works for you, great. But it's something that is not needed 99% of the time, and there are other (perhaps better) ways of achieving what you are trying to do, so there is no sense in bloating the Kohana core with it.

It's something that all developers need 99% of the time. I'm really confused how you think otherwise. Do your development and production databases use the same credentials?

Of course I can extend Kohana. I could make it like CI if I wanted. But I'd rather help make it better for everyone. That is the purpose of feature requests. Debating with strong counter-arguments is productive, but idly dismissing change is not.

#17 Updated by Michael Peters over 4 years ago

Rick Jolly wrote:

No, other software doesn't do it because it is bad practice.

Source? Why would that be bad practice? I mean, there are better ways of doing it, but I can't really think of a big reason why logic in a config file is horrible.

It's something that all developers need 99% of the time. I'm really confused how you think otherwise. Do your development and production databases use the same credentials?

Of course not, but thats the whole point of having config files. Just have a different config file on your dev and production servers. If that's not possible with the way you push things to production, then just one config file with some logic. (But I can't think of a good deployment solution that doesn't let you ignore things like logs and config)

#18 Updated by Lorenzo Pisani over 4 years ago

I just explained how development and production are NOT 2 config groups... they should both be the default group... you are handling environments badly

#19 Updated by Rick Jolly over 4 years ago

Michael Peters wrote:

Source? Why would that be bad practice? I mean, there are better ways of doing it, but I can't really think of a big reason why logic in a config file is horrible.

It's bad practice because there are better ways to do it. As for source, I could list any good software, or ask the world's best programmers, but I don't have to since you acknowleged it isn't ideal. I won't argue this anymore.

Just have a different config file on your dev and production servers. If that's not possible with the way you push things to production, then just one config file with some logic. (But I can't think of a good deployment solution that doesn't let you ignore things like logs and config)

I push everything to production without having to make a single change in code. Not one. It saves time and prevents errors without requiring a deployment script. The simplest working approach is usually best. So separate configs doesn't work for me. No need to rehash the logic in the config file.

#20 Updated by Rick Jolly over 4 years ago

Lorenzo Pisani wrote:

I just explained how development and production are NOT 2 config groups... they should both be the default group... you are handling environments badly

You haven't explained how you load different configs. That would be helpful. I've acknowledged a solution with different config modules loaded according to environment. Is that what you suggest? I've said that isn't a bad solution, it just isn't that obvious - hence the confusion in the forum threads. If you are suggesting having to manually change configs, well that is unacceptable.

#21 Updated by Matt Button over 4 years ago

Logic in config is not ideal, but neither is a set_default() method.

Either create separate "modules" for environment specific configuration (like Isiah suggested), or define the environments in the config and in the bootstrap do:

Kohana::config('database.default', Kohana::get('database.'.Kohana::$enviroment));

IMO the chances of a set_default() function getting into core are extremely unlikely.

#22 Updated by Lorenzo Pisani over 4 years ago

your bootstrap file can load an extra config reader for development environments, this new one would have a config file for database that overwrites the other...

when you deploy, the environment will be production and the new config reader will not be loaded above the regular one (with the live DB config)

#23 Updated by Rick Jolly over 4 years ago

Good stuff. I'm interested in specific solutions.

@Matt, your suggestion is a good one, but I can't see it working as is. Kohana::config only accepts one parameter and I haven't found a Kohana::get method.

@Lorenzo, I haven't found a way to overwrite a config in Kohana 3. I think you are suggesting adding a config reader and setting it to run first for development so that it executes before the default Kohana config reader? That way, it "hides" the default config rather than overwriting it? I don't think that was the intention of config readers, but I suppose it's a solution.

#24 Updated by Rick Jolly over 4 years ago

Ok, so I figured out how to overwrite configs. Perhaps the best solution is to put this in the bootstrap:

$environment = Kohana::$environment;
Kohana::config('database')->default = Kohana::config('database')->$environment;

I'm satisfied. Thanks for helping me work through this.

#25 Updated by Isaiah DeRose-Wilson over 4 years ago

  • Status changed from New to Closed
  • Assignee changed from Woody Gilk to Isaiah DeRose-Wilson
  • Resolution set to invalid

Glad you figured out a solution that works for you.

#26 Updated by Woody Gilk over 4 years ago

Rick Jolly wrote:

Ok, so I figured out how to overwrite configs. Perhaps the best solution is to put this in the bootstrap:

[...]

I'm satisfied. Thanks for helping me work through this.

You can configure the database dynamically in config/database.php:

$config = array();
if (Kohana::$environment === 'production'){
    $config['default']['connection']['database'] = 'prod';
    // Anything can be overloaded here
}

return $config;

#27 Updated by Matt Button over 4 years ago

Rick Jolly wrote:

@Matt, your suggestion is a good one, but I can't see it working as is. Kohana::config only accepts one parameter and I haven't found a Kohana::get method.

Sorry, I wasn't thinking straight when I wrote that.

Woody Gilk Wrote:

You can configure the database dynamically in config/database.php:

He (Rick) didn't seem to like the idea of putting that sort of logic in config files...

#28 Updated by Woody Gilk over 4 years ago

Matt B wrote:

Woody Gilk Wrote:

You can configure the database dynamically in config/database.php:

He (Rick) didn't seem to like the idea of putting that sort of logic in config files...

Unfortunate, because it is the most clear and reasonable solution.

#29 Updated by Rick Jolly about 4 years ago

The recent change to use a default static class variable resolves this issue. It's a better, simpler fix. http://github.com/kohana/database/commit/321c676e56ece86505beb6c2752973f11bff004c

#30 Updated by Woody Gilk about 4 years ago

  • Category set to Modules:Database
  • Assignee changed from Isaiah DeRose-Wilson to Woody Gilk
  • Target version set to v3.0.5

#31 Updated by Woody Gilk about 4 years ago

  • Resolution changed from invalid to duplicate

#32 Updated by farijon farijon over 2 years ago

I love spam.

Also available in: Atom PDF