Comments for "Config_Lite"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments
  • Till Klampaeckel  [2010-09-09 18:02 UTC]


    I'd really like to see this in pear. Extra bonus points for tests. And here is some feedback:

    1) Use exceptions:
    Config_Lite_Exception extends PEAR_Exception {}

    ... and then "throw new Config_Lite_Exception('message');" (vs. the aged PEAR_Error).

    2) Only require PEAR_Exception

    3) Improve error handling (e.g. the return from parse_ini_file, if the file exists, etc.)

    4) Run PHP_CodeSniffer

    5) Add an AllTests.php file - see other packages for examples.

    6) Add the package.xml to your download. (Don't forget "pear package-validate" to check and "pear package" to produce the tarball.)

    Let me know if this helps, or if I can help.

  • Bill Shupp  [2010-09-11 19:37 UTC]

    To add to Till's comments, these days we're getting away from using PEAR_Exception, and simply extending Exception. This helps remove the dependency on the base PEAR package.
  • Christian Weiske  [2010-12-31 11:05 UTC]

    Regarding exceptions: If there are any SPL exceptions, extend from them. Implement/extend your own package exception class.

    The ini file has a <?php exit; ?> in it. This is bad in two ways:
    - php code in a .ini file
    - "exit" stops the whole application. If you want to prevent inclusion of the ini file in php, you should just use return; - but i'd completely remove it.

    Introduce a "Config/" folder that contains Lite.php in your git repo. This makes unit testing easier.

    You could use file_put_contents() instead of fopen&&fwrite&&fclose.

    function get(): null is a good default value sometimes. Making "null" throw an exception is not a good idea.

    __toString() should now throw an exception when no filename is set - maybe I am in the process of creating a new config file and want to debug it. In that case, I get an exception.

    The Config package supports directives (keys) like "foo[]=bar" or "foo[bar]=baz", opening an array. Maybe Config_Lite should support that, too.

    Config also supports comments in ini files, which is a good thing. I'd like to see that in Config_Lite, too.

    Really nifty would be Config_Lite implementing ArrayObject - so that reading/setting config values would be as easy as $config['section']['foo'] = 'bar'.

    There should be a way to set key/value pairs without a section, i.e. by using a null/empty section name.

    How do you handle multi line values? Currently, one can break the ini file by setting a value with newlines. Config does that with a configurable line-continuing string, i.e. \ at the end of the line.
  • Christian Weiske  [2011-01-05 08:28 UTC]

    - run phpcs on the code
    - rename AllTests.php -> Config_ListeTest.php. AllTests in PEAR is to include these specific files only, not to contain any tests itself
    - Exceptions in Exception subfolder: Config_Lite_Exception_Runtime
    - docblocks should be more verbose, i.e. get(): "get" as description is a bit short. "Section" param doc should contain a hint about null values. $default param doc should state that it's used when the config key does not exist
    - I'd get rid of hasSection() and make the $key in has() optional, same for remove/removeSection
    - cweiske:~/Dev/pear/sandbox/config_lite/Config> php ../tests/AllTests.php
    PHP Notice: Please no longer include "PHPUnit/Framework.php". in /usr/share/php/PHPUnit/Framework.php on line 50
    - cweiske:~/Dev/pear/sandbox/config_lite/Config> phpunit ../tests/AllTests.php
    There were 22 errors:
    Config_Lite_RuntimeException: file not found: test.cfg
    -> you need to use dirname(__FILE__) to get the correct
    test.cfg path when running unit tests
    - tests/test.cfg is changed during the unit tests, but is
    also used as test fixture. This is not a good idea.
    Temporary config files should be stored in sys_get_temp_dir()
    - "no filename given" is a bit incorrect in sync(),
    since there is no filename parameter.
    - you may not register your own autoloader. either propse the package in pear2 and expect the class to exist, or require_once the files you need as all pear packages do. throwing an exception when a file is included is also not a good idea.
  • Matthew Fonda  [2011-01-06 19:35 UTC]

    I'd like to emphasize that docblock comments could be a little more descriptive, as Christian pointed out. While the code and method names may be self-explanatory, these comments are used to automatically generate API documentation, so it's important that they give a lot of additional details, beyond just the repeating the method name.
  • Christian Weiske  [2011-01-15 18:51 UTC]

    The code looks really good to me now. I'd say call for votes - being an accepted package doesn't mean development stops :)