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

Skip chmod if permission is already the same #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phena109
Copy link

Q A
Is bugfix?
New feature?
Breaks BC?
Fixed issues comma-separated list of tickets # fixed by the PR, if any

In the original code if chmod is not permitted, php 7.4 throws a warning and the exception actually won't get thrown.

If we need to ensureDirectory an existing directory (which the developer may not know it existed and with sensitive permission), it will overwrite the original permission which may not be desired.

@samdark
Copy link
Member

samdark commented Mar 15, 2021

What do you mean by "sensitive permission"?

@@ -93,6 +93,11 @@ public static function ensureDirectory(string $path, int $mode = 0775): void
restore_error_handler();
}

// Skip chmod if permission is already the same
if (fileperms($path) == $mode) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (fileperms($path) == $mode) {
if (fileperms($path) === $mode) {

?

@samdark samdark added the pr:request for unit tests Unit tests are needed. label Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:request for unit tests Unit tests are needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants