[PEPr] Comment on File System::File_Mogile

Philippe Jausions (http://pear.php.net/user/jausions) has commented on the proposal for File System::File_Mogile. Comment: Quick review of Mogile.phps - Check all your docblocks, some of the @access are not reflective of the declaration (also check order of tags, see PEAR CS docblock example) - The open { for a method declaration is on the following line not on the same as the "function" keyword - Run your code with E_ALL, things like "list($ip, $port) = split(':', $host, 2)" will spit a notice if $host doesn't contain a ":". - BTW, use explode(':', $host, 2) instead of split() - Why use microseconds as timeouts if they're going to be divided by 1000000? - Pick a single quotation style throughout your package, preferably single quotes, and stick to it. - Avoid things like "{$domain}class{$j}" when you could use $domain.'class'.$j or "Unable to open \"$data\"" vs. 'Unable to open"'.$data.'"' (more syntax coloring friendly) - Add "b" when opening files (i.e. fopen($path, 'rb') vs. fopen($path, 'r') - The code would be a little bit cleaner if instead of (this is just one example): if ($words[0] == 'OK') { parse_str(trim($words[1]), $response); } else { throw new File_Mogile_Exception('mogilefs: ' . trim($line)); } you had: if ($words[0] != 'OK') { throw new File_Mogile_Exception('mogilefs: ' . trim($line)); } parse_str(trim($words[1]), $response); - In _store(), is !preg_match('/^http:\/\/([a-z0-9.-]*):([0-9]*)\/(.*)$/' really correct? That would match http://...:00/ (btw use ! as regex delimiter so you can use / without the need of escaping it.) - I don't think the require_once 'PEAR.php'; is needed for the exceptions Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=528 -- Sent by PEPr, the automatic proposal system at http://pear.php.net