7
\$\begingroup\$

I'm writing my own framework (purely for learning purposes, SPL, OOP, patterns, etc) and have written a class to manage config data throughout the framework (not front end/view, just the core framework).

Example of config data:

The framework requires a specific PHP version to run correctly, and that "required PHP version" is stored in config settings. That config data is needed in a class which checks if the running PHP version is not lower than the required PHP version.

I've read about various options for configs (for tens of hours over weeks), and the main ones seem to be:

Constants; ini files; an array in an included file; static classes.
They all have pros and cons, but the main con is they push things into global, which I'm really trying to avoid.

They also don't lend themselves to dependency injection, which I'm really trying to achieve.

I've made my own class for managing this data, and would like it reviewed, please.

Class Description

The config class is instantiated in the bootloader file before any classes which need it, so the object variable is ready to be used in any class to call the method.

Note: I'll be changing how this class is instantiated when I get around to Dependency Injection Container, so please review based on that, and ignore that the object variable for this class is global - it eventually won't be.

The config class' method getConfig() is used in the __construct (or getter method etc) in any class wanting config data. The getConfig() accepts parameters so specific config data can be retrieved.

The class

NOTE: In code comments and later text, by "sub array" I mean one of the single sub arrays within the config array - such as the "php" array, or "router" array.

class coreConfig {

  private $configData;
  private $returnData;


  private function setConfigData($key, $value = '') {

    $configData = array (

      'php' => array(
        'current' => PHP_VERSION_ID, 
        'required' => '50400',
        'test-index' => 'php test data',
      ),

      'framework' => array(
        'name' => 'frameworky',
        'version' => '1.0',
        'date' => '01/01/2015',
        'email' => 'nothing@nowhere.com',
      ),

      'router' => array(
        'allowedchars' => 'A-z',
        'url-prefix' => 'http://',
        'test-index' => 'router test data',
      ),

    );


    if (!isset($configData[$key]) 
      || (!empty($value) && !isset($configData[$key][$value]))) {      
      return false;

    }elseif (empty($value)) {
      return $configData[$key];

    } else {
      return $configData[$key][$value];
    }

  }


  public function getConfig($request = array()) {

    $this->returnData = '';

    // No data requested
    if (empty($request)) {
      $this->returnData = false;

    } elseif (!is_array($request)) {

        // Returns 1 single sub array
        $this->returnData[$request] = $this->setConfigData($request);

    } else {


      // Array(s) requested
      foreach ($request as $key => $value) {

        if (!is_array($value)) {
          // Returns entire sub array
          $this->returnData[$value] = $this->setConfigData($value);

        } else {

          // Specific sub array value(s) requested
          foreach ($value as $subKey => $subValue) {

            // 3rd level sub arrays not expected
            if (is_array($subValue)) {
              $this->returnData[$subKey] = false;

            } else {

              // Returns sub array value
              $this->returnData[$key][$subValue] = $this->setConfigData($key, $subValue);

            }

          }

        }

      }


    }

    return $this->returnData;

  }


}

Options

Note: By "key" I mean send in a key which exists in the config array.

The options to send to the getConfig() method are:

  1. A single key: Returns an entire single sub array
  2. An array of single keys: Returns multiple entire single sub arrays
  3. An array with a key and value(s), or multiple arrays with a key and value(s): Returns specific value(s) from the sub array(s) for each specified key
  4. Any mixture of 2 and 3 can be sent in one call to the method

Here are some example usages based on the above options. The results are from print_r():

$obj = new coreConfig;

//*Option 1*:  
$config1 = $obj->getConfig('php');
// Returns single entire sub array:
Array (
  [php] => Array (
    [current] => 50609 [required] => 50400 [test-index] => php test data 
  )
)

//*Option 2*:  
$config2 = $obj->getConfig(array('php', 'router'));
// Returns 2 entire sub arrays:
Array (
  [php] => Array (
    [current] => 50609 [required] => 50400 [test-index] => php test data 
  )
    [router] => Array (
      [allowedchars] => A-z [url-prefix] => http:// 
      [test-index] => router test data 
    )
  )

//*Option 3*:  
$config3 = $obj->getConfig(array('php' => array('current', 'test-index')));
// Returns partial sub array:
Array ( 
  [php] => Array (
    [current] => 50609 [test-index] => php test data
    )
  ) 

