Comments for "Service_Amazon_S3"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments

  • Till Klampaeckel  [2008-02-25 17:36 UTC]

    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. :)