1
\$\begingroup\$

I wanted to get some feedback on this way of initializing a connection to a MySQL database.

File includes/db.php:

<?php
$config_path = __DIR__ .  '/../config/database.ini';

$db_ini = parse_ini_file($config_path , true);
$db_info['db_host'] = $db_ini['db_data_source_name']['host'];
$db_info['db_user'] = $db_ini['db_general']['user'];
$db_info['db_pass'] = $db_ini['db_general']['password'];
$db_info['db_name'] = $db_ini['db_data_source_name']['dbname'];

foreach($db_info as $key => $value) {
    $upper_key = strtoupper($key);
    if (!defined($upper_key)) {
        define($upper_key, $value);
    }
}

$connection = mysqli_connect(DB_HOST, DB_USER, DB_PASS, DB_NAME);

?>

Is there anything wrong with the file above being included in other php files as needed?

Thank you!

\$\endgroup\$
2
\$\begingroup\$

Scope

The only real problem I can forsee is that you may end up re-initializing a new connection to the DB if you include this more than once. That's an easy fix:

if(empty($connection)) $connection = ...

That way you will only re-init the connection if it hasn't been created.

I personally prefer to keep things out of the global scope, so I often use a pattern like this:

class DB{
    private static $conn;
    public static getConn(){
        if(empty(self::$conn)){
            self::$conn = new mysqli(...);
        }
        return self::$conn;
    }
}

Then you can use DB::getConn() to get the DB connection and it will only create the connection once, and you don't have to worry about accidentally naming a different variable $connection.

Maybe wrap that in a if(!class_exists("DB")){...} if you think you might include it more than once.

Efficiency

You are creating constants for your DB values. Essentially storing that data in memory even though you should only need it once. Because of this you also have to check if those constants exist before doing it again. This is an unnecessary check if you just use the values from the array instead of committing them to constants.

\$\endgroup\$
  • \$\begingroup\$ I think the singleton-like pattern as suggested here would be an improvement over original code. Make sure you have private/protected constructor for proper implementation of singleton pattern (or make the class abstract). The singleton can easily be abused though, if you start having DB::getConn() sprinkled throughout your code. Ideally you would instantiate this connection in your application bootstrap process and pass around the connection to classes/code that need it. \$\endgroup\$ – Mike Brant Jun 20 '17 at 14:09
  • \$\begingroup\$ I just saw that comment.. I fully disagree with passing the connection around. Db stuff should all be done In a designated db class unless you're writing procedural code. Passing around a db connection to every single class sounds like an awful (but not uncommon) idea. If you absolutely must tho, pass by reference. \$\endgroup\$ – iwrestledabearonce Jul 16 '17 at 3:47
  • \$\begingroup\$ I usually write a "Model" class that has a db connection and a class for each table or model that extends it. \$\endgroup\$ – iwrestledabearonce Jul 16 '17 at 3:47
2
\$\begingroup\$

Why the intermediate step of writing values to array? Why not:

define('DB_HOST', $db_ini['db_data_source_name']['host']);
define('DB_USER', $db_ini['db_general']['user']);
// etc.

Be careful with relative path with regards to including in other pieces of code. If being included by code in another directory (particularly at a different level in directory structure), this may break. Also, you won't be able to include this file more than once within any given script execution since you are defining constants.

Since you can only include this once per execution, do you really need to define constants at all?

$connection = mysqli_connect(
    $db_ini['db_data_source_name']['host'],
    $db_ini['db_general']['user'],
    $db_ini['db_general']['password'],
    $db_ini['db_data_source_name']['dbname']
);

I am just generally worried about your thinking of including this within other files "as needed". That tends to make me think that you don't have a good strategy around managing your application dependencies (such as database connection). Just sprinkling includes/requires throughout your code can really make your code fragile and hard to maintain. If you really want to take you application coding to another level, I would suggest familiarizing yourself with dependency injection, both in a philosophical sense as well as from a practical standpoint looking at typical PHP libraries that do this (Pimple, PHP-DI, or pretty much any popular framework has some level of dependency injection strategy).

Hopefully your ini file is separate from your codebase (i.e. not under revision control) to where you are storing your DB passwords in your code. If that is the case, consider taking the next step and have the ini file parsed in a different process such that this particular code is not responsible for reading configuration data from files that should perhaps should just be available in environmental variables.

\$\endgroup\$
  • 1
    \$\begingroup\$ he is checking if it's defined before doing it again. other than re-initializing the connection there is nothing wrong with including it as is several times. \$\endgroup\$ – iwrestledabearonce Jun 19 '17 at 22:44
  • 1
    \$\begingroup\$ your comments about the relative path are also incorrect. he is using the __DIR__ constant which will always make that path relative to that exact file. he can include that file from anywhere on the filesystem and it will work. \$\endgroup\$ – iwrestledabearonce Jun 19 '17 at 22:49
  • 1
    \$\begingroup\$ @Iwrestledabearonce WRT include path, yes __DIR__ is being used, but what if the file that is including this file is at a different directory level? Having a relative path the to INI file is going to be fragile unless you are sure that every file including this file is at the same level in the directory structure. I did update my content to make this more clear. I do echo your concern over spawning multiple unneeded database connections. This whole thing is a very flaky and fragile approach which really represents years-old approaches to getting DB connections vs. dependency injection. \$\endgroup\$ – Mike Brant Jun 20 '17 at 14:02
  • \$\begingroup\$ Thank you for your feedback @MIkeBrant. To ease your concern, I am simply learning PHP and I was following along some tutorials employing more or less this approach to establish their DB connection. It looked sketchy, so I posted it here to get some feedback. \$\endgroup\$ – HEADLESS_0NE Jun 25 '17 at 23:33
1
\$\begingroup\$

With this code, you effectively have no idea which credentials are used to create a connection.

So, after Mike Brant I would suggest to make this code less bloated and more explicit

<?php
$config_path = __DIR__ .  '/../config/database.ini';
$db_ini = parse_ini_file($config_path , true);

$connection = mysqli_connect(
    $db_ini['db_data_source_name']['host'],
    $db_ini['db_general']['user'],
    $db_ini['db_general']['password'],
    $db_ini['db_data_source_name']['dbname']
);

while some constants defined elsewhere should be discarded.

\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.