Adds protocol relative URL support to the S3Link view helper.

[elyobo]

Protocol relative URLs (e.g. "//my-bucket.s3.amazonaws.com/my-object") are useful to make an efficient use of your S3 resources; on secure pages, secure objects will be requested and on insecure pages, insecure objects will be requested.

Backwards compatibility is maintained in this patch, with HTTPS links remaining the default and the setUseSsl() and getUseSsl() functions remaining. I have marked them deprecated as using ssl or not is no longer a binary decision after this patch, as three schemes (https, http and protocol relative) are now supported and so setScheme() and getScheme() are more appropriate.

[elyobo]

If you're happy with the idea and approach, I'll update with the same implementation for cloudfront.

[jeremeamia]

Seems fine to me, but I'd like to hear what @gwis and @bakura10 think.

[bakura10]

I like the idea, however I don't understand why setSsl would be marked as deprecated. I can't see exactly where, but it could be useful in some cases to force a particular scheme.

[gws]

Wouldn't setScheme be used to force a particular scheme?

[elyobo]

@bakura10 , @gwis has the idea; setScheme('https') can still be used to set SSL. I've marked it as deprecated because it's no longer a binary ssl/not ssl situation and so a boolean toggle (setUseSsl()) isn't really appropriate. The setScheme() function provides the same functionality in the end.

[elyobo]

Updated to throw an InvalidSchemeException when invalid schemes are set and to permit only null, not the empty string (which generates invalid URLs).

[bakura10]

Ha yeah, you're right about setScheme!

[bakura10]

Looks good to merge @jeremeamia

[elyobo]

Hang on, if you're happy to merge, would you like me to implement the same changes for the cloudfront helper before merging? They might as well be consistent and come through in the same patch.

And folks here do seem to care about the style, so my inclination would be to create an AbstractLinkHelper which handles the common parts for S3 and CloudFront links (client, scheme) and leaves the specifics to the children, rather than copy-pasting across the code. Preferences?

[jeremeamia]

Yeah, I think that would be great! An AbstractLinkHelper makes sense as well. Nice work. I'll submit this PR to our internal legal approval and get a CLA to you to sign (if they require it). All submissions are made under the Apache 2.0 license.

[elyobo]

Unfortunately the CloudFrontClient from the core AWS PHP SDK does not support protocol relative URL signed URLs, so my implemented version likewise rejects this.

I'm not too familiar with CloudFront; is this a limitation of the signing system, or is just a problem with the code (i.e. if I submit a fix for the handling to permit null schemes, will it generate valid links, or will it still be invalid)?

[jeremeamia]

I'm not totally sure. I'll have to look into this and get back to you. There is some funniness in handling the scheme since http(s) and rtmp urls are handled differently.

[elyobo]

OK, I've pushed support for CloudFront and added tests. I throw an InvalidSchemeException for now if they try to sign a protocol relative URL.

For merging, do you prefer this to be squashed down to a single commit?

[jeremeamia]

Yes, please squash. It doesn't necessarily have to be 1 commit though. Thanks for your work on this. I'll look more into the CloudFront thing tomorrow.

[gws]

I had a couple tiny comments. I should've associated them with the PR rather than the commit object - my bad :(

Looks good so far!

[elyobo]

@gwis, thanks for those, I've fixed and pushed up a squashed version.

I didn't see a code standard mentioned anywhere; it would be useful for this to be documented so that contributors can use PHPCS to check that we're doing things right. It's too easy to make small mistakes otherwise!

[gws]

Makes sense - @jeremeamia might have a preference on this. I just noted a few inconsistencies as I read over the PR, but I certainly don't want CS bikeshedding (beyond a certain point) to be a barrier to work getting done :)

[jeremeamia]

I'll add a CONTRIBUTING.md soon. @elyobo Please send an email to the address on my GitHub profile and I'll get the CLA to you to sign.

[jeremeamia]

CLA received. Waiting for final legal approval before merging. Thanks.

[jeremeamia]

@elyobo There ended up being a failing test that I had to fix, but everything is working in master now. I also added TravisCI support and a CONTRIBUTING.md document.

[elyobo]

Great, thanks @jeremeamia.