Filesystem::write() accept NULL in $mode parameter#139
Conversation
|
It breaks compatibility with PHP 7.0 |
|
Ping @jkuchar |
src/Utils/FileSystem.php
Outdated
|
|
||
| /** | ||
| * Writes a string to a file. | ||
| * @param $mode int|NULL |
There was a problem hiding this comment.
Not sure how to write php doc here as you usually do not use @param with $variableName.
There was a problem hiding this comment.
Most importantly you switched parameter name with parameter type, the annotation should be @param int|NULL $mode
src/Utils/FileSystem.php
Outdated
| public static function write(string $file, string $content, $mode = 0666) | ||
| { | ||
| assert($mode === NULL || is_int($mode)); | ||
|
|
There was a problem hiding this comment.
I usually use assert when there is no possibility of declaring type using typehint (union types, ...) Do you like it or should it fail at line 144
|
@JanTvrdik thanks for review, updated |
src/Utils/FileSystem.php
Outdated
|
|
||
| /** | ||
| * Writes a string to a file. | ||
| * @param int|NULL $mode |
There was a problem hiding this comment.
It does not look obvious what this parameter does... Is that creation mode or does it override current file mode?
Maybe add to docs something like:
- Alters file mode to given value (does not matter if file already or will be created exists)
Shouldn't NULL be the default value? created follow up in #140 and PR #141
There was a problem hiding this comment.
There should be two spaces after @param to align with @return and @throws.
|
@JanTvrdik Should I squash this, have you finished review? |
|
@JanTvrdik Added the same fix also for |
|
@jkuchar Hopefully not. |
|
How differs |
This reverts commit e5ecd37.
|
TL;DR: @dg you are right. NULL should be prohibited as it is evaluated to I have performed the following test. mkdir("testFromPHPNULL", NULL);
mkdir("testFromPHP0777", 0777);
exec("mkdir -m 0777 testFromBash0777");
exec("mkdir testFromBashDefault");Tried this obscure situation with ACLs: $ getfacl .
# file: .
# owner: jkuchar1a
# group: domain^users
user::rwx
group::r-x #effective:---
mask::---
other::---
default:user::rwx
default:group::r-x #effective:---
default:mask::---
default:other::---$ ll
drwx------+ 2 jkuchar1a domain^users 4096 čen 9 19:40 testFromBashDefault/
drwx------+ 2 jkuchar1a domain^users 4096 čen 9 19:40 testFromBash0777/
d---------+ 2 jkuchar1a domain^users 4096 čen 9 19:40 testFromPHPNULL/ <--- WRONG
drwx------+ 2 jkuchar1a domain^users 4096 čen 9 19:40 testFromPHP0777/Result permissions on testFromBashDefault and testFromPHP0777 are the same. When no ACLs are used it behaves the same. $ ll
drwxr-xr-x 2 jkuchar1a domain^users 4096 čen 9 19:46 ./
drwxr-xr-x 12 jkuchar1a domain^users 4096 čen 9 19:26 ../drwxr-xr-x 2 jkuchar1a domain^users 4096 čen 9 19:50 testFromBashDefault/
drwxrwxrwx 2 jkuchar1a domain^users 4096 čen 9 19:50 testFromBash0777/ <--- WRONG
d--------- 2 jkuchar1a domain^users 4096 čen 9 19:50 testFromPHPNULL/ <--- WRONG
drwxr-xr-x 2 jkuchar1a domain^users 4096 čen 9 19:50 testFromPHP0777/ |
|
Thanks |
3 participants
Code of function expects
$modeto beNULL. If that happens it does notchmodit.