[Mack]

Hamador, on 02 April 2010 - 08:24 AM, said:

What if you would give it an alias when registering the connection, whenever a database connection is called without an alias it would default to your primary source as you have it now. This would prevent anyone currently in use of Hydrogen from having to change their code as the default connection would always be used unless specified otherwise.

I may just build a separate class that will build the connection and call the SQLBean within it.

In the config file, you could have a new field Database Name. Then you could call the database using that name, and if left blank it would default to the first one.

[Hamador]

Mack, on 02 April 2010 - 09:08 AM, said:

In the config file, you could have a new field Database Name. Then you could call the database using that name, and if left blank it would default to the first one.

Mack,

I have all the database information stored in the config file for both databases, however I would need to copy/paste the following code every time I needed to access the second database :

				$host = Config::getVal('Altiris', 'host', false);
				$port = Config::getVal('Altiris', 'port', false);
				$socket = Config::getVal('Altiris', 'socket', false);
				$database = Config::getVal('Altiris', 'database', false);
				$username = Config::getVal('Altiris', 'username', false);
				$password = Config::getVal('Altiris', 'password', false);
				$engine = Config::getVal('Altiris', 'engine');
				$db = DatabaseEngineFactory::getEngine($host, $port, $socket, $database, $username, $password, $engine);
				$alt_q = new Query("SELECT", $db, False);
				return AltirisBean::select($alt_q);

I was hoping to built that into the SQLBean, since that bean would only ever access that database. Unless I'm misunderstanding your suggestion, that wouldn't help.

[Mack]

Hamador, on 02 April 2010 - 09:36 AM, said:

Mack,

I have all the database information stored in the config file for both databases, however I would need to copy/paste the following code every time I needed to access the second database :

				$host = Config::getVal('Altiris', 'host', false);				$port = Config::getVal('Altiris', 'port', false);				$socket = Config::getVal('Altiris', 'socket', false);				$database = Config::getVal('Altiris', 'database', false);				$username = Config::getVal('Altiris', 'username', false);				$password = Config::getVal('Altiris', 'password', false);				$engine = Config::getVal('Altiris', 'engine');				$db = DatabaseEngineFactory::getEngine($host, $port, $socket, $database, $username, $password, $engine);				$alt_q = new Query("SELECT", $db, False);				return AltirisBean::select($alt_q);

I was hoping to built that into the SQLBean, since that bean would only ever access that database. Unless I'm misunderstanding your suggestion, that wouldn't help.

In the config, you could have two databases, [database_firstname] and [database_secondname]. You then call DatabaseEngineFactory::getEngine(firstname) and DatabaseEngineFactory::getEngine(secondname) and it would get the config from [database_firstname] and [database_secondname]. That way you could store all the database settings in the config file, and it would get it from the config file that way.

I don't know how well I'm explaining this though :/

[Hamador]

Mack, on 02 April 2010 - 10:03 AM, said:

In the config, you could have two databases, [database_firstname] and [database_secondname]. You then call DatabaseEngineFactory::getEngine(firstname) and DatabaseEngineFactory::getEngine(secondname) and it would get the config from [database_firstname] and [database_secondname]. That way you could store all the database settings in the config file, and it would get it from the config file that way.

I don't know how well I'm explaining this though :/

Ok I see what you're saying, that would actually work great. It would require retrofitting the DatabaseFactoryEngine and all other classes that interact with it, but I definitely like the idea, I'll see what I can do without breaking Kyek's wonderful code

[Kyek]

That's a great idea It feels a little dirty doing it with the ini group name, but this would be clean(er):

[general]
; This part you would add yourself for your own app
primary_db = default
secondary_db = remote_backup

[database]
; This part would be built into the config
config_name[] = "default"
engine[] = "MysqlPDO"
host[] = "localhost"
port[] = 3306
socket[] = 
database[] = "hydrogenapp"
username[] = "hydrogenapp"
password[] = "password"
table_prefix[] = "hydro_"

config_name[] = "remote_backup"
engine[] = "MysqlPDO"
host[] = "remotehost.mydomain.com"
port[] = 3306
socket[] = 
database[] = "hydrogenapp"
username[] = "hydrogenapp"
password[] = "password"
table_prefix[] = "hydro_"

Then DatabaseEngineFactory could be made to get the config by name in addition to specifying connection info on-the-fly. And this way, you're not hardcoding any names. Just do this:

$engine = DatabaseEngineFactory::getEngineByName(Config::getVal("general", "primary_db"));

