Re: [PATCH]#55651

hi! On Mon, Sep 12, 2011 at 2:29 AM, Avi Brender <abrender@elitehosts.com> wrote: > Hi, > > Please see if the attached patch better addresses your concerns. > > Regarding the variable name, the PHP_FTP_OPT_USEPASVADDRESS is only internal > and is modeled after the other variables PHP_FTP_TIMEOUT_SEC and > PHP_FTP_OPT_AUTOSEEK. The variable actually passed to the ftp_set_option() > function by users is FTP_USEPASVADDRESS as defined in php_ftp.c which is > inline with the other variables FTP_AUTORESUME, FTP_TIMEOUT_SEC, > FTP_AUTOSEEK etc. + REGISTER_LONG_CONSTANT("FTP_USEPASVADDRESS", PHP_FTP_OPT_USEPASVADDRESS, CONST_PERSISTENT | CONST_CS); It is a userland constant too. Everyone understands FTP_AUTORESUME or FTP_TIMEOUT_SEC, _FTP_OPT_USEPASVADDRESS is cryptic compared to the other :) > In terms of tests, what type of tests were you thinking of? We can't ensure > that ftp->pasvaddr is set properly in response to a PASV command unless > there's a way to expose those internal variables to the test - I'm not > familiar enough with the internal PHP code to know if that's possible. If > you're referring to tests that ensure that 1/0 true/false values passed to > ftp_set_option() work properly then I've attached a test file for that. > > I don't want to clutter the bugzilla ticket with many attachments  so once > the patch is approved I will post the final version in the ticket if that's > okay. That's fine too. You can the tests with the patch too, just do svn add ext/ftp/tests/... before you call svn diff. Thanks for your work so far! Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org