docker icon indicating copy to clipboard operation
docker copied to clipboard

Fix initialization of `autocreate` and `use_ssl`

Open vbrandl opened this issue 1 year ago • 6 comments

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

  • not set
  • the empty string
  • any string that is not equal to false when converted to lowercase

This should now match the documented behavior.

vbrandl avatar Oct 08 '24 22:10 vbrandl

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.

vbrandl avatar Oct 09 '24 06:10 vbrandl

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

Juoelenis avatar Oct 09 '24 10:10 Juoelenis

How do I submit my PR via mail? Or do you mean that my commit message has to contain Signed-off-by: Author Name <[email protected]> as stated in the failed CI action?

I will update the commit message and push again soon

vbrandl avatar Oct 09 '24 10:10 vbrandl

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

Juoelenis avatar Oct 09 '24 11:10 Juoelenis

Related: #1948

joshtrichards avatar Oct 09 '24 13:10 joshtrichards

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.

vbrandl avatar Oct 09 '24 13:10 vbrandl

Thanks!

J0WI avatar Oct 24 '24 19:10 J0WI