Re: Net_Curl bug fixes / status

1.) The {{{ / }}} are code folding for vim (and others?). They are present throughout a lot of the PEAR code. Including PEAR.php. They may not be in the CS per se, but it's somewhat implied and not any more of a major issue than the vim rules that appear in some code as well. BTW, the vim rules are even in the example code. Hrm. Actually, the code folding appears in the sample file as well: http://pear.php.net/manual/en/standards.sample.php 2.) You got me. I'll add extra checking around my is_resource() calls. 3.) True, it does not, but the majority of the code written in PEAR fullows humpBack, as did many of the variables and functions in Net_Curl. This lead to inconsistency within the package and went against common practices in PEAR (even if it's no CS per se). Considering how broken the previous Net_Curl package is/was and the fact that only one package relies on this I think it's fine to break BC in this case. 4.) Yup. Not my code, but it was valid code. Also, I believe 3xx codes are for redirections, which is a non-issue for Net_Curl since it is set up to follow location, thus the *final* code should either be a 2xx or some error code (4xx or 5xx). I'll update the code just to be safe as I find your suggestion a cleaner implementation. Actually, in a later patch you'll note that I *do* use the substr(), but don't check for 3xx. Thanks! --Joe On Jul 13, 2005, at 3:51 PM, Ian Eure wrote:

On Wednesday 13 July 2005 02:47 pm, Joe Stump wrote:

All, I've spent the last few hours clearing out all of the bugs for Net_Curl, fixing the coding standards, adding PHP5 support (native __construct()/__destruct() with 4.x BC) and adding extra error checking. The patches can be found here: http://zebulon.miester.org/~jstump/pear/Net_Curl/

Are these: // }}} // {{{ __construct($url = '', $userAgent = '') useful at all? Do they do some sort of editor folding/phpDocumentor magic? In 2_Net_Curl_PHP5_Compatiblity.patch:

        if (is_resource($this->_ch)) {

This should be:

        if (isset($this->_ch) && is_resource($this->_ch)) {
otherwise you'll get a warning with E_ALL if you call close() before $_ch is

set. In 3_Net_Curl_Cleanup_and_Fields_Fix.patch:

-    var $follow_location = 1;
+    var $followLocation = 1;

Doesn't this break BC? I don't think CS specifies anything about variable names. In 4_Net_Curl_Bug_2562.patch: $info = curl_getinfo($this->_ch); $httpCode = (string)$info['http_code']; if ($httpCode != '' ... Ugly. According to the PHP docs, curl_getinfo() always returns an array if called with just the resource argument. Why is this ugly code here? (I assume it's not your work, Joe.) How about: $info = curl_getinfo($this->_ch); if (!is_array($info) || !isset($info['http_code'])) {

    return PEAR::raiseError("Unknown or invalid HTTP response");

} $t = substr($info['http_code'], 0, 1); if ($t != 2 && $t != 3) {

    return PEAR::raiseError('Unexpected HTTP code: ' . $info['http_code']);

} In 4_Net_Curl_Bug_2562.patch: if ($httpCode != '' && substr($httpCode,0,1) != '2') { What if the server returns a 3xx status?