Re: I want to publish my extension (php websockets)

Hi, On Thu, 2012-12-27 at 09:31 -0600, Ray Perea wrote: > Hello. My name is Ray Perea of Snap Web Systems. (http://www.snapws.com) > I am writing this list because I would like to publish my PHP extension in PECL > > The extension introduces some functions for reading from and writing to websockets (RFC6455) > > These functions are: > ws_send() - Send data to a websocket connection > ws_read() - Read data from a websocket connection (Can read binary frames and files) > ws_close() - Sends a close frame to the websocket client > > I have published the source code at sourceforge: > https://sourceforge.net/p/phpwebsockets > > Please have a look and see if you would like me to publish my extension in PECL. > Thanks in advance for your consideration. I took a quick look on the code and found some common flaws I see in proposals. So instead of listing them here I started a wiki page: https://wiki.php.net/internals/reciew_comments please have a look at the comments there. Some additional comments on the code: * It would be great if you would follow the PHP CODING_STANDARDS (see that file in php-src) for my mind it is simpler to read ;-) * For php_websockets.h please check the comments on the wiki, especially avoid includes like #include <stdint.h> * E_ERROR is a very bad error code. We try to limit its usage to cases where we can't recover at all. This will disable most ways for the user to handle errors. * In ws_read there is a strange code part if (zend_is_callable()) { } is there any reason for that? - I assume you want to extend that for a callback? If yes please look at the "f" modifier for zend_parse_parameters() and the family of zend_call_method[_with[1-4]param[s]] functions/macros from zend_interfaces.h * Overall the code is quite weak on error handling, especially stream operations are often not checked for an error but run into undefined behaviour in case of errors * In WebsocketsSendClose() you're passing a length for the reason but are completely ignoring it but call strlen() on the message. Sometimes you actually calculate it two times - once before calling, once inside the function. The suggested way is to use sizeof() on the constant strings and pass that through. * In different functions you are calling TSRMLS_FETCH() even though you don't need the TSRM context. In ZTS mode this operation is quite expensive. When possible (and needed) you should pass the TSRMLS parameters through ... TSRMLS_FETCH() should be a lat resort in cases were you can't pass it through (i.e. callbacks to TSRM-unaware things) * Some type usage might also lead to errors, i.e. php_stream_read() returns size_t but you're assigning to a uint32_t which might cause issues on systems where size_t is 64 bit (even though it's unlikely a single read would return a few gigs of data ;-)) Looks like a long list, but all minor things in fact :-) Going away from those code-centric things: I wonder why this is implemented using custom functions. Won't it be nicer to implement this as custom stream type, or maybe even a stream filter? All it seems to do is enrich stream messages ... I also wonder what benefits you expect from implementing this in C over a pur PHP approach. I expect most time to be spent in streams themselves ... I always suggest to use pure PHP things as they are simpler to write, maintain, deploy, monitor and debug. johannes