[PEPr] +1 for Web Services::Service_Amazon_S3

Till Klampaeckel (http://pear.php.net/user/till) has voted +1 on the proposal for Web Services::Service_Amazon_S3. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=527 Vote information: http://pear.php.net/pepr/pepr-vote-show.php?id=527&handle=till This vote is conditional. The condition is: Hey Christian, first off, great work on this. They are a few things, I want to note: I still don't think that you should silence what you called "low-level" errors. I believe people should run this code with display_errors off and use an appropriate setting in error_reporting or get rid off them completely if it doesn't matter to them. I for myself want to see those errors (at least in the log) and an "@" in the middle of the code makes it super hard if not impossible to debug - whatever the issue may be. In some ways, I'm not too comfortable with some of your code, e.g. I think your if/else/elseif's are too huge. I believe we just had that discussion the other day, since I am not sure if I make myself clear, check this comment by Helgi: http://paul-m-jones.com/?p=276#comment-298417 If it was up to me, I also wouldn't use ternary if in the code (when you build $stringToSign). I agree that a ternary if has its use case with one liners, but with the line breaks and indents it looks rather weird. And last but not least in regard to control flow, please do this: if ($foo && $bar) ... instead of: if ($foo && $bar) On class naming - IMHO, Service_Amazon_S3_Prefix should be Service_Amazon_S3_Resource_Bucket_Prefix. It is used by Amazon_S3_Resource_Bucket exclusively, why not reflect it visibly as well to show the hierarchy in code. As outlined on my previous comments, I also would use more error codes which are defined as a class constant in your class somewhere. This enables the developer to match error cases based on that, and not just basic exception type and parsing the error message. For example: Services_Amazon_S3 { const ERR_WHATEVER = 1; ... } ... try { ... }catch (Services_Amazon_S3_Exception $e) { if ($e->getCode() == Services_Amazon_S3_Exception::ERR_WHATEVER) { throw $e; } die ($e->getMessage()); } Of course HTTP_Request's error codes get transported out in the exceptions (btw, good job), but those other exceptions where HTTP_Request is of no concern, I'd have to match based on Exception type and then parse the error message to understand what went wrong exactly and based on that decide if I go on and gracefully ignore it, or not. I haven't run the code just now, but by reading through it, I believe that some of the calls to Services_Amazon_S3_Exception seem to be "borked". Whenever you do something like "throw new Services_Amazon_S3_Exception($result->getMessage(), $result->getCode());" but your "__construct()" only takes one argument. Not sure how this works, if I overlooked something, please ignore this. My last concern with naming are your exception classes, but this is just on a sidenote, a very personal preference and by no means conditional. For example I would like, Services_Amazon_S3_Resource_Bucket_Exception instead what you have now. I think those would make it clearer where an exception comes from, etc.. And last but not least, some of your test suits mentions Net_LDAP, which is prolly a leftover and probably just eyecandy! That's all! Great work none the less! -- Sent by PEPr, the automatic proposal system at http://pear.php.net