Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Nov 2, 2022

Conversation

samandeggs
Copy link
Contributor

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.

…w to configure the PublicCDNAdapter to support a custom cdn asset path name.

Updating PublicCDNAdapter.php to support a custom path variable, instead of always utilizing /assets/ taken from ASSET_DIR in silverstripe.
.editorconfig Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@michalkleiner michalkleiner merged commit b523d27 into silverstripe:main Nov 2, 2022
@samandeggs
Copy link
Contributor Author

Thanks @michalkleiner for assisting me here.

@samandeggs
Copy link
Contributor Author

@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
Copy link
Member

madmatt commented Nov 3, 2022

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
Copy link

@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
Copy link
Contributor Author

Thanks @madmatt and thanks again @michalkleiner

@madmatt
Copy link
Member

madmatt commented Nov 5, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants