Re: Waiting on comments regarding the proposed Aerospike PHP extension

Hello Ronen, Just a few comments: 1) it's A LOT of code 2) most of it doesn't seem to conform to the coding standard we're used to: goto's everywhere, strange things like

    if (PHP_TYPE_ISNOTARR(key_record_p) ||
            (!bin_name_p) || (!prepend_str_p) ||
            ((options_p) && (PHP_TYPE_ISNOTARR(options_p)))) {

That's quite a lot of unnecessary brackets. Also the check itself doesn't make a lot of sense to me: these variables aren't optional, so they cannot be NULL anyway. Similarly, in many places you request arrays using zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "a") and then check if they are really arrays. There's no point in that. 3) Returning long on error (especially on parameters parsing error) I'd call a bad idea - there's no way to distinguish between the 'real' errors and programmer's errors this way. 4) Do you really need to duplicate the code this way?

        PHP_EXT_SET_AS_ERR(&error, AEROSPIKE_ERR_PARAM, "Unable to parse php parameters for append function");
        DEBUG_PHP_EXT_ERROR("Unable to parse key parameters for append function");

DEBUG_PHP_EXT_ERROR() can be added to PHP_EXT_SET_AS_ERR() instead. Btw, these _ERROR() and _ERR() suffixes should probably be consistent if you insist on using these macros. 5) The extension doesn't compile: In file included from /local/git/php-src/ext/aerospike/aerospike.c:35:0: /local/git/php-src/ext/aerospike/php_aerospike.h:35:32: fatal error: aerospike/as_error.h: No such file or directory #include "aerospike/as_error.h" I guess it requires some library, but it doesn't look or check for it during configure because it uses --enable-aerospike instead of --with-aerospike=[INSTALL PREFIX of the lib]. On 10/17/2014 08:13 AM, Ronen Botzer wrote:

Hello, It's been over a week since I introduced the PHP extension for Aerospike ( http://news.php.net/php.pecl.dev/12321). So far there have been no comments for or against. I'd appreciate if someone took a look at it. Subsequently, I would need an account as the maintainer. Aerospike is an open-source NoSQL database, not unlike Redis or Couchbase or Mongo, all of which are already in PECL. The code is currently on GitHub (https://github.com/aerospike/aerospike-client-php) and available through Composer ( https://packagist.org/packages/aerospike/aerospike-client-php). Thanks, Ronen

-- Wbr, Antony Dovgal --- http://pinba.org - realtime profiling for PHP