3
\$\begingroup\$

I want to preface this by saying I am fully aware PHP already has functions for handling .ini files, and there's already libraries out there that do what this project does. The purpose here wasn't to make something new, it was to be a personal learning experience in OOP (This is my first time making an object oriented project in PHP), documentation (Via PHPDoc), unit tests with PHPUnit, and building a composer project.

The project's separated into 3 classes - 'Controller', 'StaticController', and 'ObjectController'.

The source code if you'd rather view it on GitHub.

The main Controller class:

This is extended by StaticController and ObjectController below, and handles the actual reading, parsing, and editing of the INI files themselves.

<?php
/**
 * Master class that all other INI controllers in this package will extend.
 *
 * @author SierraKomodo
 * @license GPL3
 */


namespace SierraKomodo\INIController;


/**
 * Master class that all other INI controllers in this package will extend.
 *
 * This class handles all of the common, internal functions for the INI controller. You probably don't want to directly
 * use this class unless you're writing your own controller that extends it; Instead use one of the other classes that
 * extend this.
 *
 * @internal This class is designed for use by child classes that extend it, it is not meant to be used directly.
 * @package SierraKomodo\INIController
 * @used-by \SierraKomodo\INIController\ObjectController
 * @used-by \SierraKomodo\INIController\StaticController
 * @version 0.1.0-dev Currently in development; Not fully tested yet.
 */
class Controller
{
    /**
     * @var string $file Contains the filepath used in the last call to readFile()
     * @used-by \SierraKomodo\INIController\Controller::readFile()
     * @used-by \SierraKomodo\INIController\Controller::writeFile()
     */
    protected $file = '';
    /**
     * @var array $fileArray Array containing the output of parse_ini_file generated in the last call to readFile()
     * @used-by \SierraKomodo\INIController\Controller::readFile()
     * @used-by \SierraKomodo\INIController\Controller::writeFile()
     * @used-by \SierraKomodo\INIController\Controller::generateFile()
     */
    protected $fileArray = array();
    /**
     * @var string $fileContent Contains the output of the last call to generateFile()
     * @used-by \SierraKomodo\INIController\Controller::writeFile()
     * @used-by \SierraKomodo\INIController\Controller::generateFile()
     */
    protected $fileContent = '';


    /**
     * Reads the specified file.
     *
     * Reads the specified file, storing the file name in $this->file, output of parse_ini_file() in $this->fileArray,
     * and then running $this->generateFile() afterwards.
     *
     * @param string $parFile Full/relative path to the INI file to read. Defaults to $this->file
     * @return bool True on success
     * @throws \BadFunctionCallException If $parFile and $this->file are both null
     * @throws \RuntimeException If $this->file does not exist or is unreadable
     * @throws \RuntimeException If parse_ini_file() fails to parse $this->file
     * @uses \SierraKomodo\INIController\Controller::$file
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     * @uses \SierraKomodo\INIController\Controller::generateFile()
     */
    protected function readFile(string $parFile = null): bool
    {
        if ($parFile == null) {
            $parFile = $this->file;
        }

        // Input validation
        if ($parFile == null) {
            throw new \BadFunctionCallException("Parameter File must not be null if no filepath has previously been defined");
        }
        if (!is_file($parFile)) {
            throw new \RuntimeException("File '{$parFile}' does not exist or is inaccessable");
        }
        if (!is_readable($parFile)) {
            throw new \RuntimeException("File '{$parFile}' is not readable");
        }

        // Load and attempt to parse the INI file
        $array = parse_ini_file($parFile, true);
        if ($array === false) {
            throw new \RuntimeException("Failed to parse '{$parFile}' as an INI file");
        }

        // Set object variables
        $this->file      = $parFile;
        $this->fileArray = $array;
        $this->generateFile();

        // Indicate success
        return true;
    }