//*Option 4*:  
$config4 = $obj->getConfig(array(
    'php' => array('current', 'test-index'), 
    'router'
  )
);
// Returns partial sub array, and entire sub array:
Array ( 
  [php] => Array ( 
    [current] => 50609 [test-index] => php test data
  )
  [router] => Array (
    [allowedchars] => A-z [url-prefix] => http:// 
    [test-index] => router test data
    )
  )

My Questions

  1. Is my general approach sane and decent for retrieving system config data within a framework?
  2. Is it acceptable using the public (get) method to call a private (set) method (same class) to set the config array and retrieve data?
    e.g. $this->returnData[$value] = $this->setConfigData($value);
  3. Unit testing - I have no experience, but know of its existence and some coding practices to following to cater for unit testing. With my class, the config data cannot be mocked in a test, is that a problem for unit testing?
  4. Does the code set the array in setConfigData() every time data is requested by getConfig()? If so, would this be resolved/improved if I made the array an object - i.e. $this-configData? In trying to potentially re-use objects
  5. Is the setConfigData() method ok having those if/else? It's like that because it allows the setConfigData() method to return only the required data, whereas this functionality being in the getConfig() method meant getting the entire array to check if keys existed etc.
  6. My main niggle is to add config settings for the framework, one has to edit an array within a class method. This seems.. fiddly for config, but there are also benefits of it being in a class (DI, DIC, etc). Do you see this as a major issue as a developer adding config data in a class method array?
  7. Is it worthwhile having the class take in so many options? Such as full config array request, partial, and a mixture, etc, all in one method call.

    Or would it be better to instead have the class only accept a single value and return an entire single sub array for each method call, and then call the method as many times as needed?

    //*Currently*:  
    $allConfigData = $obj->getConfig('everything-I-could-possibly-need')
    
    //*Alternative with class only accepting and returning one full sub array*:  
    $configData1 = $obj->getConfig('data-I-need');
    $configData2 = $obj->getConfig('more-data-I-need');
    $configData3 = $obj->getConfig('additional-data');
    

    I know this is a bit broad, but I'm asking you as a seasoned developer if this could potentially be useful. Currently I do not need more than one config sub array in any class, but my framework is expanding so potentially this could become useful.

\$\endgroup\$
  • \$\begingroup\$ Don't reimplement the wheel; Build a better one. \$\endgroup\$ – r3wt Jul 13 '15 at 8:13
  • \$\begingroup\$ As someone who needed to roll their own config once, I too wanted to DI it for reasons. The fact is its global data for a reason. The biggest pitfall with a DI-able Config is keeping things in sync when one part of your program needs to save changes back to the config. \$\endgroup\$ – RubberDuck Jul 13 '15 at 11:02
  • \$\begingroup\$ @r3wt I understand what you are saying, but do you mean I have built a better one, or I should? I'm not asking for a full review, just confirmation of that :) \$\endgroup\$ – James Jul 13 '15 at 14:42
  • 1
    \$\begingroup\$ @RubberDuck There are patterns to combat that (as far as I can tell with my limited knowledge). I have come across issues like you described, but there are ways around them. And losing DI and falling back to "let's put stuff in global because it's easier" is not how I want to do this. I want to do it like "heck, this is a tricky issue, but once resolved this will be a much better solution" ;) \$\endgroup\$ – James Jul 13 '15 at 14:44
4
+100
\$\begingroup\$

Introduction

First of all: well done you for writing your own framework! (Just don't forget to throw it away once you feel you've learned enough). Configuration is, architecturally speaking, much more solid than relying on convention, so you score points there to!

Some minor issues

  • Composer is a dependency management tool but it has some added functionality like a class autoloader and requirement enforcement for PHP versions and/or PHP modules. Especially if your framework will consist of separate libraries it might be useful to leverage the power of Composer to allow users to defined which parts they would/wouldn't want to use instead of having to do this through custom configuration. I say "might" because it will most likely depend on how complex your configuration is/will become and how well versed you (and/or users of your code) are in Composer usage.
    As for the specific example of the framework requiring a certain PHP version, I would use a Composer and simply state a PHP version as a Platform package.
  • Configuration should definitely go in a text file somewhere but you are correct that you will need a code wrapper for easy access. To avoid abuse and to keep things simple, using a format that can not contain logic (like XML, INI, JSON or YAML) is preferable over using code, even if it is "just" a PHP array. The text values can be parsed and passed to a configuration object which is then given to the framework as configuration. As you observed in the comments, this would effectively replace the setConfigData method with a (preferably immutable) data object.
  • I would also suggest you might want to have separate objects for different kinds of data instead of having everything cobbled together. Instead of splitting the array into single sub arrays I would opt for having separate specific config objects per responsibility (php/router/etc). These would not have to be separate classes but they could be if you feel this improves your design. Whether or not this is wise (as opposed to your suggestion in the comments of having one object with specific functions per responsibility) depends mostly on how the config is going to be used elsewhere in the framework.
  • You may also want to take a look at "immutable objects" (basically a class with no setters, with data being injected through constructor parameters).
  • The convention is for classnames to start with an uppercase letter.
  • Using var_export instead of print_r yields a more "natural" output format. That is to say, a format that read more like natural PHP and thus requires less of a mind-switch when reading.

