Managed to take a look and it works very well. Very nice, very cool. :)
Thanks :-)
* Why not use Crypt_HMAC2 (to minimize PHP4 dependencies)?
Crypt_HMAC2 is still in beta, so BC breaks are still allowed, so I'd rather not depend on it. But the HMAC-signing isn't exposed outside the class, so when Crypt_HMAC2 becomes stable, I can upgrade the requirement without breaking BC of the S3 package.
* Maybe lazy load the sub classes (e.g. Stream) with include_once when
necessary?
The stream class isn't explicitly included anywhere (is it?). I'll try to conditionally include_once some of the least used classes e.g. exceptions.
* 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).
Are you suggesting that I replace
@unlink($foo);
with
$oldLevel = error_reporting();
error_reporting($oldLevel & ~E_WARNING)
unlink($foo);
error_reporting($oldLevel);
?
As long as the method is guaranteed not to trigger an E_ERROR, I think that the @ syntax is easier to understand and less error prone.
Or are you suggesting that I refrain from calling functions that may trigger an error? The latter is not always possible.
WRT $doc->loadXML() I can use libxml_use_internal_errors() to prevent it from triggering errors on malformed XML.
* Your hardcoded ".s3.amazonaws.com/", is that save to do?
Yes - unless a competing company wants to offer the same service using Amazon's API.
* Minor preference: Some of your if-else seem redundant:
if (...) {
return ...
} else {
return ...
}
Could be:
if (...) {
return ...
}
return ...
I used the former way in order to indicate that both code paths represent a valid/sound way to end the call. The latter way may look like an early return due to an error condition. But this is just my private view.
* Curious, what exactly needs the 5.1.1, and not 5.2.0 (for example)?
Both 5.2.0 and 5.1.1 are fine. Does <php><min>5.1.1</min></php> imply otherwise?
I haven't given the PHP version requirement much thought and I haven't tested with 5.1.x. My reason for choosing 5.1.1 is my use of the constant DATE_RFC1123 that was added in that version.
* 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?
Users of the API usually don't need to care about Amazon_S3_Resource, and the term "resource" isn't used a lot in the Amazon docs. But I guess it makes the relationship more visible, so I'll change the names.
* I'd like more sub-classed Exceptions to be able to catch specific
errors.
Ok, I'll introduce some more exceptions based on the HTTP error code.
* I'd also like error codes in the exceptions to be able to match them in
my app.
Note that $exception->code often the HTTP error code (if available), and $exception->errorCode contains the Amazon-specific code (if available):
http://docs.amazonwebservices.com/AmazonS3/2006-03-01/ErrorCodeList.html
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);
I think this particular example would indicate a programming error and not an exception that you would specifically want to handle at runtime. But it understand your general point.
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.
The PHP manual is pretty vague about how to write stream wrappers, so I don't know for sure. But the manual page doesn't mention exceptions, so I guess they aren't allowed.
http://www.php.net/stream_wrapper_register
Thanks a lot for your detailed feedback.
BTW for those interested I can mention that the stream wrapper is being using in production in a Drupal setup where uploaded files (i.e. those that are usually saved in DOCUMENT_ROOT/files) are saved to S3 (almost) transparently.
Christian