[PEPr] Comment on Web Services::Service_Amazon_S3

Till Klampaeckel (http://pear.php.net/user/till) has commented on the proposal for Web Services::Service_Amazon_S3. Comment: Managed to take a look and it works very well. Very nice, very cool. :) Thanks for doing this. I have a few suggestions, or questions, hope you don't mind. :) Questions/suggestions: * Why not use Crypt_HMAC2 (to minimize PHP4 dependencies)? * Maybe lazy load the sub classes (e.g. Stream) with include_once when necessary? * I wouldn't silence any calls with @ in your code - people should either set the respective error_reporting to get rid of these, or make sure that the code works (it's also harder to debug if those are in place). * Your hardcoded ".s3.amazonaws.com/", is that save to do? * Minor preference: Some of your if-else seem redundant: if (...) { return ... } else { return ... } Could be: if (...) { return ... } return ... * Curious, what exactly needs the 5.1.1, and not 5.2.0 (for example)? * When you extend from your abstract (Amazon_S3_Resource), wouldn't it be better to call the result Amazon_S3_Resource_Bucket, instead of Amazon_S3_Bucket? * I'd like more sub-classed Exceptions to be able to catch specific errors. * I'd also like error codes in the exceptions to be able to match them in my app. For example: - throw new Service_Amazon_S3_Exception('Array index "' . $part2 . '" not found'); + throw new Service_Amazon_S3_Exception('Array index "' . $part2 . '" not found', 404); Or maybe a class constant instead of a hardcoded error code - const ERR_NOT_FOUND = 404 (etc.). (This class would also be a perfect match for the "http-request-code" in Services_Akismet. :)) For streams: * I see you used trigger_error() in the streamwrapper, is it possible to throw exceptions instead? Can't say I tried it and would know what happens. Just a question, or maybe a configuration option. Sorry for not writing this earlier. :) Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=527 -- Sent by PEPr, the automatic proposal system at http://pear.php.net