several fixes to the AmazonS3Resolver based on feedback

[havvg]

This is a follow-up on #66.

[lsmith77]

is the PR ready for review?

[havvg]

@Spea I wrapped the if_object_exists, so you could add any caching to it by extending the resolver and overwriting this single method. I think that's a valid solution, until the Cache component is available.

@neilferreira Do the object url options work for you? You can add a call in your production environment to have the https flag set, while having it unset in your development env.

[havvg]

@lsmith77 Now it is :-)

[lsmith77]

yeah .. the approach of allowing people to easily extend and add caching is the best way atm.

[neilferreira]

Hey,

Nearly there! Just a few problems left.

The getBrowserPath function is now completely broken, when you're callign getObjectUrl, you're passing the target path rather than the objectPath, so the filter name is not prepended to the URL (so you're linking users to a 404)

Am about to test the options array.

[neilferreira]

This next problem you might want to fix asap,

in 'getObjectUrl' when you call 'get_object_url' you've missed a parameter

public function get_object_url($bucket, $filename, $preauth = 0, $opt = null)

You're passing your options inside the preauth variable, other than that, your ssl parameter is working fine.

I did ask about a seperate issue related to his back in issue 66 which I'm hoping I can get your help on

What I did just notice though is that my content is being served via:
http://[mybucket].s3.amazonaws.com/compositeWelcomeBack/composite/819237ea4e16337558cc9cb6ecf4be06.jpg

Weirdly adding https:// to this url, this throws up a bug SSL warning, there is a certficate for '*.s3.amazonaws.com' but my browser doesn't appear to like it, at all (Chrome + FF)

If you try:
$this->storage->get_object_url($this->bucket, $objectPath, 0, array('https' => true));

You'll notice that is returns the exact same URL, with https:// prepended to it, the url amazon generates via AWS console is:
https://s3-ap-southeast-1.amazonaws.com/[mybucket]/compositeWelcomeBack/composite/819237ea4e16337558cc9cb6ecf4be06.jpg

Can anyone comment on whether or not that SSL cert should have validated in the first place? Or an alternative solution to this issue?

As you can see when you use their actual SDK, it returns a URL that no longer appears to have a valid SSL certificate, I don't actually need SSL for the project I'm workign on right now, but I thought i may mention it to you. Have a ponder how you'd like to fix that issue.

[havvg]

Damn; thanks for the fast reply! PR will be up asap.

[havvg]

So, the SSL certificates are not valid? - We should notify the AWS support about that.

[neilferreira]

And last but not least

Can you please add some basic docs on the speed issue, I've just I've just extended the AmazonS3Resolver and added an APC caching layer on top of it and works a treat.

Should the remove function become a protected method now too? Since you'd need to override that too to clear your caching layer that sits on top?

cheers

[neilferreira]

@havvg have you thought about catching the error 'The specified bucket does not exist' inside your store method and reporting it via monolog as an error? At the moment it just fails silently, so a user may not realize that their images are not being served via s3, despite telling it to.

That said, there are plenty of errors we should probably try to catch.

[havvg]

I left the error handling to the user; that's why the resolver is not catching anything (\S3Exception) on its own.
The if_bucket_exists throws the exception from within the constructor; so you won't get to the store method throwing that error, or am I missing something?

[neilferreira]

@havvg I'm not even sure what the deal is with S3 right now.

Take a peek here at my examples

Non SSL works:
http://liip.neilf.net.s3.amazonaws.com/compositeWelcomeBack/composite/ed8bc557c2f9a703c7f2f8766bf5d553.jpg

SSL breaks (invalid cert)
https://liip.neilf.net.s3.amazonaws.com/compositeWelcomeBack/composite/ed8bc557c2f9a703c7f2f8766bf5d553.jpg

It claims 'The certificate is only valid for the following names: *.s3.amazonaws.com , s3.amazonaws.com'

This on the other hand, works:
https://s3.amazonaws.com/liip.neilf.net/compositeWelcomeBack/composite/ed8bc557c2f9a703c7f2f8766bf5d553.jpg

afaik if sometimes 'sg1' (aka singapore) or some location-identifiers are prepended to the URL, so I think doing a straight swap of the URL using regex may cause trouble.

Cheers

[neilferreira]

@havvg you're definitely missing something

If you call getBrowserPath whilst your config is using a bucket that does not exist, there are no errors, If you request that URL that getBrowserPath returns, you see no errors.

[havvg]

I believe the issue with the certificate is your bucket name. It contains dots, which adds more domain levels. I'm not exactly sure on this, but may be the *.s3.amazonaws.com certification does not include *.*.s3.amazonaws.com (and even more levels)?

I'm currently adding unit tests for this resolver.

[neilferreira]

ahh you are correct, it is because of the dots.

I've always added the dots for the purpose of doing a cname on my domain name to remove the .s3.amazonaws part of the domain name,

once I removed the dots it is now returning 'https://s3.amazonaws.com/liip_neilf_net/' - I still think it would be worth stripping of the http: part of the uri, since the method is a 'get browser path' the browser should load the image on the protocol it is currently browsing on. If you disagree I'll just overload that method too, so no biggie.

Cheers

[havvg]

Doesn't the https option work in this case?

[neilferreira]

The https option does work, as it does return a secure URL. The problem with that is that it will return secure links ALL the time (if the config variable is set), not dependent on the current HTTP request's secure status.

I would prefer passing https as false, but embedding the image into the browser with a // prefix, so users browsing via https will use a secure link but users browsing via http will have the non ssl url embedded, as afaik ssl does slow the request down a tad.

My biggest use case for this would be facebook apps, when doing a facebook app you MUST have SSL on your domain name, altough less than 50% of users will actually browse your website via SSL, embedding the images hosted on S3 with a https:// prefix (because thats how it has been setup in your service config) for the non-ssl users seem pointless to me.

Am I making any sense here? :)

[havvg]

Yes, now I got your issue here :)

For now I would suggest to overwrite the getBrowserPath because I can't come up with a clean solution for it.
If someone sets the https flag (or even torrent), removing the protocol may fail to satisfy the expectations for it.

This could be a scenario for the $absolute argument of getBrowserPath, need to think a bit about it :)

[neilferreira]

Thats fine, I will override it.

Another thing to bring to your attention

        $this->storage = $storage;
        $this->storage->if_bucket_exists($bucket);

That snippet is from your constructor within AmazonS3Resolver, on my dev machine that takes between 1.5 and 2.5 seconds to run, this is fairly bad because now ANY page on the entire site, or any CLI script has a bootup time of 1.5 seconds, that function call is unnecessary, as I hinted before you could just catch the errors when you attempt to do a store, and log them from there, that way you're only ever connecting to s3 when needed.

With the current setup, the moment services.yml is loaded you're automatically doing a connection to S3, which takes a fair bit of time.

If you can push up a fix for this soonish that would be much appreciated.

[havvg]

Good point, I will remove it from the constructor.