Hi Johannes, Thanks for the remarks. I've pushed a new commit that fixes them.
I requested a PECL account using that form. Best regards, Boro Sitnikovski On Friday, September 5, 2014 1:12 PM, Johannes Schlüter <[email protected]> wrote: Hi, On Fri, 2014-09-05 at 02:40 -0700, Boro Sitnikovski wrote: > I've made a PHP extension for managing (e.g. playing, decoding, > retrieving metadata, mixing) module tracked files > (http://en.wikipedia.org/wiki/Module_file), which is using the libxmp > library. > You can find the code for the extension here: > https://github.com/bor0/xmp From a quick glance over the code it looks good. Some minor comments: * In one place I noticed you used a size_t for a string lenth for "s" modifier of zend_parse_parameters. This has to be int (in PHP 5.x) * You should use zend_parse_parameters_none() in function taking no arguments for being consistent with other extensions * In xmp_create_context error handling looks wrong * Using add_assoc_*_ex instead of add_assoc_* lets you provide the length of the key, this saves a strlen() call for each of those * If you move the xmp_functions list to the end of the file you can remove the declarations from the header. I'd prefer that stylistically, see also https://wiki.php.net/internals/review_comments#php_extnameh_should_be_minimal * Two comments on licensing: libxmp licensing is confusing. The sourcefore page lists LGPL 2 and GPL 2. Looking at the source I see a mixture of files with LGPL, BSDish, public domain, no license. This is no problem for PHP (i.e. no GPL) but if you talk to them you might as them to clean that up. Secondly you are using PHP license. Debian sometimes claims this can only be used for PHP itself, we disagree. Some people therefore prefer BSD or Apache license. These are all minor things. If nobody else sees an issue feel free to request PECL account for publishing on pecl.php.net via http://pecl.php.net/account-request.php . If you want to host the primary git repo on git.php.net (will be mirrored to github) and bug tracker and out git setup make sure to set the checkbox (unless you already have an account) and tell us so we can enable that. johannes
