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

Adding param to PublicCDNAdapter to allow for custom asset path folder name. #56

Closed samandeggs closed 2 years ago

samandeggs commented 2 years ago

Default path extension of /assets/ doesn't always line up to desired S3 bucket path/origin path, and causes unnecessary issues when this can be easily overridden. Must be fully overridden in the config, but allows for edge-case handling of this path naming.

Just have to redeclare the following config:

---
Name: silverstripes3-cdn
Only:
  envvarset: AWS_PUBLIC_CDN_PREFIX
After:
  - "#assetsflysystem"
  - "#silverstripes3-flysystem"
---
SilverStripe\Core\Injector\Injector:
  SilverStripe\S3\Adapter\PublicAdapter:
    class: SilverStripe\S3\Adapter\PublicCDNAdapter
    constructor:
      s3Client: '%$Aws\S3\S3Client'
      bucket: "`AWS_BUCKET_NAME`"
      prefix: "`AWS_PUBLIC_BUCKET_PREFIX`"
      cdnPrefix: "`AWS_PUBLIC_CDN_PREFIX`"
      cdnAssetPath: "public" # example of altered path name, which will produce https://cdn.example.com/public/Uploads/file.jpg

Have updated the README.md to reflect this, as well as ripping out what appeared to be accidental inclusion of >> HEAD comments from git, as well as adjusting the .editorconfig to have 2 space tabs instead of 4, which was causing issues with the appearance of the .yml and .json code examples.

samandeggs commented 2 years ago

Thanks @michalkleiner for assisting me here.

samandeggs commented 2 years ago

@madmatt could I ask is there anything required to expedite this PR into a release? Would be keen to use this change in a production environment on my side without having to use dev-main

madmatt commented 2 years ago

Hey @samandeggs, no worries - I've just released version 1.3.0 now. Thanks for making these changes, thanks for including docs, and thanks @michalkleiner for reviewing!

michalkleiner commented 2 years ago

@madmatt since we changed the constructor I wasn't sure whether we wanted this as 2.0 or 1.3, so didn't tag a release myself. I guess this is one of the modules where the strict semver commitment wasn't as strong as it probably could/should.

samandeggs commented 2 years ago

Thanks @madmatt and thanks again @michalkleiner

madmatt commented 2 years ago

Good point @michalkleiner, I read the change and saw that it’s backwards compatible so figured it’s fine as a minor release. If anyone is overriding the constructor with a subclass that would need it’s signature changed to avoid strictness warnings, that seems simple enough - I take your point though that maybe 2.0.0 would have been more accurate of a version number despite how simple the change was.