search

yii2tech / file-storage

This package has been abandoned. See: https://www.patreon.com/posts/59332556
Other
119 stars 30 forks source link

Allow server side encryption on S3 objects #18

Closed SonOfHarris closed 6 years ago

SonOfHarris commented 6 years ago
klimov-paul commented 6 years ago

First of all, do not mix 2 different features into a single pull request. If you wish to help - open a separated pull request for SDK 3.0 support and another one for the encryption.

Secondly, if you wish to make upgrade for SDK 3.0, the support of the previous 2.0 version should be dropped, instead of creating countless if statements in pointless attempt to support both.

Last, but not least: as I already said, I am unable to verify or accept any changes to Amazon support code, since I do not have access to S3 service anymore. If you create a PR for SDK 3.0 support, you will have to wait until other people confirm it is correct, or until I will obtain access for Amazon S3 service from some source.

SonOfHarris commented 6 years ago

Thanks Paul for response and you've made valid points.

I've been having a look around and if you're concerned about maintaining API integrations maybe a solution is to create a generic wrapper for FlySystem (similar to https://github.com/airani/yii2-flysystem). If you're interested let me know and I could submit it as an issue.

klimov-paul commented 6 years ago

Creating a wrapper around "league/flysystem" within "yii2tech/file-storage" makes a little sense. 'FlySystem' itself is a high-level abstraction around file operations, which uses its own terms and approach. It sounds weird to create a wrapper around wrapper. For the developer, who wish to use 'FlySystem' it will be more convenient to use simple wrapper for it, like the one you have mentioned, or even use it directly.

The approach used withing "yii2tech/file-storage" is almost 10 years old. It was created at the times 'FlySystem' has not yet exist. It seems more logical this extension should simply fade away, as it unlikely to have any practical benefit in comparison to 'FlySystem' usage.

Although, I am not familiar enough with 'FlySystem', thus I can not say for sure, whether it make any sense to wrap it here or not. If you think it make sense, please provide the justification for it.

SonOfHarris commented 6 years ago

Thanks Paul. There's no issue in my mind of having a wrapper of a wrapper as people should be coding against the interface. Your library is beneficial especially with the bucket and hub mentality, but I guess there is nothing stopping someone from simply registering multiple application components to mimic the same thing.

There's not much justification I can give apart from helping devs to avoid refactoring their existing code. According to packagist some of the existing flysystem wrappers do appear to have more installs than this library. So the call to let the library fade away is ultimately up to you. If you are deciding to let it go you may want to make it clear within the readme. You could always create an open issue marked as an enhancement and see how the community responds.