Re: [PEPr] Comment on Web Services::Service_Amazon_S3

Hey Christian, I am just citing/quoting what I am replying or feel like giving feedback to. On Mon, Feb 25, 2008 at 9:26 PM, Christian Schmidt <chsc-public@peytz.dk> wrote: > > * 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. My bad then. Thought I had seen it in S3.php. It was more a general thing. > > * 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); No. Generally: Replace: @unlink($foo); With: unlink($foo); Or, if you really want to make sure to not distract your app through "output", catch it with: $error = ''; ob_start(); unlink($foo); $error .= ob_get_contents(); ob_end_clean(); if ($error != '') { throw new.../trigger_error... } (No guarantees for typos.) Generally I wouldn't silence an error in there. > (...) > WRT $doc->loadXML() I can use libxml_use_internal_errors() to prevent it > from triggering errors on malformed XML. No, see above. I wouldn't silence them. They are probably reported for a good reason. And if people are serious about their app they will probably log everything to a logfile and will want to know about this. At least so I think. > > * 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. Yeah, probably true though I haven't looked into what Nirvanix for example does. I just heard that service was not so great (yet). I was also thinking along the lines of - different datacenter, etc. pp.. I am not sure if you can abstract that through the API. For example - you can (somehow) select where your data is stored. In the U.S. or Europe (or other places as other datacenters are being build). Not sure if you need to abstract that or if the loadbalancer "behind" .s3.amazonaws.com does that for you. > > * 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. Fair enough. :) > > * 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? No, I was just wondering why that specific version. But you answered that with date. ;-) Just curious anyway. > > * 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 IMO - since the game is called REST people should be aware of this themselves. The list of errors is pretty standardized and I see no real reason aside from lazyness ;-)) for the programmer to double the information in your class? But that's just my very personal point of view. So $e->getCode() "replies" to the HTTP equivalent and $e->errorCode to Amazon? > > 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 I think Philippe answered that for us. :) Thanks again! Till