Review Frederik =============== [done] ezcLogFilter - This is a struct. Place into structs directory and add constructur and the __ method. [done] ezcLog - Add large example on how to use the class a a logger. ezcLog::attach/detach - These names are strange as we are not attaching anything. What about: map and unmap? [done] ezcLog::setSeverityAttributes/setSourceAttributes - These need doc tag cleanup and examples in order to be understandable. Maybe a rename of the methods would be nice also. E.g setStandardSeverityAttributes [done] ezcLog::LogHandler - name starts with capital letter? Add example on how to use trigger_error. [done] ezcLog::__set and __get don't throw when they should. [done] ezcLogWriter::writeLogMessage - source and category, where do these come from? ezcLogContext - Class description needs a better introduction an example. ezcLogMap - rename to ezcWriterMap? Or are we using it for other things as well? ezcLogMap - The intro from "Our solution" is a bit unclear to me. ezcLogMap::map --> addMap? (remove ambiguity) ezcLogMap::mergeSingle - lacks doc. I'm not entirely sure what this method does. .. $tmp?? ezcLogMap::mergeHash - lacks doc and is marked for rewriting. ezcLogMap::unmergeHash - lacks doc and is marked for rewriting. ezcLogMap::printStructure - lacks doc ezcLogMap::printStructureRecursive - lacks doc [done] ezcLogFileException - constants. Correct naming? (capital vs non capital?) [done] ezcLogMessage::__set and __get does not throw when they should [done] ezcLogMessage::parseMessage - document the format for $message. ezcLogDatabaseWriter - Document the properties and how they work with the table that is writes to. ezcLogDatabaseWriter::__construct - doc talks about tie-in. What do you mean by that in this context? RB assumed: ezcLogWriterBase -> ezcLogFileWriter ezcLogWriterBase:: last three methods. Should they be public? They also lack doc. [done] ezcLogWriterBase::writeLogEntry - only doc no impl. Should this be removed? [ no ] ezcLogWriterBase::rotateLog - shouldn't the for loop start at ($this->maxFiles - 1)? ezcLogWriterBase::logTo - you do not catch the exception from openFile, should it be catched and rethrown or should it be documented? [done] ezcLogWriterBase::logTo -> mapEventToFile ? [done] ezcLogWriterBase::logNotTo ->unmapEventToFile Comments by Jan Borsodi (15-12-2005) ------------------------------------ General: - Several @param entries lacks documentation, also the synax is wrong in many places. ezcLogMap:: - This class definitely needs a cleanup, it's not easy to understand the code here The current documentation for the class is something which belongs in a design.txt file not here. - ::map(), should be named insert(). - ::unmap(), should be named remove(). - ::get(), the use of $tmp is not good, please use multiple variables with good names. ie. $events, $sources, $categories and the last if/elseif/else should return immediately with the found value. - ::printStructure - What is the purpose of this? isn't this debug code which should be removed? ezcLogMessage: - [done] Document the properties, no mention of them. - [done] Make sure the property array is initialised with empty strings. ezcLogFilter: - [done] Needs better documentation, no explanation of what it is and what it used for. ezcLogContext: - I have no idea what a context is, please explain. - No constructor. ezcLogFileException: - [done] Document constants (after fixing the naming). ezcLogWriter: - Document how this is meant to be implemented by classes, e.g. by using an example class. ezcLogDatabaseWriter: - The database calls should either use the new Query class or use prepared statements with binding. If the second is used the code could be changed so you can pass PDO objects to it making it more general. - [ no ] Why use $query->now(), isn't it better to insert the time with mktime()? - [done] Get rid of the ezcDbInstance::get() in the constructor, the instance should always be passed. - Throw exceptions if $databaseInstance or $defaultTable are not set. - Document the table structure it requires. - If this is meant to rely on the database component it should be moved out in a separate Tie-In package. - [done] ::getColumnTranslations() - rename, it has nothing to do with translations, the doc for it says coupling - [done] ::logTo() - rename, this sounds like it logs something to the database but it only sets up the mapping + doc - [done] ::logNotTo() - same as logTo() + doc - Document how this writer is meant to be used. ezcLogUnixFileWriter: - [done] What is Unix style? - More documentation for class. - [done] ::implodeWithKey() - document! ezcLogFileWriter: - [done] Explain how this is meant to be subclassed. - [done] ::logTo(), ::logNotTo() - same as for writerdatabase - [done] ::openFile() - lacks @param doc, lacks @throw doc - [done] ::__destruct() - Make sure openFiles is initialised as an array, then you can get rid if the if() check.