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

Thank you I will take a look and revise my code accordingly. -- Ray Perea Snap Web Systems Like us on Facebook Follow us on Twitter On Dec 27, 2012, at 2:07 PM, Johannes Schlüter <johannes@php.net> wrote: > 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 > > > >