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

modify PublicCDNAdapter for Flysystem v3 #65

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Conversation

Fexiven
Copy link
Contributor

@Fexiven Fexiven commented Nov 30, 2023

#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

@Fexiven Fexiven force-pushed the main branch 2 times, most recently from f3aa08c to e7e37de Compare November 30, 2023 08:19
@obj63mc
Copy link
Collaborator

obj63mc commented Nov 30, 2023

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

@GuySartorelli
Copy link
Member

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

obj63mc commented Nov 30, 2023

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.

@obj63mc obj63mc merged commit db33c6e into silverstripe:main Nov 30, 2023
@GuySartorelli
Copy link
Member

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

obj63mc commented Dec 1, 2023

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

@GuySartorelli
Copy link
Member

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

@GuySartorelli
Copy link
Member

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