4
\$\begingroup\$

I'm working on a config class for PHP, where I can easily bring in config options. Any improvements/suggestions are appreciated.

Example usage:

echo Config::get('database.host');
// Returns: localhost

Example config file:

<?php

return array(
    'host' => 'localhost',
    'username' => 'root',
    'password' => '',
    'database' => 'example_database'
);

The class:

<?php

class Config {

    /**
     * All of the items from the config file that is loaded
     *
     * @var array
     */
    public static $items = array();

    /**
     * Loads the config file specified and sets $items to the array
     *
     * @param   string  $filepath
     * @return  void
     */
    public static function load($filepath)
    {
        static::$items = include('config/' . $filepath . '.php');
    }

    /**
     * Searches the $items array and returns the item
     *
     * @param   string  $item
     * @return  string
     */
    public static function get($key = null)
    {
        $input = explode('.', $key);
        $filepath = $input[0];
        unset($input[0]);
        $key = implode('.', $input);

        static::load($filepath);

        if ( ! empty($key))
        {
            return static::$items[$key];
        }

        return static::$items;
    }

}

I should also add that this doesn't seem to work with multidimensional arrays - I'm working on this. If anyone has a solution, I'd appreciate it.

\$\endgroup\$
3
\$\begingroup\$

You can continue to use a class if you want, but I think making a config class is overkill. Just a plain config file with constants should be good enough. It has less overhead and less abstraction. As for loading a specific config file, you can use a helper function to simulate what you have here, or just manually include it. Not everything has to be in a class.

echo Config::get( 'database.host' );
//VS
echo DB_HOST;

$config->load( 'config' );
//VS
config( 'config' );//helper function
//OR
include 'config/config.php';

I think there's something wrong with your load() method by the way. You are resetting the array each time you load a new config file. I believe it should look something like so.

static::$items[] = include "config/$filepath.php";

I'm not sure I really agree with setting $items to static. I understand you aren't instantiating the class, but I think you should, and then just make this a normal property. Typically when I think of static properties, I think of properties that aren't supposed to change, but you are adding to it, which seems contradictory. Maybe I'm wrong here, but this just seems odd to me. Of course I really just dislike the static keyword altogether.

You can set an array element to a variable and unset it all at the same time by using array_shift(). For example.

$filepath = array_shift( $input );

It might be worth profiling that explode/implode task and comparing it to a substr version. Assuming the array version is faster, then I would suggest changing the first delimiter to something other than a period since that's the only one you care about. Maybe a forward slash or underscore instead? It would make needing to implode the array unnecessary.

$input = explode( '/', $key);
$filepath = array_shift( $input );
$key = strval( $input );

PHP empty() should really only be used when checking arrays. A string can be determined just by querying it. For example:

if ( $key ) {//not empty
if( ! $key ) {//empty

Finally, I would think that the "default" return value for your getter would be NULL or FALSE. Why return the entire configuration array? This could be confusing, especially since you mentioned using multidimensional arrays. Speaking of which, what about this doesn't work for them? The config file? The getter? What exactly is happening, and what are you expecting? Besides the things I already mentioned, I don't see anything that would cause an issue.

Edit to clarify last comment:

Early check and return ensures less overhead and better legibility.

public static function get( $key = null ) {
    if( ! $key ) {
        return static::$items;
    }
    //etc...

I don't condone the use of variable-variables, but its the only way I can foresee doing this. Unless you can put all of those arrays into another array, then you can access them as associative arrays.

$input = explode( '.', $key );
$filepath = array_shift( $input );
if( count( $input ) > 1 ) {
    $array = array_shift( $input );
    $key = $$array[ strval( $input ) ];
}
\$\endgroup\$
  • \$\begingroup\$ Resetting the array limits the extensibility, but ok. If you want to return entire array, then you should do an early check and return if your parameter is empty. Don't implode the array if you want to use multidimensional. Check the length and if > 1 then use variable variables. However, this will make your code more difficult to read. \$\endgroup\$ – mseancole Sep 5 '12 at 17:58
  • \$\begingroup\$ @Bethesda: edited answer to clarify that last comment \$\endgroup\$ – mseancole Sep 5 '12 at 18:21
  • \$\begingroup\$ May I ask why I was downvoted? if there is something wrong with my post let me know and I will fix it. Please don't just downvote with no explanation. \$\endgroup\$ – mseancole Sep 5 '12 at 20:33
  • \$\begingroup\$ +1 to offset the seemingly arbitrary downvote - I think this is a good response to the question. \$\endgroup\$ – cori Sep 5 '12 at 20:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy