I still don't think that you should silence what you called
"low-level" errors.
This is being discussed in another thread, but I just want to note that
@foo() is only once and that is inside the stream handler where I am
required to silence errors, unless the STREAM_REPORT_ERRORS flag is set. If I break that contract, the wrapper does weird stuff.
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.
Could you provide some examples where this is a problem?
check this comment by Helgi:
http://paul-m-jones.com/?p=276#comment-298417
I agree that returning early in case of an error is often a good idea, and AFAICT I do that wherever possible. In other cases, I don't think it necessarily makes the code more readable.
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.
I guess I could do that. On the other hand would it perhaps make it less obvious that the Resource subdirectory reflects another kind of hierarchy (class inheritance).
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 [...]
You should rather use $exception->getAmazonErrorCode(). But yes, I'll look into adding some constants to S3_Exception reflecting the error codes at http://docs.amazonwebservices.com/AmazonS3/2006-03-01/ErrorCodeList.html and add some constants for $exception->code for all exceptions.
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".
You are right. I'll fix that.
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..
The exception types are currently orthogonal to buckets and objects. I could split NotFoundException into a BucketNotFoundException and an ObjectNotFoundException (and likewise for AccessDeniedException), but is it really relevant to catch exceptions related to buckets and objects in seperate catch sections? I think it may be better to add a field to the exception class that indicates the resource in question.
And last but not least, some of your test suits mentions Net_LDAP,
which is prolly a leftover and probably just eyecandy!
Fixed.
That's all! Great work none the less!
Thanks :-)
Christian