Comment in regard to the code

Looking through the provided code, a view points of concerns:

  • The function setConfigData doesn't actually do any "setting". It just returns a value. Also, the $value parameter would be better of being called $subKey or something similar, as it currently gives the illusion that $value will be assigned to $key. The logic could also be more elegant but that's a matter for another review.
  • The local variable $configData would be better of as a class property.
  • The class property $returnData isn't needed, just use a local variable in the getConfig() function instead.

Answers to your questions

1. I find it hard to judge either "Sane" or "Decent". I think "Clean" is more important: single responsibility, clear separation of concerns, no side-effects, etc. Reading Clean Code by Robert C. Martin can be invaluable there.

2. If the set method actually set anything this would be less clean as it causes side-effects. Since it is really only another getter, there's no real problem.

3. Yes. This can become problematic. The best thing for it is simply to learn how to write unit tests (it honestly isn't all that difficult) and having tests with your code will teach you how to create code that is easy to test.

4. Yes, the array in setConfigData() is set again every time data is requested by getConfig. This is because it is being used from local scope and not from the class scope since it does not have a $this-> in front of the variable name. You don't need to make the array an object to resolve this, just load it from class scope using $this->configData.

5. OK or not is mostly a point of view. Most folks will probably think it is fine. In my personal taste the conditions could be cleaner/less muddled.

6. Yes, MAJOR issues. Configuration should live outside of the class. It can be injected into an object at the point where it is being instantiated. You really don't want for me to edit the framework code when I want an application I build on top of it to alter its behaviour!

7. Personally I would not put all of that functionality into one method. I'd create separate methods for separate scenarios to keep the class signature (or interface) simple and easy to understand.

In closing

If you feel inclined to look at this from other angles, you should probably also take a look at other data structures that can/could be used for configuration other than an array. For instance, the ARC config component uses a tree structure.

If any of my comments seem unclear or need more elaboration, don't hesitate to ask. I will update my answer as needed.

Furthermore, as you stated that building a framework was mostly about the educational values, I think you will agree that something as essential as configuration has a lot of lessons in it. You seem to be learning them quite well.

Keep it up!

\$\endgroup\$
  • \$\begingroup\$ Thanks for taking the time to read and answer. Composer: I will read more about it, but at first glance this seems like a library management tool rather than internal code dependency management. I will likely have libraries, but they will be coded my myself, and the framework will be able to manage loading them or not based on user preference. Maybe I'll "see the light" when I read more :) \$\endgroup\$ – James Jul 13 '15 at 14:50
  • \$\begingroup\$ "Configuration should definitely go in a text file somewhere" should this be a file with just an array in it (which would end up being global if not careful)? Or a class with a getter with the array in it? The latter would essentially be removing my setConfigData method and placing it in it's own class. \$\endgroup\$ – James Jul 13 '15 at 14:52
  • \$\begingroup\$ "have separate objects for different kinds of data instead of having everything cobbled together" So split the array into single sub arrays? So have php array in it's own array and 'router' in it's own array? Do you mean separate arrays in the same method, or each set of config (php/router/etc) in an entirely new file and class of it's own? Or same class different methods perhaps? With getter methods being named as per the config data they hold - public method getPhpConfig() public method getRouterConfig() etc \$\endgroup\$ – James Jul 13 '15 at 14:55
  • \$\begingroup\$ I'm thinking about having separate files for each config type, and each in their own class with a getter method (just an array in the getter). Then the "config getter class" which gets the data can call for the config objects via SPL_autoload_register and just call on the getter (like it is now, just reaching out into a new class/method rather than method in the same class). There are pros and cons to this, and all other approaches. I'll see how this pans out. As it's for learning, and a small framework, refactoring isn't too bad. \$\endgroup\$ – James Jul 15 '15 at 9:27

Your Answer

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

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