Re: New XSLT class for PEAR

Stuart Herbert wrote:

Hi Pierre-Alain, I've read the code. XML_XSLT_Wrapper *is* a simple wrapper, and I'm sure it works. A couple of comments, if I may. o I'd prefer to see XML_XSLT_Wrapper::factory() take a second parameter - an array to be passed to the constructor of the backend. o What does XML_XSLT_Wrapper::Init() do? $obj doesn't appear to be in-scope, so I'd expect a PHP warning generated for that. I'm not a fan of the design of your backend baseclass and API ;-)

Me neither :-p I think we can come up with something better. I guess we should move this discussion from the main pear mailing list soon :-p

* It's stateful. Urgh. Why?

As mentioned, I agree. I think we can correct this. As for the rest of your comments, I will try and take them all into account as I work on this modified version that will support offloading rendering to the browser. After that it will just be up to Pierre to review and approve the changes.

* XML_XSLT_Common::setXML() takes *two* copies of the XML, if it is passed in as a string. The parameter $options is completely unused. If the XML is passed in as a file, or as a URL, this method makes no attempt to standardise behaviour by creating a string with the contents in. The variable XML_XSLT_Common::xml can be a string, a filename, and a URL. It may save on the typing, but it's a poor quality solution. * XML_XSLT_Common::setXSL() - same comments. Plus there's lots of duplicated code in these two methods. And it should be called setXSLT(), because XSL is actually something else ;-) * XML_XSLT_Common::setParams() uses foreach(). foreach() works on a copy of the array, not the array directly. As a result, this method ends up creating two copies of each parameter - and the copy that's passed in makes three. * There's no way to unset a parameter. * Why do you need setOption() at all? Much cleaner to just pass in the options to the backend as an array into the backend's constructor. * _mkdir_p() is copied from elsewhere in PEAR. That should only be necessary if there's something wrong with the design of PEAR ;-) Food for thought, Stu --