10
\$\begingroup\$

A few days ago I asked this question and I've got an answer to work with interfaces. I've tweaked around with this to get myself a INI parser class.

<?php

<?php

require "Parser.php";

class IniParser implements Parser
{

    private $data = [];

    /**
     * Parses the data from a configuration file and stores it into a array.
     *
     * @param $file string The file where the configuration / credentials / settings exist.
     * @param string $section The section within the configuration file, this is optional
     * @throws Exception
     */
    public function parse ($file, $section = "") {
        if (!$this->isValidated('file', $file)) {
            throw new Exception("File seems not to work..");
        }
        $credentials = parse_ini_file($file, true);
        foreach ($credentials as $key => $values) {
            if (!$this->isValidated('values', $credentials, $section)) {
                throw new Exception("Values or section doesn't exist");
            } elseif ($key !== $section) {
                continue;
            }
            $this->data = $values;
        }
    }

    /**
     * This function returns the value from a key inside a array.
     *
     * @param $key mixed Key of the array
     *
     * @throws Exception
     * @return mixed Value of the selected key by the parameter
     */
    public function get ($key) {
        if (!isset($this->data[$key])) {
            throw new Exception("Value doesn't exist in key: " . $key);
        }
        return $this->data[$key];
    }

    /**
     * @param $type
     * @param $data
     * @param string $section
     * @return bool
     *
     * note: when validating the values, were validating an array.
     */
    public function isValidated ($type, $data, $section = "") {
        switch ($type) {
            case "file":
                return file_exists($data) || is_file($data);
            case "values":
                //1. Check if $data is an array
                //2. Section is set
                return is_array($data) || isset($data[$section]);
        }
        return false;
    }
}

When calling the parse() method in this class you get a specific .ini file (first argument) and parses the values within a specific section.

When calling the get() method in this class, you get the value of the chosen key.

This are the required methods for it:

<?php

interface Parser
{
    public function parse($file, $section);

    public function get($key);

}

For my connection, I use this class:

<?php
class Connection
{
    private $state;
    private $credentials;

    public function __construct(Parser $credentials) {
        $this->credentials = $credentials;
    }

    public function initialize() {
        if ($this->isInitialized()) {
            throw new PDOException("Database connection seems already to be open");
        }
        $this->state = new PDO($this->credentials->get('engine') . ":host=" . $this->credentials->get('host') . ";dbname=" . $this->credentials->get('dbname'), $this->credentials->get('username'), $this->credentials->get('password'));
    }

    public function terminate() {
        if (!$this->isInitialized()) {
            throw new PDOException("Connection doesn't seem to be open");
        }
        $this->state = null;
    }

    public function isInitialized() {
        return $this->state instanceof PDO;
    }
}

To create a new connection, I use this piece of code:

<?php
/**
 * User: Bas
 * Date: 6-10-2014
 * Time: 21:24
 */

require "INIParser.php";
require "Connection.php";

$credentials = new IniParser();
$credentials->parse("configuration.ini", "Connection");

try {
    $con = new Connection($credentials);
    $con->initialize();

} catch (Exception $e) {
    echo $e->getMessage();
}
\$\endgroup\$
3
+50
\$\begingroup\$

Parser

The code as written solves the small problem of configuring a PDO connection via an INI file, but it won't scale to handle other connection types. As tim pointed out, IniParser barely abstracts the act of parsing the file. Its validation is already handled by parse_ini_file: an unreadable file or malformed sections will result in a false return value.

Worse, pulling values from another section requires reparsing the entire file. Also, while it looks like omitting the section will allow you to access the top-level values, I suspect it won't actually work because isset($data['']) will not pass.

My first step would be to design an INI file abstraction with these features:

  • Separate the parser from the result.
  • Expose all sections and top-level values with a single parse.
  • Ensure that accessing a key as a section really is a section.
  • Optional: full file validation with good clear messages.

One thing to consider first is if you want to be limited to INI files. Other configuration file formats allow sections to nest to arbitrary depth. For an implementation that supports this, take a look at Zend Framework's Config module. For now, let's stick with your current requirements.

IniParser

The parser should handle the optional file validation (does it exist? is it a file? is it readable? is it well-formed?), parse the file, and return the full data array wrapped in a Config object to provide the remaining features.

$config = IniFile::parse('application.ini');

class IniFile {
    public static function parse($file) {
        if (($config = parse_ini_file($file, true)) === false) {
            throw new InvalidArgumentException(
                    "$file is not a valid INI file");
        }
        else {
            return new Config($config);
        }
    }
}

