Re: Extension proposal: dom_varimport

Johannes, Thanks a lot, I modified the extension carefully according to all of your notes. Everything has been covered by tests, here they are: https://github.com/DmitryKoterov/dom_varimport/tree/master/tests About your note "You should add the dom extension as dependency in the module structure, this can improve error messages in some cases when using shared libraries ...". I thought that THERE IS the dependency declared, see https://github.com/DmitryKoterov/dom_varimport/blob/master/config.m4#L25 . I also added DOM extension dependency directly to PHPT tests as well - please see e.g. https://github.com/DmitryKoterov/dom_varimport/blob/master/tests/002_formats.phpt#L6 Maybe you've meant some other dependencies? How to declare them? I googled, but I failed. On Mon, Apr 21, 2014 at 6:47 PM, Johannes Schlüter <johannes@schlueters.de>wrote: > Hi, > > On Fri, 2014-04-18 at 17:11 +0400, Dmitry Koterov wrote: > > Dom_varimport is a simple PHP extension to convert nested arrays into > > DOMDocument with minimum CPU and time consumption. > > > Samples: > > https://github.com/DmitryKoterov/dom_varimport > > From a quick review this seems to be fine, a few minor comments: > > * You should remove the [RM][INIT|SHUTDOWN] functions, especially > the ones with R lead to unneeded calls and pointer dereferencing > etc. on every request > * You should add the dom extension as dependency in the module > structure, this can improve error messages in some cases when > using shared libraries ... > * The declaration of "static int le_dom_varimport;" makes me > suspicious - when compiling with proper error messages enabled > this should trigger a warning. Maybe there are other warnings > from the compiler (CFLAGS=-Wall etc.)? > * In general we prefer php_error_docref over php_error. This could > (depending on the user's configuration) link directly to the > docs from the error message :-) > * In php_dom_varimport() for the default case I suggest throwing > an error ("Unknown type") to be on the safe side. > * From a design I don't think the fallback to "item" for > "invalid" > tag names is a good choice - I think an error might be more > appropriate. Ideally the "key" attribute name could also be > configured, especially when thinking about XML namespaces > > All minor things and the last thing is more a matter of taste :) > > johannes > > >