Make enable_avatars setting more robust

[MorrisJobke]

• handles the setting in the same way everywhere
• fixes Setting enable_avatar to false with occ causes some bugs #21949

@nickvergessen @rullzer @LukasReschke @Pookay Please review

@karlitschek We maybe want to backport this ;)

[mention-bot]

By analyzing the blame information on this pull request, we identified @rullzer, @DeepDiver1975 and @LukasReschke to be potential reviewers

[karlitschek]

please backport. 👍

[nickvergessen]

I don't think this is needed.

[MorrisJobke]

I don't think this is needed.

Be more precise please.

Set it to:

• true -> it is always evaluated as true
• 'everyString' -> it is sometimes evaluated as true (where no explicit check is done - if($config)) and sometimes evaluated as false where the check is specified (if($config === true)
• false -> is is always evaluated as false

This change unified the overall behavior to: 'randomString' means false

[DeepDiver1975]

We have this 'issue' with multiple config settings.

[MorrisJobke]

We have this 'issue' with multiple config settings.

That's because we have no proper options validation/casting and unified defaults management.

[nickvergessen]

Well we could do casting based on the provided default value...

Anyway this is at least nothign we should backport, because it is a behaviour change

[MorrisJobke]

Well we could do casting based on the provided default value...

-.- Really? This could break everything.

[nickvergessen]

Well with null being an exception, but everytime we set a default its of the type we want to have, anyway your patch here is changing behaviour as well, because '1' previously was enabled, and now it's disabled...
From my pov we shouldn't touch something like this until we have a final outcome about casting in general...

[MorrisJobke]

anyway your patch here is changing behaviour as well, because '1' previously was enabled, and now it's disabled...

Not really. Because it was enabled in some code paths and disabled in other code paths, which actually cause the issue this is fixing. The issue was, that it tried to load avatars(setting was evaluated to true) even if the markup to put them in is not present (setting was evaluated to false). This only aligns all usages of the setting to evaluate in the same way.

[DeepDiver1975]

TBH: the real issue is that the value in the config file was simply wrong - which was caused by the occ command.

[MorrisJobke]

TBH: the real issue is that the value in the config file was simply wrong - which was caused by the occ command.

I would nevertheless make all checks be proof against this. Either all check for true or all use the plain if($value) format. But definitely not the mixed stuff, because PHP is sometimes hard to tame and we should avoid this. At least in this case where the actual issue is really hard to notice easy. (compared to a typo in the config.php or a plain wrong DB password, etc)

[nickvergessen]

We can also check the type of all known values and display warnings on the admin page 🙊

[DeepDiver1975]

Seriously - let's just assume users editing the config file are capable to read.

As said above: the original issue was raised because of the config command was not capable to set the boolean value properly

[MorrisJobke]

Could we get back to topic and simply review this change. Even if it is stupid - it doesn't hurt to merge it and it fixes a problem that exists in the real world. The actual problem here is that PHP is dynamically typed and we as developers failed to use a value in the same way everywhere. So this is a pragmatic approach to fix it and it doesn't make anything more complex.

[PVince81]

👍

[DeepDiver1975]

👍

[MorrisJobke]

backport is in #22047