Re: Waiting on comments regarding the proposed Aerospike PHP extension

Hi Antony, Thanks for the feedback. It's due a bit of cleanup, apparently. Regarding (3): methods in which the client talks to the server return a status code, which may be an error but not necessarily. It's not unlike mysqli or curl. Regarding (5): I believe it does compile if you run the build.sh script, which fetches the C SDK the extension wraps around. If not, can you let me know on which platform you're seeing this? Or are you saying that the extension must follow the phpize && ./configure && make form? Thanks, Ronen On Fri, Oct 17, 2014 at 2:15 AM, Antony Dovgal <tony@daylessday.org> wrote: > 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 > > -- > PECL development discussion Mailing List (http://pecl.php.net/) > To unsubscribe, visit: http://www.php.net/unsub.php > >