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

Fix initialization of autocreate and use_ssl #2309

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

vbrandl
Copy link
Contributor

@vbrandl vbrandl commented Oct 8, 2024

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 #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.

Juoelenis

This comment was marked as off-topic.

@vbrandl
Copy link
Contributor Author

vbrandl commented Oct 9, 2024

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
Copy link

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
Copy link
Contributor Author

vbrandl commented Oct 9, 2024

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

This comment was marked as off-topic.

@joshtrichards
Copy link
Member

Related: #1948

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`. (See nextcloud#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.

Signed-off-by: Valentin Brandl <mail@vbrandl.net>
@vbrandl
Copy link
Contributor Author

vbrandl commented Oct 9, 2024

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 J0WI merged commit d3341b6 into nextcloud:master Oct 24, 2024
18 of 21 checks passed
@J0WI
Copy link
Contributor

J0WI commented Oct 24, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OBJECTSTORE_S3_SSL does not work as documented
4 participants