-
Notifications
You must be signed in to change notification settings - Fork 62
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
Added MinIO Storage adapter #51
base: main
Are you sure you want to change the base?
Conversation
* @param string $region | ||
* @param string $acl | ||
*/ | ||
public function __construct(string $root, string $accessKey, string $secretKey, string $protocol, string $host, string $bucket, string $region = self::EU_CENTRAL_1, string $acl = self::ACL_PRIVATE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function __construct(string $root, string $accessKey, string $secretKey, string $protocol, string $host, string $bucket, string $region = self::EU_CENTRAL_1, string $acl = self::ACL_PRIVATE) | |
public function __construct(string $root, string $accessKey, string $secretKey, string $host, string $bucket, string $region = self::EU_CENTRAL_1, string $acl = self::ACL_PRIVATE) |
https://github.com/minio/minio-js/blob/master/src/main/minio.js#L70
Please avoid changing the signature of the constructor. These days it is very uncommon to use HTTP so let's default to HTTPS like the official SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @shimonewman,
thanks for the review. The problem is that in my setup MinIO is in a Self Hosted Private Network
so I access it with the internal IP so it don't have an https protocol.
* @param string $root | ||
* @param string $accessKey | ||
* @param string $secretKey | ||
* @param string $protocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param string $protocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protocol has to be here, because MinIO can be hosted in an internal Network, where you don't have SSL
src/Storage/Device/S3.php
Outdated
@@ -659,9 +660,9 @@ private function getSignatureV4(string $method, string $uri, array $parameters = | |||
* | |||
* @return object | |||
*/ | |||
private function call(string $method, string $uri, string $data = '', array $parameters=[]) | |||
public function call(string $method, string $uri, string $data = '', array $parameters=[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function call(string $method, string $uri, string $data = '', array $parameters=[]) | |
private function call(string $method, string $uri, string $data = '', array $parameters=[]) |
Please avoid changing the access modifiers
src/Storage/Device/MinIO.php
Outdated
* | ||
* @return array | ||
*/ | ||
public function listObjects($prefix = '', $maxKeys = 1000, $continuationToken = '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function listObjects($prefix = '', $maxKeys = 1000, $continuationToken = '') | |
private function listObjects($prefix = '', $maxKeys = 1000, $continuationToken = '') |
Please change S3 listObjects() access modifiers to protected in order for you to override it.
$this->root = 'minio-test-bucket'; | ||
$key = $_SERVER['MINIO_ACCESS_KEY'] ?? ''; | ||
$secret = $_SERVER['MINIO_SECRET'] ?? ''; | ||
$protocol = $_SERVER['MINIO_PROTOCOL'] ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$protocol = $_SERVER['MINIO_PROTOCOL'] ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protocol has to be here, because MinIO can be hosted in an internal Network, where you don't have SSL
$host = $_SERVER['MINIO_HOST'] ?? ''; | ||
$bucket = 'minio-test-bucket'; | ||
|
||
$this->object = new MinIO($this->root, $key, $secret, $protocol, $host, $bucket, MinIO::EU_CENTRAL_1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->object = new MinIO($this->root, $key, $secret, $protocol, $host, $bucket, MinIO::EU_CENTRAL_1); | |
$this->object = new MinIO($this->root, $key, $secret, $host, $bucket, MinIO::EU_CENTRAL_1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protocol has to be here, because MinIO can be hosted in an internal Network, where you don't have SSL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!! few more changes and then we can merge
* @param string $region | ||
* @param string $acl | ||
*/ | ||
public function __construct(string $root, string $accessKey, string $secretKey, string $protocol, string $host, string $bucket, string $region = self::EU_CENTRAL_1, string $acl = self::ACL_PRIVATE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to adapt all adapters to use and endpoint instead of protocol and hostname for more flexibility. Hardcoding https://
is wrong to begin with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would leave it for now and make it in another Pull Request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@groschi24, please create another PR with the proposed changes to the endpoint.
@groschi24 sorry looks like there are merge conflicts now. Would you be able to fix the merge conflicts? |
Any hope that this will be merged soon? |
What does this PR do?
Implements a new Adapter for MinIO.
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Implemented new Adapter, tested with
vendor/bin/phpunit
and custom function test ( code below )Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Issue #37
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Yes