    /**
     * Writes data stored in $this->fileArray into $this->file
     *
     * @param string $parFile Full/relative path to the INI file to read. Defaults to $this->file
     * @return bool True on success
     * @throws \BadFunctionCallException If $parFile and $this->file are both null
     * @throws \RuntimeException If $this->file is not writable
     * @throws \RuntimeException If file_put_contents() fails to write to $this->file
     * @uses \SierraKomodo\INIController\Controller::$file
     * @uses \SierraKomodo\INIController\Controller::$fileContent
     * @uses \SierraKomodo\INIController\Controller::generateFile()
     */
    protected function writeFile(string $parFile = null): bool
    {
        if ($parFile == null) {
            $parFile = $this->file;
        }

        // Input validation
        if ($parFile == null) {
            throw new \BadFunctionCallException("Parameter File must not be null if no filepath has previously been defined");
        }
        if (file_exists($parFile) and !is_writable($parFile)) {
            throw new \RuntimeException("File '{$parFile}' is not writable");
        }

        // Ensure the latest changes to content are converted to the full content string
        $this->generateFile();

        // Write content string to file
        if (file_put_contents($parFile, $this->fileContent) === false) {
            throw new \RuntimeException("Failed to write content to '{$parFile}'");
        }

        // Indicate success
        return true;
    }


    /**
     * Generates a fully formatted INI string and stores it in $this->fileContent
     *
     * @return bool True on success
     * @uses    \SierraKomodo\INIController\Controller::$fileArray
     * @uses    \SierraKomodo\INIController\Controller::$fileContent
     * @used-by \SierraKomodo\INIController\Controller::readFile()
     * @used-by \SierraKomodo\INIController\Controller::writeFile()
     */
    protected function generateFile(): bool
    {
        $this->fileContent = '';

        // Convert array into formatted INI string
        foreach ($this->fileArray as $section => $block) {
            // Section header, on its own line
            $this->fileContent .= "[{$section}]\r\n";

            // Enter each key=value pair on separate lines
            foreach ($block as $key => $value) {
                $this->fileContent .= "{$key}={$value}\r\n";
            }

            // Blank lines between sections/at the end of the file
            $this->fileContent .= "\r\n";
        }

        // Indicate success
        return true;
    }
}

The StaticController class:

'StaticController' provides an interface to read/manipulate ini files without having to initialize an object. The thought here is this would be useful for quickly pulling specific data out of an INI file, whether it be the entire file, a specific section, or a specific key (See examples at the bottom)

<?php
/**
 * API class that allows static usage of the master INI controller
 *
 * @author SierraKomodo
 * @license GPL3
 */


namespace SierraKomodo\INIController;

use SierraKomodo\INIController\Controller as Controller;


/**
 * API class that allows static usage of the master INI controller
 *
 * Provides a static interface for direct manipulation of INI files. All methods in this class directly read from and
 * write to INI files.
 *
 * @api
 * @package SierraKomodo\INIController
 * @uses \SierraKomodo\INIController\Controller
 * @version 0.1.0-dev Currently in development; Not fully tested yet.
 */
class StaticController extends Controller
{
    /**
     * Adds/sets a specified key=value pair to an INI file.
     *
     * @param string $parFile
     * @param string $parSection
     * @param string $parKey
     * @param string $parValue
     * @param bool $parCreateFile If set to bool 'TRUE', will attempte to create $parFile if it doesn't already exist. Defaults to bool 'FALSE'
     * @return bool True on success
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     * @uses \SierraKomodo\INIController\Controller::readFile()
     * @uses \SierraKomodo\INIController\Controller::writeFile()
     */
    public static function set(
        string $parFile,
        string $parSection,
        string $parKey,
        string $parValue,
        bool $parCreateFile = false
    ): bool {
        $self = new static;

        // Create the file if it doesn't exist
        if (!file_exists($parFile) and $parCreateFile) {
            touch($parFile);
        }

        // Read the INI file
        $self->readFile($parFile);

        // Set the new key=value pair
        $self->fileArray[$parSection][$parKey] = $parValue;

        // Write to the file
        return $self->writeFile($parFile);
    }


    /**
     * Fetches the full INI file as a multi-level associative array
     *
     * @param string $parFile
     * @return array In the format of $array['Section']['Key'] = 'Value'
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     */
    public static function fetchFile(string $parFile): array
    {
        $self = new static;

        // Read the INI file
        $self->readFile($parFile);

        // Return the full array
        return $self->fileArray;
    }


    /**
     * Fetches an INI section from a file as an associative array
     *
     * @param string $parFile
     * @param string $parSection
     * @return array|bool In the format of $array['Key'] = 'Value'. Returns boolean 'FALSE' if no matching section was found
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     */
    public static function fetchSection(string $parFile, string $parSection)
    {
        $self = new static;

        // Read the INI file
        $self->readFile($parFile);

        // Return the section, or false if the section doesn't exist
        if (isset($self->fileArray[$parSection])) {
            return $self->fileArray[$parSection];
        } else {
            return false;
        }
    }