Then getEngineByName could read the config like this:

public static function getEngineByName($configName) {
    if (isset($engineNames[$configName]))
        return $engineNames[$configName];
    $names = Config::getVal("database", "config_name");
    $idx = -1;
    for ($i = 0; $i < count($names); $i++) {
        if ($names[$i] === $configName) {
            $idx = $i;
            break;
        }
    }
    if ($idx === -1)
        throw new NoSuchDBNameException("Configuration named '$configName' does not exist.");

    $host = Config::getVal("database", "host");
    if (!isset($host[$idx]))
        throw new MissingDBConfigException("No 'host' field for DB config named '$configName'.");
    $host = $host[$idx];

    // etc etc etc...
}

But getEngine() would need to be rewritten to support names, and support using them when no name is specified, and support using the config options when they're not arrays.

But if you want to write something like that, be my guest . If you're feeling adventurous, fork my github repo, write it in, then send me a pull request

[Mack]

Kyek, on 02 April 2010 - 12:46 PM, said:

That's a great idea It feels a little dirty doing it with the ini group name, but this would be clean(er):

[general]
; This part you would add yourself for your own app
primary_db = default
secondary_db = remote_backup

[database]
; This part would be built into the config
config_name[] = "default"
engine[] = "MysqlPDO"
host[] = "localhost"
port[] = 3306
socket[] = 
database[] = "hydrogenapp"
username[] = "hydrogenapp"
password[] = "password"
table_prefix[] = "hydro_"

config_name[] = "remote_backup"
engine[] = "MysqlPDO"
host[] = "remotehost.mydomain.com"
port[] = 3306
socket[] = 
database[] = "hydrogenapp"
username[] = "hydrogenapp"
password[] = "password"
table_prefix[] = "hydro_"

Then DatabaseEngineFactory could be made to get the config by name in addition to specifying connection info on-the-fly. And this way, you're not hardcoding any names. Just do this:

$engine = DatabaseEngineFactory::getEngineByName(Config::getVal("general", "primary_db"));

Then getEngineByName could read the config like this:

public static function getEngineByName($configName) {
    if (isset($engineNames[$configName]))
        return $engineNames[$configName];
    $names = Config::getVal("database", "config_name");
    $idx = -1;
    for ($i = 0; $i < count($names); $i++) {
        if ($names[$i] === $configName) {
            $idx = $i;
            break;
        }
    }
    if ($idx === -1)
        throw new NoSuchDBNameException("Configuration named '$configName' does not exist.");

    $host = Config::getVal("database", "host");
    if (!isset($host[$idx]))
        throw new MissingDBConfigException("No 'host' field for DB config named '$configName'.");
    $host = $host[$idx];

    // etc etc etc...
}

But getEngine() would need to be rewritten to support names, and support using them when no name is specified, and support using the config options when they're not arrays.

But if you want to write something like that, be my guest . If you're feeling adventurous, fork my github repo, write it in, then send me a pull request

getEngine() could stay the same, but you could then add getEngineByName() too. I was thinking it should be all inside [database], but I couldn't think of a good way to make it work if the user had no name in it, but having a default in [general] is a good idea

[Kyek]

The thing is, if getEngine() stays the same, you're forced to make getEngineByName do alllll the Config::getVal calls every time you call it. The engine index that makes sure there can only ever be one connection to each database stores the engine objects under this key name:

$key = ($host ?: '') . ':' . ($port ?: '') . ':' . ($socket ?: '') . ':' . 
	($database ?: '') . ':' . ($username ?: '') . ':' . ($engineClass ?: '');

So getEngineByName would have to get each of those things out of the database every time you call it, which defeats the purpose of naming the connection. If you do it that way, you might as well get all the config values in the main part of your code any not worry about the name.

So if I decided to add connection naming to Hydrogen, I'd have to change the engine index so that it indexes by name first, and only using the key above if the config is not named.

