Till Klampaeckel [2010-09-09 18:02 UTC] Great,
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.
Till
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
EEEEEEEEEEEEEEEEEEEEEE
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 :)
|