    /**
     * Fetches the value of a requested key=value pair from an INI file.
     *
     * @param string $parFile
     * @param string $parSection
     * @param string $parKey
     * @return bool|string The value of the requested key=value pair, OR boolean 'FALSE' if no matching entry was found.
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     * @uses \SierraKomodo\INIController\Controller::readFile()
     */
    public static function fetchKey(string $parFile, string $parSection, string $parKey)
    {
        $self = new static;

        // Read the INI file
        $self->readFile($parFile);

        // Return the key=value pair, or false if the entry doesn't exist
        if (isset($self->fileArray[$parSection][$parKey])) {
            return $self->fileArray[$parSection][$parKey];
        } else {
            return false;
        }
    }


    /**
     * Deletes a key=value pair from an INI file.
     *
     * @param string $parFile
     * @param string $parSection
     * @param string $parKey
     * @return bool True on success
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     * @uses \SierraKomodo\INIController\Controller::readFile()
     * @uses \SierraKomodo\INIController\Controller::writeFile()
     */
    public static function delete($parFile, $parSection, $parKey = null)
    {
        $self = new static;

        // Read the INI file
        $self->readFile($parFile);

        // If $parKey is null, remove the whole section. Otherwise, remove only a specific key.
        if ($parKey == null) {
            unset($self->fileArray[$parSection]);
        } else {
            unset($self->fileArray[$parSection][$parKey]);
        }

        // Write to the file
        return $self->writeFile($parFile);
    }
}

The ObjectController Class:

'ObjectController' allows you to load an ini file into memory and manipulate whats in memory without modifying the ini file itself until you want to save the changes that were made.

<?php
/**
 * API class that allows object/in memory usage of the master INI controller
 *
 * @author SierraKomodo
 * @license GPL3
 */


namespace SierraKomodo\INIController;

use SierraKomodo\INIController\Controller as Controller;


/**
 * API class that allows object/in memory usage of the master INI controller
 *
 * Provides an object for loading INI files into memory. Methods in this class will read from and write to data stored
 * in memory. INI files are only directly read or modified using the 'load' and 'save' methods.
 *
 * @api
 * @package SierraKomodo\INIController
 * @uses \SierraKomodo\INIController\Controller
 * @version 0.1.0-dev Currently in development; Not fully tested yet.
 */
class ObjectController extends Controller
{
    /**
     * ObjectController constructor. Alias of self::load(). Does nothing if no file parameter is provided.
     * @param string $parFile
     */
    public function __construct(string $parFile = null)
    {
        if (!empty($parFile)) {
            $this->load($parFile);
        }
    }


    /**
     * Loads the specified INI file into memory.
     *
     * @param string $parFile The INI file to load into memory. Defaults to the last INI file that was loaded into memory or written to.
     * @return bool True on success
     * @uses \SierraKomodo\INIController\Controller::readFile()
     */
    public function load(string $parFile = null): bool
    {
        // Read the given file. parent::readFile already handles storing all information we care about at this stage
        return $this->readFile($parFile);
    }


    /**
     * Saves the INI data currently in memory to the specified file.
     *
     * @param string $parFile The INI file to save data to. Defaults to the last INI file that was loaded into memory or written to.
     * @return bool True on success
     * @uses \SierraKomodo\INIController\Controller::writeFile()
     */
    public function save(string $parFile = null): bool
    {
        // Write to the file
        return $this->writeFile($parFile);
    }


    /**
     * Adds/sets a key=value pair in memory
     *
     * @param string $parSection
     * @param string $parKey
     * @param string $parValue
     * @return bool True on success
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     */
    public function set(string $parSection, string $parKey, string $parValue): bool
    {
        // Set the new value
        $this->fileArray[$parSection][$parKey] = $parValue;

        // Indicate success
        return true;
    }


    /**
     * Fetches the full INI file from memory as a multi-level associative array
     *
     * @return array In the format of $array['Section']['Key'] = 'Value'
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     */
    public function fetchFile(): array
    {
        return $this->fileArray;
    }


    /**
     * Fetches an INI section from memory as an associative array
     *
     * @param string $parSection
     * @return array|bool In the format of $array['Key'] = 'Value'. Returns boolean 'FALSE' if no matching section was found
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     */
    public function fetchSection(string $parSection)
    {
        if (isset($this->fileArray[$parSection])) {
            return $this->fileArray[$parSection];
        } else {
            return false;
        }
    }


