[PEPr] Comment on XML::Create_KML

Responding to existing comments: 3.) I don't see an issue with a method that sets the appropriate headers as long as it remains optional and doesn't get called by default. 4.) Wikipedia's entry on unit testing is pretty good. You can also check out some examples of packages with unit tests in pear. For example: Services_Amazon_SQS and HTTP_Request2. My additional feedback: a.) Use require_once, not require as specified in the PEAR coding standards. This will prevent fatal errors if code is inadvertantly required from two or more places. b.) There's a parse error in XML_KML_Style and XML_KML_Place, you need to define class properties outside of functions, not in the constructor. c.) @access in docblocks is not needed since you're specifying the scope of methods with public/private/protected already. d.) CDATA escaping, etc can all be done automatically and correctly using the DOM API. (http://ca3.php.net/manual/en/class.domcharacterdata.php) What you have now is vulnerable to XML injection. If you're using simplexml, it may also automatically escape data when required. I have less experience with simplexml so I cannot say for sure that it will escape properly. e.) Consider a fluent interface for set* methods. In such an interface, your setters return $this. Then in calling code you can do: $place->setFolder('foo')->setCoords(10,200)->setDesc('My House'); -- http://pear.php.net/pepr/pepr-proposal-show.php?id=616