Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Filesystem] Consider the umask setting when dumping a file#19872
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedSep 7, 2016
hboomsma commentedSep 7, 2016
@nicolas-grekas I original also added the part that would listen to the umask, but that would have been a BC break, so I left it out. I do support the idea of adding it. 👍 |
| // Ignore for filesystems that do not support umask | ||
| @chmod($tmpFile,0666); | ||
| @chmod($tmpFile,0666 & ~umask()); |
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.
Can you also adjust the test case to test this behavior? (be aware of windows not having umask)
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.
There is nothing I could adjust, because the Symfony unit test suite explicitly sets the umask to0, so the expected file permissions in the tests will always be666 on both Unix and Windows.
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.
@hboomsma Ah, I see. Thank you. 😄
nicolas-grekas commentedSep 8, 2016
@leofeyer can you please rebase so that the merge commit goes away? |
leofeyer commentedSep 8, 2016
Done. |
nicolas-grekas commentedSep 11, 2016
I'd be 👍 here, looks consistent to me. |
jakzal commentedSep 12, 2016
👍 |
1 similar comment
hboomsma commentedSep 12, 2016
👍 |
fabpot commentedSep 14, 2016
Thank you@leofeyer. |
…e (leofeyer)This PR was merged into the 3.1 branch.Discussion----------[Filesystem] Consider the umask setting when dumping a file| Q | A| ------------- | ---| Branch? | 3.1| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#14246 ,#17063| License | MIT| Doc PR | -`Filesystem::dumpFile()` does not consider the umask setting and thus creates files with write permissions for everyone (`0666`). Other `chmod()` calls in Symfony do consider the umask setting (see e.g. [here](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/File/File.php#L101) or [here](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/File/UploadedFile.php#L230)), therefore I have adjusted the `dumpFile()` method accordingly.Commits-------fdd266f Consider the umask setting when dumping a file.
Uh oh!
There was an error while loading.Please reload this page.
Filesystem::dumpFile()does not consider the umask setting and thus creates files with write permissions for everyone (0666). Otherchmod()calls in Symfony do consider the umask setting (see e.g.here orhere), therefore I have adjusted thedumpFile()method accordingly.