Hi everybody, especially Florian, I think this is a great opportunity to lay down some rules for designing appenders. Here are several comments I have on the FirePHP appender implementation:
1. Check for pre-requisites on activation. You should check for presence of FirePHP library in activateOptions(). This method is called before any logging happens and is the perfect place for this. If pre-requisites are not met, then close the appender by setting $this->closed = true. This way no further logging will be forwarded to this appender. This is better than checking on each append. 2. Don't fail silently. This is very important: if it doesn't work, users should be informed that it doesn't work, and if possible why it doesn't work. Appenders have a warn($msg) method for this purpose. If you can't find the firephp lib, warn the user that this is the problem. For an example of 1 and 2 see activateOptions() in LoggerAppenderPDO:126. 3. Use a layout if appropriate. I don't see why we should not use a layout in this appender. It would allow the user to define his own format instead of forcing our own in formatMessage(). Bruce mentioned in his comment (http://bit.ly/H4jbhc) that firephp can render arrays and objects, so perhaps we can make it so that the layout is not used for those types? To discuss. 4. Always add documentation along with a new appender That means, create a new page for the appender at: /log4php/src/site/xdoc/docs/appenders/firephp.xml (easiest way is to copy and modify an existing appender page) In this page, list the configurable parameters, prerequisites and a simple example. Then add the page to the menu in: /log4php/src/site/site.xml 5. Add to XML change log Every time a non-trivial change is made to the code, an entry should exist in the change log: /log4php/src/changes/changes.xml Here is a reference file for the changes xml: http://maven.apache.org/plugins/maven-changes-plugin/changes.html Basically, for this change type is "add", due-to is Bruce, and dev is you. 6. Add to changelog page Big changes such as new features, any breaking changes, etc. should be included in the changelog page: /log4php/src/site/xdoc/changelog.xml Have a look at how it was done for previous versions: http://logging.apache.org/log4php/changelog.html 7. We should agree on which phpdocs tags to include. There seems to be an abundance of phpdoc tags used in this appender, some of which are used wrongly, and some of which are not needed. Not to go through them, one by one, I will make a little guide for phpdoc. 8. Conclusion: So this turned out to be a long email... I wrote all these issues down (instead of just fixing them myself) because I want to show you what is required before the new code can be released. Unfortunately, there is more work than just writing code. And some of that work is boring. I should know. :-) If you need any further with this, just ask. Regards, Ivan
