-
Notifications
You must be signed in to change notification settings - Fork 104
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
[!!!][BUGFIX] Set permissions correctly #4
Conversation
Thanks. We'll take a look at this shortly. |
); | ||
|
||
$originalVisibility === AdapterInterface::VISIBILITY_PUBLIC ? $this->publishObject($newpath) : $this->unPublishObject($newpath); |
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.
Here this code uses strict comparisons and a ternary to call the functions and then later on here, we use an if-else.
I think we should use strict comparisons everywhere, if possible (even if we aren't already), and then call functions in an if-else, because it's easier to read.
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.
Agreed, will fix asap.
I have ameded the changeset to your suggestions. Please let me know if I got everything right. |
All the changes look good, can we just use an if-else here please, so it reads a little easier. Something like if ($originalVisibility === AdapterInterface::VISIBILITY_PUBLIC) {
$this->publishObject($newpath);
} else {
$this->unPublishObject($newpath);
} 👍 after that |
The change will allow graceful fallback to the default bucket permissions for new objects to avoid users not being able to operate on file ACLs via other tools (for example gsutil or the cloud console). It also removes the semantics of `private` and `publicRead` CloudStorage API predefined ACLs in favour of default bucket permissions and properly created additional ACLs on visibility change. Fixes: #3
Ah I missed / misinterpreted that one. Done. :) |
Awesome, thanks. 👍. There are just some minor linting issues, but I'll take care of those. |
[BUGFIX] Set permissions correctly
Thanks for taking care! :) Suggestion: Make Code Style issues fail the build, as a contributor I'd be glad about it. |
My pleasure. :) I agree. Scrutinizer is supposed to lint it after a travis build, but it doesn't seem to be complaining, but I'll look into it tomorrow, before we do a release. |
The change will allow graceful fallback to the default bucket
permissions for new objects to avoid users not being able to
operate on file ACLs via other tools (for example gsutil or
the cloud console).
It also removes the semantics of
private
andpublicRead
CloudStorage API predefined ACLs in favour of default bucket
permissions and properly created additional ACLs on
visibility change.
Fixes: #3