-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Show error message if config file is not readable #185
Conversation
@adsworth the stable9 patch looks like this (because of some refactoring this patch doesn't apply to stable9) diff --git a/lib/private/config.php b/lib/private/config.php
index 368dafd..2a21cd6 100644
--- a/lib/private/config.php
+++ b/lib/private/config.php
@@ -184,10 +184,11 @@ class Config {
// Include file and merge config
foreach ($configFiles as $file) {
- $filePointer = file_exists($file) ? fopen($file, 'r') : false;
+ $fileExistsAndIsReadable = file_exists($file) && is_readable($file);
+ $filePointer = $fileExistsAndIsReadable ? fopen($file, 'r') : false;
if($file === $this->configFilePath &&
- $filePointer === false &&
- @!file_exists($this->configFilePath)) {
+ $filePointer === false) {
// Opening the main config might not be possible, e.g. if the wrong
// permissions are set (likely on a new installation)
continue; |
if($file === $this->configFilePath && | ||
$filePointer === false && | ||
@!file_exists($this->configFilePath)) { | ||
!$fileExistsAndIsReadable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to be redundant, if $fileExistsAndIsReadable
is false, then $filePointer
is already false, see https://github.com/nextcloud/server/pull/185/files#diff-e8ccb791da0aa95230141afcf200336dR188
Btw, the old check didn't checked if the config file exists but if the "configFilePath" exists. Don't know in which cases this makes a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, the old check didn't checked if the config file exists but if the "configFilePath" exists.
$file === $this->configFilePath
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested your proposal and also thought about it and you are correct :) I fixed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, the old check didn't checked if the config file exists but if the "configFilePath" exists.
$file === $this->configFilePath
;)
This just check if $file
equals $this->configFilePath
.
The old check evaluated the if-statement to false if $this->configFilePath
exists but fopen() returned false. The new check would evaluate the if-statement to true.
But I believe that this is just a theoretical difference. I don't see how this should make a difference and also tested it.
* when the config file is not writable there is a error message shown * same happens now if the config file is not readable * fixes #180
3409440
to
191a6c6
Compare
LGTM |
tested, LGTM |
cc @adsworth @Mar1u5 @williambargent @LukasReschke @schiessle