    /**
     * Fetches a value from a key=value pair
     *
     * @param string $parSection
     * @param string $parKey
     * @return string|bool Returns the value of a key=value pair OR boolean 'FALSE' if no matching entry was found
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     */
    public function fetchKey(string $parSection, string $parKey)
    {
        if (isset($this->fileArray[$parSection][$parKey])) {
            return $this->fileArray[$parSection][$parKey];
        } else {
            return false;
        }
    }


    /**
     * Deletes a key=value pair from memory
     *
     * @param string $parSection
     * @param string $parKey
     * @return bool True on success
     * @uses \SierraKomodo\INIController\Controller::$fileArray
     */
    public function delete(string $parSection, string $parKey = null): bool
    {
        // If $parKey is null, delete the entire section. Otherwise, delete the specific key.
        if ($parKey == null) {
            unset($this->fileArray[$parSection]);
        } else {
            unset($this->fileArray[$parSection][$parKey]);
        }

        // Indicate success
        return true;
    }
}

Some example usage:

// Using StaticController to fetch database information from a config.ini file
use SierraKomodo\INIController\StaticController as INIController;

$fileName = __DIR__ . DIRECTORY_SEPARATOR . "config.ini";
$dbParams = INIController::fetchSection($fileName, 'Database');
$db       = new PDO("mysql:host={$dbParams['sHost']}; dbname={$dbParams['sDbName']}; charset={$dbParams['sCharset']}", $dbParams['sUser'], $dbParams['sPassword']);

// Using ObjectController to load user-specific configuration options
use SierraKomodo\INIController\ObjectController as INIController;

$fileName = PROJECT_ROOT . DIRECTORY_SEPARATOR . $userID . DIRECTORY_SEPARATOR . "config.ini";
$config   = new INIController($fileName);

I'm looking to get some feedback specifically on formatting, documentation, how useful the unit tests I wrote are (They're in the tests folder in the github repository), and ways I could improve on writing these classes.

\$\endgroup\$
1
\$\begingroup\$

Generally speaking, I don't find the naming of the library and classes as "Controller" to be appropriate. In modern programming, "controller" has it's own specific meaning that is unrelated to what this library does.

More specifically: - Controller is way too generic to convey meaning for what this class does - read and write files. - Use of Static or Object as prepended identifier to your class names to tell whether they are designed to be used in static or concrete context seems superfluous. Shouldn't the exposed class API make this readily apparent?


The inheritance model does not make much sense to me. It seems like you have made an arbitrary line of separation of responsibility between the classes. For example, the base class has responsibility for:

  • physical file operations, including validation of file-system-level read/write permissions
  • parsing ini content
  • formulating ini content

Whereas the extending classes (which have, in essence, the same functionality) are responsible for:

  • providing aliased methods for file read/write (why are these even necessary? they do nothing that inherited methods do not already do.)
  • managing individual key-value pairs in memory.
  • providing "getter" type methods for inherited properties.

Why the arbitrary split in responsibility between these classes. I would argue that if you got this library out of the business of performing file-level operations (for example leveraging something like passed SplFileObjects instead of just file path strings), that you could remove a lot of the validation code in this class around basic file read/write operations and get to the point where you have a single class that:

  • parses ini content from file (using injected file read/write dependency)
  • persists ini content to file (using injected file read/write dependency)
  • manages and exposes the in-memory configuration representation

Or maybe, if you find that to be too much code in a single class, split out the ini file read/write functionality into a class or trait that is injected or composed into the ini management class for its use - not as a base class to be extended upon.

I thing I like to suggest when making decisions about using composition vs. inheritance is - "what makes most sense in the real world?" In real world terms, is an INI configuration manager an "extension" of file systems operations, or does it really just need to be given the ability to operate against the file system to perform it's job. If the former, inheritance is likely the best answer, if the latter (which I believe to be the case here), then composition is likely your best bet.

Now, let me talk about static vs. concrete, as I don't really understand why you have a static class that does exactly the same thing as the concrete class.

Sure you may have different application contexts, which may need to operate against INI configuration data in different ways and your application may need to operate against multiple such files. I like that you are thinking about how an application might need to work with data from a number of these files in different contexts. But, let me ask you, why do both the static class AND the concrete class BOTH need to deal with all of these potential use cases?

