search

silverstripe / silverstripe-s3

Silverstripe module to store assets in S3 rather than on the local filesystem (SS4/SS5 only)
BSD 3-Clause "New" or "Revised" License
20 stars 25 forks source link

Feedback on 0.1.0 #1

Closed Firesphere closed 7 years ago

Firesphere commented 7 years ago
sminnee commented 7 years ago

Yeah — use a namespace and use PSR-4 class loader.

Before namespacing your code you may want to talk to TSP about whether this will be our officially supported module (which would make sense); in which case the package name might be silverstripe/s3assets and the namespace SilverStripe\S3Assets

sminnee commented 7 years ago

It'd be better to have all the getenv('SS_AWS_S3_KEY') checks in a _config.php

I think it would be better still to use the backtick syntax in config yaml, using these variables to configure a default Injector service. Maybe ask @tractorcow for if/how this works.

madmatt commented 7 years ago

At https://github.com/madmatt/silverstripe-s3/blob/master/src/SilverStripeS3AdapterTrait.php#L4, why is it not namespaced? Any specific reason?

Ah, because it was previously in a single file that was namespaced, and I copied it out into it's own file and forgot the namespace declaration. Fixed in 0.2.0.

Dockblocks are missing

I've added a couple, I'm not sure about re-working the trait yet so haven't documented that as I expect it will change.

It'd be better to have all the getenv('SS_AWS_S3_KEY') checks in a _config.php, breaking out of functioning when it's not set, instead of checking it every time in the Trait

The way it currently works is a bit more performant because doing it in _config.php means that it will get executed on every request (whereas now it only gets called when an asset is actually required - e.g. it's not even used when accessing /admin/assets, just when calling the graphql endpoint to get data).

However I'd like a better way to specify this too so it doesn't need a whole trait just for configuration. I'll have a think.

madmatt commented 7 years ago

Oops, didn't mean to close this. Thanks for the comments @Firesphere and @sminnee.

Firesphere commented 7 years ago

Hmmm, not checking in _config.php actually makes sense. I'm off to YubiAuth to fix this!

madmatt commented 7 years ago

With the repository move I'll close this for now, the only remaining thing was where the getenv() calls live, which I'm okay with living in the adapter rather than _config for now.