Re: Extension proposal: dom_varimport

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