nextcloud / docker

⛴ Docker image of Nextcloud
https://hub.docker.com/_/nextcloud/
GNU Affero General Public License v3.0
6.09k stars 1.83k forks source link

Fix initialization of `autocreate` and `use_ssl` #2309

Closed vbrandl closed 4 weeks ago

vbrandl commented 1 month ago

According to the documentation, both OBJECTSTORE_S3_SSL and OBJECTSTORE_S3_AUTOCREATE should default to true. Currently, when these environment variables are not set, they default to false. (Closes https://github.com/nextcloud/docker/issues/2308).

This fix works, because strtolower(false) returns the empty string. So when OBJECTSTORE_S3_SSL is not set and getenv('OBJECTSTORE_S3_SSL') returns false, the check strtolower($use_ssl) !== 'false' will evaluate to true.

With this fix, both values will be true if they are

This should now match the documented behavior.

vbrandl commented 1 month ago

What exactly do you mean? What "gibberish" don't you understand and which part of my proposed change is "no good"?

I did my best to be clear and improve this project. I opened an issue with a reproducible bug, then did local tests to see where the bug originates and fixed the bug so the image behaves as documented.

Maybe this testcase helps clear things up:

<?php

$foo = getenv('FOO');

$old_result = (strtolower($foo) === 'false' || $foo == false) ? false : true;
echo 'Old Result: ';
var_dump($old_result);

$new_result = strtolower($foo) !== 'false';
echo 'New Result: ';
var_dump($new_result);
# no environment variable, expected result: `true`
$ php test.php
Old Result: bool(false)
New Result: bool(true)

# environment variable set to `false`, expected result: `false`
$ FOO=fAlse php test.php
Old Result: bool(false)
New Result: bool(false)

# environment variable set to `true`, expected result: `true`
$ FOO=true php test.php
Old Result: bool(true)
New Result: bool(true)

# environment variable set but neither `true` nor `false`, expected result: `true`
$ FOO=invalid php test.php
Old Result: bool(true)
New Result: bool(true)

This shows, that the old logic does not work as documented (e.g. default to true) when the environment variable is not set at all. This conflicts with the documented behavior, which states, that the value should default to true.

Juoelenis commented 1 month ago

Ah yes sorry for da thang, it seems it is a legit bug fix, but submit it to mail, in PR they dont merge, i dunno why but you have to send the fix via mail

vbrandl commented 1 month ago

How do I submit my PR via mail? Or do you mean that my commit message has to contain Signed-off-by: Author Name <authoremail@example.com> as stated in the failed CI action?

I will update the commit message and push again soon

Juoelenis commented 1 month ago

They dont accept pr's thru the PR, you must send them the changes via mail, understand?

joshtrichards commented 1 month ago

Related: #1948

vbrandl commented 1 month ago

I just pushed again with proper Signed-off-by in my commit message.

Also sorry for feeding the troll... I fell for them, but reported the account to Github.

J0WI commented 4 weeks ago

Thanks!