if anybody wonders the wiki page linked by Johannes was moved to https://wiki.php.net/internals/review_comments
On Thu, Dec 27, 2012 at 9:07 PM, Johannes Schlüter <[email protected]> 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 > > > > > -- > PECL development discussion Mailing List (http://pecl.php.net/) > To unsubscribe, visit: http://www.php.net/unsub.php > > -- Ferenc Kovács @Tyr43l - http://tyrael.hu