I'm still not sold that that's the way to go, though. I the cleaner solution might be for the app itself to have a function that gets the secondary engine (so that all the Config::getVal calls can be there and you wouldn't have to repeat them in the main part of the code). OR, the app could extend DatabaseEngineFactory and just add another function to do it, custom written for the app. I'm still not sure -- but those methods (especially the latter) both use less CPU time and make the config file much more readable.

[Mack]

Kyek, on 02 April 2010 - 06:26 PM, said:

The thing is, if getEngine() stays the same, you're forced to make getEngineByName do alllll the Config::getVal calls every time you call it. The engine index that makes sure there can only ever be one connection to each database stores the engine objects under this key name:

$key = ($host ?: '') . ':' . ($port ?: '') . ':' . ($socket ?: '') . ':' . 
	($database ?: '') . ':' . ($username ?: '') . ':' . ($engineClass ?: '');

So getEngineByName would have to get each of those things out of the database every time you call it, which defeats the purpose of naming the connection. If you do it that way, you might as well get all the config values in the main part of your code any not worry about the name.

So if I decided to add connection naming to Hydrogen, I'd have to change the engine index so that it indexes by name first, and only using the key above if the config is not named.

I'm still not sold that that's the way to go, though. I the cleaner solution might be for the app itself to have a function that gets the secondary engine (so that all the Config::getVal calls can be there and you wouldn't have to repeat them in the main part of the code). OR, the app could extend DatabaseEngineFactory and just add another function to do it, custom written for the app. I'm still not sure -- but those methods (especially the latter) both use less CPU time and make the config file much more readable.

Maybe have getEngine extend getEngineByName or something? Have it so if getEngine() is called, it is the equivalent of calling getEngineByName(default)

[Mack]

Whats wrong with this code (I'm sure theres something I'm not seeing )

<?php

use hydrogen\database\Query;

$query = new Query("INSERT");
$query->intoTable("count");
$query->intoField("user_id");
$query->intoField("score");
$query->intoField("time");
$query->values("(?, ?, NOW())", $username, $score);
$stmt = $query->prepare();
$stmt->execute();

?>

Hydrogens already included in another file.

Fatal error: Uncaught exception 'hydrogen\database\exceptions\InvalidSQLException' with message 'Value mismatch error: More ? symbols were used than values provided.' in /var/www/fb/public/lib/hydrogen/database/formatters/StandardSQLFormatter.php:63 Stack trace: #0 /var/www/fb/public/lib/hydrogen/database/statements/QueryStatement.php(48): hydrogen\database\formatters\StandardSQLFormatter->getPreparedValues(Array) #1 /var/www/fb/public/log.php(12): hydrogen\database\statements\QueryStatement->execute() #2 /var/www/fb/public/index.php(74): include('/var/www/fb/pub...') #3 {main} thrown in /var/www/fb/public/lib/hydrogen/database/formatters/StandardSQLFormatter.php on line 63


Edit: After rereading your tutorial, I see that it should've been in an array. Oops. In the tutorial, the two variables are arrays inside of arrays, so it must have confused me

[Hamador]

So here's what I did :

The configuration file looks like this:

[database]
engine[default] = "MysqlPDO"
host[default] = "localhost"
port[default] = 3306
socket[default] = 
database[default] = "hydrogen"
username[default] = "hydrogen"
password[default] = "hydrogen"
table_prefix[default] = ""

engine[altiris] = "MysqlPDO"
host[altiris] = "localhost"
port[altiris] = 3306
socket[altiris] = 
database[altiris] = "mysql"h
username[altiris] = "root"
password[altiris] = "password"

I've changed DatabaseFactoryEngine to take the name of the database as its first variable. This is only used to pull the variable if it is not set. I also modified the getVal function to accept the subkey as a variable to allow for pulling the exact variable you want. I then went and changed Query and SQLBean to accept the database name as a variable and pass it on to getEngine to select the database you want. I believe I made all the necessary adjustments to accommodate the changes I made, and have had no problems with my basic testing. The basic calls are as seen below:

$db = DatabaseEngineFactory::getEngine('secondary');
$stmt =  $db->prepare("SELECT * FROM users");
$stmt->execute();

$query = new Query("SELECT",'','','secondary');
$query->field("*");	
$query->from("db");
$stm = $query->prepare();
$stm->execute();

$alt = new AltirisBean('secondary');
$alt->Name = "Freddy";
$alt->Comment = "Hello Fred";
$alt->insert();
$altiris = AltirisBean::select('secondary');

Questions, Opinions, Critiques . . . please.

[Mack]

Hamador, on 02 April 2010 - 11:27 PM, said:

So here's what I did :

The configuration file looks like this:

[database]engine[default] = "MysqlPDO"host[default] = "localhost"port[default] = 3306socket[default] = database[default] = "hydrogen"username[default] = "hydrogen"password[default] = "hydrogen"table_prefix[default] = ""engine[altiris] = "MysqlPDO"host[altiris] = "localhost"port[altiris] = 3306socket[altiris] = database[altiris] = "mysql"husername[altiris] = "root"password[altiris] = "password"

I've changed DatabaseFactoryEngine to take the name of the database as its first variable. This is only used to pull the variable if it is not set. I also modified the getVal function to accept the subkey as a variable to allow for pulling the exact variable you want. I then went and changed Query and SQLBean to accept the database name as a variable and pass it on to getEngine to select the database you want. I believe I made all the necessary adjustments to accommodate the changes I made, and have had no problems with my basic testing. The basic calls are as seen below:

$db = DatabaseEngineFactory::getEngine('secondary');$stmt =  $db->prepare("SELECT * FROM users");$stmt->execute();$query = new Query("SELECT",'','','secondary');$query->field("*");	$query->from("db");$stm = $query->prepare();$stm->execute();$alt = new AltirisBean('secondary');$alt->Name = "Freddy";$alt->Comment = "Hello Fred";$alt->insert();$altiris = AltirisBean::select('secondary');

Questions, Opinions, Critiques . . . please.

Looks good, it works fine? If so, I'd say this should be part of the next alpha release.

[Kyek]

Very sexy, Ham! Will your changes allow the original single-db configuration to work without modification?

Edit: And did you do this with Git? If not, you might want to think about it -- that way I can merge your code with my new changes, plus you get credit on GitHub

[Hamador]

Kyek, on 03 April 2010 - 07:19 AM, said:

Very sexy, Ham! Will your changes allow the original single-db configuration to work without modification?

Edit: And did you do this with Git? If not, you might want to think about it -- that way I can merge your code with my new changes, plus you get credit on GitHub

Yes/No, and No. So for this to work, the database config must be key[subkey] and subkey=default for primary database. If the dbselect variables are left blank it will use the default database. Some of the accepted variables in the functions have changed from your original layout:

getEngine($dbselect=false, $host=false, $port=false, $socket=false, $database=false, $username=false, $password=false, $engine=false)

Query($verb, $dbengine=false, $autoTablePrefix=true, $db_select=false)

SQLBean($dbSelect=false, $dbengine=false, &$dbrow=false, $fieldPrefix=false, $bindRow=true)
select($dbSelect=false, $query=false, $doMapping=false, $mapOverride=false, $dbengine=false)

As far as Git . . . as much as I hate to admit, I've never really worked with any kind version control, but I'll definitely look into it.

[Kyek]

Hmm... How about a solution compromise:

Rather than adding another argument to Query::__construct, SQLBean::__construct, and SQLBean::select(), just allow the existing $dbengine field to accept a DatabaseEngine object OR a string -- the name of the engine to create. So if it's an engine object, it uses that engine. If it's a string, it gets the engine with that name. If it's false, it takes the first (or only) database config and gets that engine. Anything else, throw new hydrogen\database\exceptions\NoSuchEngineException.

Then, in DatabaseEngineFactory, since PHP doesn't allow you to ^%#$&^#%$$#@ overload function names, I think it's still cleaner to have two methods -- getEngine() and getEngineByName(). My reasoning is that I don't expect it would be at all common for people to need more than one database configuration, so making that the first thing that would need to be typed into that function would be an annoyance for most Hydrogen users. I think splitting it up like that is a good compromise

With that, then, the old configuration style could still be made to work. People with only one database wouldn't need the brackets in their config (which might be potentially confusing to the people they distribute their application to), people with multiple databases could use them. DatabaseEngineFactory would just have to be smart enough to consider both possibilities

Your change to the Config library is killer, though. Love it love it love it

[Hamador]

That, I think may be able to accomodate. Let's see what I can do before bedtime today, I may even figure out how to use Git

[Hamador]

I think I may have got it . . . Kyek, I'd like you take a look over my fork before I do a pull request : http://github.com/hamador/Hydrogen.git

[Mack]

Hamador, on 04 April 2010 - 12:25 PM, said:

I think I may have got it . . . Kyek, I'd like you take a look over my fork before I do a pull request : http://github.com/hamador/Hydrogen.git

Isn't it git://github.com/hamador/Hydrogen.git and http://github.com/hamador/Hydrogen? Gives me a 404 if I just go to that, although I don't know much about git

[Kyek]

The http URL with .git is for use with git itself . But yeah-- for web browser viewing, you just pull the .git off.

Trapped on my phone all day Ham, but I'll let you know as soon as I get to a decent-sized screen

[Mack]

Kyek, on 04 April 2010 - 04:04 PM, said:

The http URL with .git is for use with git itself . But yeah-- for web browser viewing, you just pull the .git off.

Trapped on my phone all day Ham, but I'll let you know as soon as I get to a decent-sized screen

but http://... didn't work right for git with me, only git://..., although it could just be some screwed up config I have

[Kyek]

Finally getting to look at your code This is awesome. My notes:

Query
- Your needing to look up the table prefix for the given engine is something I hadn't anticipated. Your solution is clean, but the problem you run into with that is that, if I pass the secondary engine in with the engine object and not the name of the database configuration, the code has no idea what prefix to use. Concerning and frustrating. Maybe it would be better to have the DatabaseEngine abstract contain the name of the config, if it has one? That way you can pull it out of the engine itself rather than relying on the coder to supply it. Hmm... I'm not sure if that's the best way to do it or not. Thoughts?

- With the code as-is, you're calling the Config library to get the array of table prefixes. Then, if what it gives back is an array, you call Config again to get the table prefix with the appropriate array key. It would be doubly fast to just stick the key into the array you already have to get that value, rather than making another Config call

Config
- I... I have no idea why I was returning '' instead of false when exceptionOnError is off. Wtf? Actually... maybe I was doing it so I could use the return value in concatenation. Hmm. This will take more exploration. Creating a new string takes way more cycles than returning false. Do you know what you get when you concatenate a string with false? Either way, thanks for sticking with what I already had there

- I'm torn on the order of arguments in getVal(). Ideally, it would be nice to be able to say getVal($section, $key, $subkey, $exceptionOnError), but it's so rare that you need the subkey... getVal($section, $key, false, $exceptionOnError) would be a pain after awhile. But keeping getVal($section, $key, $exceptionOnError, $subkey) feels so messy. These aren't so much notes, as they are my streams of consciousness xD. Maybe instead of making $exceptionOnError an argument in the public function, it should be an argument in a private function, then we'll have TWO public functions. a getVal-like function for throwing an exception, and a getVal-like function for returning false. So getVal($section, $key, $subkey) would throw the exception, and getValOrFalse($section, $key, $subkey) wouldn't. Or maybe getValNoError. Or the return-false version could be getVal and the exception-throwing version could be getRequiredVal. Thoughts? xD God I'm tired.

DatabaseEngineFactory
- Line 55->85 ... holy shit I can't believe I spelled "port" wrong. Thanks for the correction!

- You changed this line:

$engine = $engine ?: Config::getVal('database', 'engine');

To this line on line 90:

$engine = $engine ?: Config::getVal('database', 'engine', true);

Those two lines do exactly the same thing The third argument is true by default. Did you mean false there?

- We shouldn't force keyed database entries to have one named "default". In PHP, there's an array method (array_keys()) to get all the keys in an array, and they're returned in the order they've been declared in. So maybe we can see if one of them is named 'default' and use that, but if there's not, then just use the first one? Much more freedom for programmers with that.
Edit: Now I'm second-guessing myself. What if a programmer would rather the default be 'main'? Or 'primary'? Maybe going with the first declared key instead of looking for certain names would be best anyway. Thoughts?

SQLBean
- Wow, that fix was really dead easy with the changes made above. That's awesome Beautiful code, dude!

General crap
- If you can, try to match my coding style as much as you can with this . Everyone has their own personal preferences, but we should keep the format consistent throughout the library. Opening brace at the end of the same line that needs it, closing brace left-aligned with that line, contents 1 tab to the right of that line, else if instead of elseif, no braces around if-statement contents if the contents are one line... all that jazz. Not that I couldn't reformat it myself, but hey, I'm lazy ;-)

- If you change a function that has Javadoc (err.. PHPDoc. Whatever) on it, make sure to update the documentation

--------

Really kickass changes, most definitely Just let me know how you feel about the above. The more I'm thinking about it, the more I really like the idea of getVal and getRequiredVal (which I can do -- not saying you have to change what you've written I can accept your pull with that part as-is and write the update myself). I just have no idea what the best solution would be for table prefix lookups from an engine.

...and after all that, I now realize GitHub has an amazing notes system that allows you to leave overall notes in a file as well as line-specific notes. One day, I won't suck. One day ;-)

Edit: Another thought!
If we go with getVal and getRequiredVal, mabe getVal could have a fourth parameter to define what should be returned when the specified key isn't found. It could be 'false' by default, but you could pass it "" or NULL or whatever you wanted and it would return that instead. So that way, if you're doing string concatenation, you could have it return "" and you're in the clear. This is assuming, again, that you can't concatenate a string with boolean false. Because if that works, then there's probably no need for this. Maybe.