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

modify PublicCDNAdapter for Flysystem v3 #65

Closed Fexiven closed 11 months ago

Fexiven commented 11 months ago

https://github.com/silverstripe/silverstripe-s3/pull/63 forgot to also update the PublicCDNAdapter.

I set the values by default to null because im not sure what functionality we need here

The problem was, without this change, the constructor would set $options as VisibilityConverter

obj63mc commented 11 months ago

Looks good to me. @GuySartorelli would you mind releasing this?

GuySartorelli commented 11 months ago

@obj63mc I don't have any way to test this, so I won't be merging it. Once someone with merge access has reviewed, tested, and merged this PR I'll be happy to tag a release, assuming the person who merges it isn't able to do so.

obj63mc commented 11 months ago

I have tested this and all is working as it should. Only changes were updating the constructor signature due to flysystem v3 changing the signature for the AwsS3V3Adapter.

GuySartorelli commented 11 months ago

@obj63mc Would you like a new major release (this does technically violate semver if it's not a major release) - or just a minor?

obj63mc commented 11 months ago

I am fine with whatever. Essentially this should have been caught when upgrading everything to flysystem v3 but was missed

GuySartorelli commented 11 months ago

I'll go with the semantically correct version then, and tag as 3.0.0

GuySartorelli commented 11 months ago

https://github.com/silverstripe/silverstripe-s3/releases/tag/3.0.0