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

Make local volumes respect defaultFileMode and defaultDirMode #4251

Merged
merged 1 commit into from
Jun 1, 2020

Conversation

jeffturcotte
Copy link
Contributor

This patch makes Local volumes respect the defaultFileMode and defaultDirMode config settings by passing them into the Flysystem adapter. The documentation reads as if this was the intended behavior.

When a mode isn't set, it defaults back to what Flysystem uses internally: 0755 and 0644.

I couldn't find any existing reports regarding this issue.

Thanks!

@jeffturcotte
Copy link
Contributor Author

Any thoughts on this yet? We are currently using a somewhat-hacky module to get around this limitation: https://github.com/imarc/craft-volume-permissions. We had to use Reflection to access and modify the Flysystem defaults.

Without the ability to set the proper default file permissions for the local volume adapter, Craft will not play nicely in hosting environments using filesystem ACLs, which almost always requires having a group writable permission.

Thanks.

@andris-sevcenko
Copy link
Contributor

Ugh. Sorry. GitHub really needs to remind about inactive pull requests. Or, you know, maybe I shouldn't forget about stuff.

I just set a reminder to merge this in on Monday. Sorry again - I completely dropped the ball on this.

@jeffturcotte
Copy link
Contributor Author

No worries @andris-sevcenko. Thank you!

@andris-sevcenko andris-sevcenko merged commit 2e80b9f into craftcms:develop Jun 1, 2020
@andris-sevcenko
Copy link
Contributor

Thank you for your patience!

@brandonkelly
Copy link
Member

Craft 3.4.23 is out now with this change.

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