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\$

    1 Answer 1

    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\$
    4
    • \$\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
      CommentedSep 5, 2012 at 17:58
    • \$\begingroup\$@Bethesda: edited answer to clarify that last comment\$\endgroup\$
      – mseancole
      CommentedSep 5, 2012 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
      CommentedSep 5, 2012 at 20:33
    • \$\begingroup\$+1 to offset the seemingly arbitrary downvote - I think this is a good response to the question.\$\endgroup\$
      – cori
      CommentedSep 5, 2012 at 20:43