update default cache permissions, and allow users to define their own - #854

The thing I absolutely don't like is the unmotivated array mixture of the permissions. That is yelling "YAGNI" in my ears, i.e. it looks like it is waiting for future extensions that probably will never happen.

You have designed the cachePermissions call with dedicated values for files and directories, and then put them into an array that isn't needed for the current code, but still requires the array access for file and dir, and pass the array into the methods for dir and file creation, that both only need exactly one of these values.

I am also unsure why you are not adding these new parameters into enableCompilation, as the setting standalone does not make sense without compiling the container. Making them optional parameters with the old values would keep compatibility with previous versions.

I cannot really argue about the need for this enhancement. File permissions as well as file/group ownership is a tricky thing, and any fixed choice is likely wrong in at least some situations. I would assume any user creating a compiled container already has some deployment process in place where the compilation step is happening before the code is pushed live, and probably some intermediate step is correcting the file permissions and ownership to the requested level of security anyways - contradicting myself as this assumption is also wrong in at least some situations. I'll leave that up for someone else to consider.

The thing I absolutely don't like is the unmotivated array mixture of the permissions. That is yelling "YAGNI" in my ears, i.e. it looks like it is waiting for future extensions that probably will never happen.

You have designed the cachePermissions call with dedicated values for files and directories, and then put them into an array that isn't needed for the current code, but still requires the array access for file and dir, and pass the array into the methods for dir and file creation, that both only need exactly one of these values.

Originally I had included changes that also allowed a user to define group permissions, which meant I was passing more than one permission along. I didn't end up including those changes as part of this PR. That said, I've updated the PR to remove the use of an array.

I am also unsure why you are not adding these new parameters into enableCompilation, as the setting standalone does not make sense without compiling the container. Making them optional parameters with the old values would keep compatibility with previous versions.

As you noted, to avoid breaking existing implementations, the optional arguments would need to be tacked to the end of the function signature. However, this would require users to define what I suspect are rarely-changed values, in order to reach those arguments. I felt a standalone method for setting these permissions more closely aligned with the current design pattern of the container builder, and would be least impactful now, and in the future.

However, this would require users to define what I suspect are rarely-changed values, in order to reach those arguments.

An additional parameter could be added without specifying unneeded values using named arguments:

$builder->enableCompilation($directory, filePermissions: 0644, directoryPermissions: 0755);

A way to use just one parameter would be to allow the user to specify a mask. The default would be 0. Setting a mask of 022 would achieve the same purpose by removing the write bits.

You would apply the mask on the existing hardcoded values using &.

chmod($tmpFile, 0666 & $umask);

Could we just change the default permissions to 0775 and 0664?

That would be a much simpler PR, anything we can do to add more options is welcome.