Let's think about what a meaningful concretion truly is. I would suggest that the concrete object really only has meaning as a 1:1 representation of a single ini file (which seems to be the intent of the design). So why would it need a load() method? Would you not just pass it its file dependency in the constructor? Why would it need a save() method that takes file information as an argument? Shouldn't it already know the file it is associated with? (If this is to be able to rename/move or copy the file, perhaps there should be separate methods for that.)

On the flip side, from a static context, why do you need to do individual file level operations in that context? Your use of new static should be a strong indicator to you that you are really working with a concrete instance of an ini file object. From a static context, perhaps it really only makes sense to specify files to be loaded via a method like load() and then return concrete ini objects you can operate against. I suppose you could make individual file-level changes statically, but just be honest about what is really happening, you are providing a static convenience method with really instantiates an ini file object to operate against.

As such, I would suggest that you either just provide a static load() method on the concrete class which can be used to instantiate an ini config object, or if you find yourself truly wanting to expose more static functionality, than have a separate static class that focuses on instantiating or working with concrete ini objects, not on also being an object.

Think of the difference between a library and a book in real-world terms. That is analogous to what you are dealing with here. Your library (static class) may provide means for you to find a book (ini config object), check out a book, or even get excerpts from a book (in an electronic sense). But it doesn't need to have all the methods and properties that a book has. Similarly, an individual book doesn't need to know how to find other books in the library.

Here you have built two classes that do the same thing and I would challenge you to rethink that approach.


Also, even though I don't think Controller should be a class that is extended per above comments, I do want to note that should you have a case in the future where you build a base class like this that has no public properties, no public methods, and is clearly not intended to ever be instantiated, you really should make it an abstract class.


Some more code-specific comments:

protected $fileArray

This seems like poorly named property. I would expect this to contain an array of files, not an array of ini configurations as it seems to.


protected $fileContent = '';

Why do you ever need to store this? This is only meaningful right before writing a file correct? So why to you need to store the results of generateFile() at all? I would just generate the content and then write as single step (though I still think it make sense the separate the logic into different methods.


protected function readFile(string $parFile = null): bool
{
    if ($parFile == null) {
        $parFile = $this->file;
    }

Why would readFile ever accept null as argument? What if caller did something like $iniConfig = new ObjectController(); $iniConfig->readFile(); You are making it too easy for application to be put into exceptional state here. You should be biased towards being explicit about your dependencies vs. providing multiple ways to to inject dependencies (i.e. via constructor vs. via method call).

Consider using is_null() for null comparison. When doing comparisons you should also be biased towards explicit comparison vs. loose comparison. You seem to do this interchangeably throughout your code, which can lead to unexpected behavior.


    if ($parFile == null) {
        throw new \BadFunctionCallException("Parameter File must not be null if no filepath has previously been defined");
    }

Would \InvalidArgumentException be better suited here?


$array = parse_ini_file($parFile, true);

More meaningful variable name perhaps?


    // Set object variables
    $this->file      = $parFile;
    $this->fileArray = $array;

Some may consider this slightly odd, style-wise.


protected function generateFile(): bool

This doesn't really generate a file. generateIniContent()?


public static function set(

Consider more meaningful and specific name. setConfigToFile()?


fetchFile(string $parFile)

getConfigFromFile()? I will skip similar comments about other method names throughout classes, I generally think there is room for improvement to more meaningful/specific names. I would say that perhaps the most problematic name is delete() Does this delete the whole file? (no it does not, but you wouldn't know without reading into the method signature or documentation)


use SierraKomodo\INIController\Controller as Controller;

This is unnecessary in both extending class files. These files all share common namespace, so you already would reference the base class as Controller without this statement.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback, I'll go through this and the project; Should I edit my original question here or post something new when I've finished going through all of this? It may take a few days because of work, and this being a side project. \$\endgroup\$ – SierraKomodo May 9 '17 at 2:03
  • \$\begingroup\$ I do want to ask really quick, since it wasn't touched on: Were there any issues with the commenting/in file documentation? Anything I may have been missing, or had in there that wasn't necessary? \$\endgroup\$ – SierraKomodo May 9 '17 at 2:15
  • 1
    \$\begingroup\$ @SierraKomodo you should open a new question if you would like an iterative review. I think the Doc blocks are fine, though probably a little more verbose with uses and used-by annotatuond which I don't typically see commonly used. \$\endgroup\$ – Mike Brant May 9 '17 at 13:23

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.