If you want to perform full file validation, I'd suggest extracting that functionality to a utility class. This is overkill in this instance, but it will give you more ideas on how to separate responsibilities so that each method does one thing and each class has a single responsibility.

Note: is_readable and is_file both test if the file exists, but performing all checks ensures that you get the correct error message when things go awry.

class Files {
    public static function assertReadableFile($path) {
        self::assertExists($path);
        self::assertFile($path);
        self::assertReadable($path);
    }

    public static function assertExists($path) {
        if (!is_file($path)) {
            throw new InvalidArgumentException(
                    "'$path' does not exist");
        }
    }

    public static function assertFile($path) {
        if (!is_file($path)) {
            throw new InvalidArgumentException(
                    "'$path' is not a file");
        }
    }

    public static function assertFile($path) {
        if (!is_readable($path)) {
            throw new InvalidArgumentException(
                    "'$path' is not readable");
        }
    }
}

Config

The Config object gives access to sections and top-level values. It makes sense to expose sections using a section or getSection method.

$db = $config->section('Connection');

But you have several options for exposing the values.

  1. Use a named method such as value or get as you've done above:

    $engine = $db->get('engine');
    
  2. Implement ArrayAccess to allow array notation.

    $engine = $db['engine'];
    
  3. Provide magic getters (__get and __isset) for property-style access.

    $engine = $db->engine;
    

Though the last option requires wrapping each returned section in a Config, it provides nested sections as well. As you prefer the get method, let's go with that.

/**
 * Wraps a one- or two-dimensional configuration array
 * and exposes the second dimensional values as sections.
 */
class Config {
    /**
     * @var mixed[] May contain one level of sub-arrays for sections.
     */
    private $data;

    /**
     * Stores the configuration data.
     *
     * @param mixed[] $data
     */
    public function __construct(array $data) {
        $this->data = $data;
    }

    /**
     * Returns the named section sub-array.
     *
     * @param string $name unique name of the section
     * @return mixed[] map of configuration key/value pairs
     * @throws InvalidArgumentException if not present or not a section
     */
    public function section($name) {
        $this->assertPresent($name);
        $section = $this->data[$name];
        if (!is_array($section)) {
            throw new InvalidArgumentException("'$name' is not a section");
        }
        return $section;
    }

    /**
     * Returns the named property value.
     *
     * @param string $name unique name of the 
     * @return mixed the property's value
     * @throws InvalidArgumentException if not present or not a value
     */
    public function get($name) {
        $this->assertPresent($name);
        $value = $this->data[$name];
        if (is_array($value)) {
            throw new InvalidArgumentException("'$name' is not a value");
        }
        return $value;
    }

    /**
     * Fails if the named value/section doesn't exist.
     *
     * @param string $name unique name of the value/section
     * @throws InvalidArgumentException if not present
     */
    private function assertPresent($name) {
        if (!array_key_exists($this->data[$name])) {
            throw new InvalidArgumentException("Key '$name' does not exist");
        }
    }
}

Notice that I chose to throw exceptions for missing sections/values or when calling the wrong method on each. You could also accept a default value for get and have section return an empty array when it doesn't exist.

Configuration

By passing the Parser or Config to the Connection class directly, you're coupling these classes unnecessarily. The Connection should be given only the properties it needs; it shouldn't know how to grab them from a configuration object unless it's a custom object with named getters or a simple array with known keys (see next section below).

The simplest form of configuring an object is passing the values to the constructor.

class PdoConnection {
    private $engine;
    private $host;
    ...

    public function __construct(
        $engine, $host, $dbName, $user, $password
    ) {
        $this->engine = $engine;
        $this->host = $host;
        ...
    }
}

The application bootstrap is the place to tie the configuration source to the resulting objects. The Factory and Builder patterns come in handy here, though they certainly aren't necessary for small projects.

This separation will let you write simple code for tying the objects together.

$config = IniFile::parse('application.ini');
$db = $config->section('Connection');
$conn = new PdoConnection(
        $db['engine'], $db['host'], $db['dbname'], $db['user'], $db['password']);

Plus it gives you the flexibility to reuse these components or configure other components.

$logging = $config->section('Logging');
$log = new LogFile($logging['file'], $logging['level'], $logging['pattern']);

Alternate Forms

Update: This section shows a couple variations of configuring service objects as compared with the above method of passing individual parameters to the constructor.

