-
Notifications
You must be signed in to change notification settings - Fork 68
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
google cloud adapter: allow to inject visibility handler #160
google cloud adapter: allow to inject visibility handler #160
Conversation
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.
That's great! Thank you for your contribution !
/** | ||
* @internal | ||
*/ | ||
class GcloudFactoryPass implements CompilerPassInterface |
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.
Is this CompilerPass necessary? I'm not sure we need it.
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.
Thanks for your feedback.
I initially wanted to declare those services in the GcloudAdapterDefinitionBuilder (as they are only related to the google part), but it's obvious this class isn't where those services should be.
So, I added the services registrations in a CompilerPass to make it obvious it's only related to the google part, and in case other services would be needed for other parts as well (aws for example). So in my mind, it was just a question an organization purpose.
If you rather have the definition elsewhere (in the FlysystemBundle.php I assume), I would gladly modify this.
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 see yes, in the current state of the builders it is not possible to register any other service.
More generally, the creation of storage shoud be reworked to extend the possibility of allowing external builders and also to register services according to the builders activated.
This last case is quite rare, because for exemple for other rights we register a new service like this :
protected function createUnixDefinition(array $permissions, string $defaultVisibilityForDirectories): Definition | |
{ | |
return (new Definition(PortableVisibilityConverter::class)) | |
->setFactory([PortableVisibilityConverter::class, 'fromArray']) | |
->addArgument([ | |
'file' => [ | |
'public' => (int) $permissions['file']['public'], | |
'private' => (int) $permissions['file']['private'], | |
], | |
'dir' => [ | |
'public' => (int) $permissions['dir']['public'], | |
'private' => (int) $permissions['dir']['private'], | |
], | |
]) | |
->addArgument($defaultVisibilityForDirectories) | |
->setShared(false) | |
; | |
} |
And this service is declared as no-shared to manage different rights per storage.
From what I can see for GCloud, it's a bit different. It uses predefined rights for these entities, I don't use GCloud and I don't know if these values can change in relation to its configuration.
So at first lok, I don't see any other way.
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 also don't know about the predefined rights for GCloud, I only worked with GCloud recently in order to use it as a cdn, which has a uniform acl.
Maybe I thought too much about it, and doing as the other builders is enough (meaning creating a new visibility handler per GCloud adapter when it is given in the config), and not having to create 2 others "global" visibility handler services
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.
Don't hesitate if you think you can improve it, in the meantime I'll merge it.
Thanks @syffer ! It's release : https://github.com/thephpleague/flysystem-bundle/releases/tag/3.3.3 |
Hi,
This pr offers to simplify the configuration of the google adapter when there is a need to change the default visibility handler.
For example, when using a bucket with uniform acl, it is required to redefine the google adapter, the bucket, and the visibility handler like so:
This pr would allow to do this instead :
At first, I wanted to add a
VisibilityHandlerGuesser
directlry inthephpleague/flysystem
(by using the bucket's methodinfo
, as described here with theiamConfiguration
, but the client might not have enough rights to do so).