Re: Submission for PHP CLI Audio Streammer

Hi Lucas, On Tue, 2015-04-07 at 13:57 -0300, Lucas Mendes wrote: > https://github.com/devsdmf/php-cli-audio I think the extension is a bit too specific. I admit that I know nothing about mikmod, but from looking at the extension I would prefer export mikmod library features to PHP userland so tat the forking logic etc. can be done in userland. Anyways a few comments on the code: 1. From a design I wouldn't use ini settings but provide the values via (optional) function paramters. 2. There is a filename specific zend_parse_parameters option (I think 'p', check README.PARAMETER_PARSING) which checks for \0 and other issues. 3. Please don't use zend_error outside egine code. https://wiki.php.net/internals/review_comments#don_t_use_zend_error 4. In line 141 you set md_mode, I neither see a declaration, nor usage (might be hidden behind MikMod macors .. then MikMod is badly designed, ot your issue) 5. If Player_LoadFP returns false the fptr isn'T closed 6. Please add /* {{{ proto */ comments, those help some documentation tools. 7. Please see https://wiki.php.net/internals/review_comments#php_extnameh_should_be_minimal for comments on php_cli_audio.h 8. I think the complete logic from Makefile.frag could be put into config.m4, where the first line is also suspicious. See i.e. curl, which calles curl-config or different xml extensions which call xml2-config for a better way to handle these. johannes

Attachment: [application/pgp-signature] This is a digitally signed message part signature.asc