For components with complex configuration you can either use a builder

$conn = PdoConnectionBuilder::create()
        ->engine($db['engine'])
        ->host($db['host'])
        ->database($db['dbname'])
        ->user($db['user'])
        ->password($db['password'])
        ->build();

or create a configuration interface with implementations backed by raw values, an array, a PdoConfig object, or anything else and pass it to the service being configured. But it should definitely not accept a generic IniParser or the Config it produces.

$conn = new PdoConnection(new PdoIniConfig($config->section('Connection')));

The builder above can use any of these styles to construct the final object.

\$\endgroup\$
  • \$\begingroup\$ Wow, thanks for you time, but there is a little problem. I'm not this experienced with PHP yet, I only reconise the :: while a method is static, but for the rest.... Could you explain a little better? \$\endgroup\$ – Bas Oct 12 '14 at 15:08
  • \$\begingroup\$ Can you ask some specific questions? Explaining that whole thing differently may not help. What part is the most confusing? And yes, :: is for accessing static methods and properties. You could just as easily make it non-static: `$config = new IniParser('application.ini'); \$\endgroup\$ – David Harkness Oct 12 '14 at 20:36
  • \$\begingroup\$ The PdoConnection() class you made, what kind of argument(s) has this? Only one (CredentialsParser) or just all credentials seperatly. Further, i personally like the ->get() method, it gives me more structure. \$\endgroup\$ – Bas Oct 13 '14 at 11:29
  • \$\begingroup\$ And how do you make it so that you can seperate the parse() function in the IniParser class from the getSection()? \$\endgroup\$ – Bas Oct 13 '14 at 11:30
  • \$\begingroup\$ @Bas I added code examples. Does that help? \$\endgroup\$ – David Harkness Oct 13 '14 at 20:17
5
\$\begingroup\$

Structure

Your IniParser should be the only part of your code that is aware of how the ini file looks internally. That's the whole point of the interface: With it, you can parse different kinds of files (or even retrieve the data from something else). If the file structure changes at some point, you should only ever have to change your IniParser class, nothing else.

But right now, that isn't the case. Your Connection also needs to know how your file looks. To fix this, I would add explicit getters instead of the generic getter you have right now. So you should add getEngine, getHost, etc to your class. This has the added benefit that you can validate them (see that they exist, etc) as soon as you parse the file, instead of when you are using the credentials.

Then of course your naming doesn't make that much sense anymore. So I would rename Parser to ICredentialsParser and IniParser to IniCredentialsParser.

You might also think about passing the names of keys to it as well instead of hardcoding them inside the parser.

If you do all this, your Connection class is decoupled from the ini file itself, and also, your IniParser gets more meaning (before, it was more or less only a wrapper for parse_ini_file).

parse and continue

If possible, I would avoid continue. In some cases it makes sense, but most of the time, it makes code harder to read. Your code could look like this:

        if (!$this->isValidated('values', $credentials, $section)) {
            throw new Exception("Values or section doesn't exist");
        } elseif ($key == $section) {
            $this->data = $values;
        }

Misc

  • your isValidated('values', $credentials, $section) call is inside a loop, but doesn't use the loop variables, so you should move it out of the loop.
  • I would create separate isValidated functions, as they don't share any common code (the way you do it just complicates things). You can name them isValidFile and hasSection.
  • naming: most of the time, I see open and close for the operations you are naming initialize and terminate. Your names are not bad, just unconventional. I would go with the general convention.
  • naming: isValidated would sound a bit better as isValid.
  • isValidated should probably be private.
  • in PHPDocs, it's always good to write why an exception is thrown.
\$\endgroup\$
  • \$\begingroup\$ I'm sorry for my late reply, but im going to read it right now :) \$\endgroup\$ – Bas Oct 11 '14 at 11:44
  • \$\begingroup\$ What you said about having the getHost() and getEngine() methods. Would'nt that be better to have that inside of the Service interface? Where a connection is connecting to? \$\endgroup\$ – Bas Oct 11 '14 at 11:45
  • \$\begingroup\$ @Bas what Service interface? \$\endgroup\$ – tim Oct 11 '14 at 12:02
  • \$\begingroup\$ Oh, haha yeah sorry. I updated my code... I created an interface where i can connect to other connection datasources, such as PDO or MySQLi. But nevermind, srry. \$\endgroup\$ – Bas Oct 11 '14 at 12:04

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.