Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filesystem::write() accept NULL in $mode parameter #139

Merged
merged 7 commits into from Jun 12, 2017
Merged

Conversation

jkuchar
Copy link
Contributor

@jkuchar jkuchar commented May 9, 2017

  • bug fix? yes
  • new feature? no
  • BC break? no (fixing unintended Nette 2.x BC break)

Code of function expects $mode to be NULL. If that happens it does not chmod it.

@dg
Copy link
Member

@dg dg commented May 9, 2017

It breaks compatibility with PHP 7.0

@dg
Copy link
Member

@dg dg commented May 17, 2017

Ping @jkuchar

@@ -131,11 +131,14 @@ public static function read(string $file): string

/**
* Writes a string to a file.
* @param $mode int|NULL
Copy link
Contributor Author

@jkuchar jkuchar Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to write php doc here as you usually do not use @param with $variableName.

Copy link
Contributor

@JanTvrdik JanTvrdik Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most importantly you switched parameter name with parameter type, the annotation should be @param int|NULL $mode

{
assert($mode === NULL || is_int($mode));

Copy link
Contributor Author

@jkuchar jkuchar Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@JanTvrdik JanTvrdik Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unnecessary to me.

@jkuchar
Copy link
Contributor Author

@jkuchar jkuchar commented Jun 6, 2017

@JanTvrdik thanks for review, updated

@@ -131,10 +131,11 @@ public static function read(string $file): string

/**
* Writes a string to a file.
* @param int|NULL $mode
Copy link
Contributor Author

@jkuchar jkuchar Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@JanTvrdik JanTvrdik Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be two spaces after @param to align with @return and @throws.

@jkuchar
Copy link
Contributor Author

@jkuchar jkuchar commented Jun 8, 2017

@JanTvrdik Should I squash this, have you finished review?

@jkuchar
Copy link
Contributor Author

@jkuchar jkuchar commented Jun 8, 2017

@JanTvrdik Added the same fix also for createDir() e5ecd37 Anything more that needs to be fixed?

@JanTvrdik
Copy link
Contributor

@JanTvrdik JanTvrdik commented Jun 9, 2017

@jkuchar Hopefully not.

@dg
Copy link
Member

@dg dg commented Jun 9, 2017

How differs mkdir($dir, 0777) from mkdir($dir, NULL)?

This reverts commit e5ecd37.
@jkuchar
Copy link
Contributor Author

@jkuchar jkuchar commented Jun 9, 2017

TL;DR: @dg you are right. NULL should be prohibited as it is evaluated to 0000 permission. Good catch! Interesting is that when mkdir is called with 0777 it is ignored and creates directory with default directory permissions. (is that somehow related to umask?)

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/

@dg
Copy link
Member

@dg dg commented Jun 12, 2017

Thanks

@dg dg merged commit c01d1d1 into nette:master Jun 12, 2017
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants