kolibri-discuss team mailing list archive

[Anders Steinlein <anders@xxxxxxx>]

Review: Needs Fixing
This is shaping up very well indeed. I look forward to using the new INI configuration support. :) Most of my thoughts while first looking through this code has already been fixed, which only leaves a few minor issues and suggestions:

ConfigHelper:
- Documentation for loadApp() is out-dated (params).
- Method docs should mention what it returns, i.e. "Loads and returns the ..." in loadApp() and loadMode().
- $this->interceptors seems to be used instead of the old $classes in prepareInterceptors(). Shouldn't :156 then set $this->interceptors as well, and not $classes?
- Documentation for prepareInterceptorMappings is out-dated (params).

Config:
- It doesn't seem like getMode() really need the parameter. Can't we check !isset(self::$instance) instead?
- Move require() of conf/autoload.php to constructor and cache the data in an instance variable. Consolidates requires and prepares for future config caching.
- Application-specific /lib is added to include_path no matter what. It isn't necessary if there is no such directory (in fact it might be wrong).

Other stuff here and there:
RequestProcessor: require() DefaultActionMapper when short-circuiting findActionMapper(). We can then remove DefaultActionMapper from autoload config.
InterceptorFactory: Comments are not updated (params).
error.php views (remember both src and example): One empty($config['logging']['level']) covers both !isset() and false checks.
DatabaseFactory: Update comment regarding default database configuration
PostgreSqlConnection and SqliteConnection: Update class comment regarding database configuration.
-- 
https://code.launchpad.net/~frode-danielsen/kolibri/config+autoload/+merge/5563
Your team Kolibri Discuss is subscribed to branch lp:kolibri.