Since survey settings inheritance have been introduced, empty attributes have always been inherited.
But for some attributes, an empty value is actually a valid attribute.

In those cases, the inheritance rules get confused. Is the attribute empty because it should be inherited or because it is deactivated?
Created specific rules for specific attributes.

PR https://github.com/LimeSurvey/LimeSurvey/pull/2370

I thought in this case the field would store the value 'inherit'.

I really think we must move inherit to null in database.
Then you can merge attributes
finalValues= array_merge(
$globalValues,
$groupSettings,
$surveySettings
);

Maybe we can reproduce with some array_filter ?
array_filter($surveySettings, function($setting) {
return $setting != "inherit";
});

The solution I uploaded is a temporary solution.
Alternatives we thought:

Right now the buttons are "Inherit On", "Inherit Off".
Make them "On", "Off", "Inherir".
In case they select "Off", we can:
a) store "N" in the email field or
b) store "N" on a new field.
c) store empty on the email field.
That would be a UI complement to the current patch.
Not much else needed.

I think the best and the less error prone is alternative B.

I really think we must move inherit to null in database.

Not sure I follow. Isn't that a different discussion?
The problem here is that we are using empty as a NO option but the inheritance is understanding it as an Inherit command

Not sure I follow. Isn't that a different discussion?

It's exactly the same discussion :)

"" or 0 still return "" or 0
BUT null value is used for inherit.
If inherited is ""

Then code can test if(!attribute) { }

A quick edit on https://github.com/LimeSurvey/LimeSurvey/pull/2370/files#diff-1a1aff57bf56ce79a03058f0d5bc46f25b3f768b3bc41b53bdcd0b07959ced90R469 private function shouldInherit($attribute)

Add || is_null($this->oOptions->{$attribute}) at line 479

        // Always returhn true if NULL or not set
        if(!isset($this->oOptions->{$attribute})) {
            return true;
        }
        // The attribute should be inherited if its value is 'inherit', 'I' or '-1'. (maybe filter by attribute name)
        if (
                $this->oOptions->{$attribute} === 'inherit'
                || $this->oOptions->{$attribute} === 'I'
                // NB: Do NOT use === here, it won't work with Postgresql.
                || $this->oOptions->{$attribute} == '-1'
            )
        ) {
            return true;
        }
        // return 0, false and empty string
        return false;

It's exactly the same discussion :)

I think it is a different discussion: A much bigger one! With more impact :)

I really think we must move inherit to null in database

This would be much more error prone when thinking of importing/exporting files. Null could be easliy lost.
I don't like taking 0 or "" as inherit, but I believe it is not the time to review that.

I just want to fix it for this attributes with the tools we already have

You make it here : https://github.com/LimeSurvey/LimeSurvey/pull/2412/files

null/not set => inherit.

And i think it's the best way

I don't like taking 0 or "" as inherit, but I believe it is not the time to review that.

It's exactly my point : use is_null or !isset … for inherit, not "I" or 'inherit' or … etc …

Discussion on going