If I understand it correctly, there has been a deprecation added in 2.8 which was never merged into 3.0 and the code was never fixed properly there. If you take a look at thehistory of Filesystem.php:

• The 3rd option is marked as deprecated
• The default value is0666
• If the value is notnull, it will set the permissions
• By passing 2 arguments you willnot trigger the deprecation notice but it will still set the permissions to0666

This means that the only case where no permissions would be set, would be by passingnull as third argument which was deprecated, but that's the exact behavior 3.0 has right now.

When you take a look at the3.0 Filesystem.php, you can see that the permissions are not longer fixed. The only non-deprecated way, which is setting it0666 afterwards is broken in 3.0.

/cc@xabbuh

@iltar I think your reasoning is correct and we need to always set the permissions as proposed here.

By the way, the solution before Symfony 3.0 was not really atomic, was it? I mean thechmod() call was done after renaming the file. Shouldn't this happened the other way around (like proposed here)?

@iltar, this is indeed the case.

Code that ran in 2.8 without any deprication message stops working in 3.0 because thetempnam function of php creates files with access permissions set to 0600. In 2.8 this was fixed by running a chmod with 0666 (ignoring system umask). In 3.0 however, nothing is done and only the creating user can read the file.

@xabbuh I agree that the permissions should be changed after writing as@cs278 pointed out and before renaming to make this operation atomic.

@hboomsma Would you like to create a pull request for older Symfony versions that swaps the order of the calls?

@xabbuh sure, which versions should we patch? I'll create a pull request for 2.8 right away.

Hm, I think we will get other issues when merging this on filesystems that don't allow to change permissions (see#8205).

@xabbuh you are right, I'll fix it in the same way, it was fixed in the mentioned issue, because ifchmod is not supportedtempnam will not mess with permissions either.

As for the request for 2.8 do you have an open issue for me to mention in the PR?

@hboomsma I think you can refer to the same issue (it's also talking about the (not)-atomic behaviour).

@xabbuh#17184 for atomic behaviour in 2.8

Looks like the failure is unrelated to the changes in this PR.

@xabbuh@cs278@iltar Updated with changes done in 2.3 and merged upward for atomic behaviour

@xabbuh updated PR to not take the umask into account.

@xabbuh updated pr to not enhance the unit tests, I will create a separate PR for taking the umask into account with the extra tests for 2.3 after this PR is finished.

👍

Status: Reviewed

Thank you@hboomsma.