dumpFile(), preserve existing file permissions #21823
Conversation
| @@ -646,7 +646,12 @@ public function dumpFile($filename, $content) | |||
| throw new IOException(sprintf('Failed to write file "%s".', $filename), 0, null, $filename); | |||
| } | |||
|
|
|||
| @chmod($tmpFile, 0666 & ~umask()); | |||
There was a problem hiding this comment.
what about using the ternary op?
@chmod($tmpFile, !file_exists($filename) ? 0666 & ~umask() : fileperms($filename))
There was a problem hiding this comment.
It may depend on what is considered the most readable.
By the way, what about $this -> chmod() instead of chmod() ?
There was a problem hiding this comment.
$this->chmod wouldn't provide much more to me, except the exception on error, but I don't see it as needed here
There was a problem hiding this comment.
I like the ternary op because it's not too long ... but I'd check a positive condition because negative conditions are less readable:
@chmod($tmpFile, file_exists($filename) ? fileperms($filename) : 0666 & ~umask());|
@chs2 Can you change the target branch to 2.7 (and resolve the conflicts there)? |
|
Resolving the conflict will take a little time. |
|
The easiest is probably to close this PR and open a new one :) |
When calling Filesystem::dumpFile() on an already existing file, its permissions are lost.
|
@chs2 I rebased your PR for you. |
|
Thank you @chs2. |
This PR was squashed before being merged into the 2.7 branch (closes #21823). Discussion ---------- dumpFile(), preserve existing file permissions When calling Filesystem::dumpFile() on an already existing file, its permissions are lost. | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> <!-- - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the master branch. - Please fill in this template according to the PR you're about to submit. - Replace this comment by a description of what your PR is solving. --> Commits ------- 040a9ab dumpFile(), preserve existing file permissions
|
see #21902 that resolves the failing test case |
…abbuh) This PR was merged into the 2.7 branch. Discussion ---------- [Filesystem] respect the umask argument in dumpFile() | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21823 | License | MIT | Doc PR | In #21823 we introduced a small BC break: The `dumpFile()` method of the `Filesystem` class allowed to pass the desired file permissions (support for this feature was dropped in Symfony 3.0). Commits ------- 3a7cd08 respect the umask argument in dumpFile()
When calling Filesystem::dumpFile() on an already existing file, its permissions are lost.