feat(devmanual): add section about system config key conventions

[ChristophWurst]

☑️ Resolves

• Fix topic discussed via chat

🖼️ Screenshots

[nickvergessen]

Suggested change

	* System config keys should only contain lower case letters, numbers and `_`. This ensures that they can be used as environment variables.
	* System config keys should only contain lower case letters, numbers and ``_``. This ensures that they can be used as environment variables.

Did you mean to make it code?

[st3iny]

I like bikes and sheds.

[szaimen]

@ChristophWurst should we add a workflow to server that ensures the consistency after merging the doc here?

[ChristophWurst]

@ChristophWurst should we add a workflow to server that ensures the consistency after merging the doc here?

I was thinking that we can update the phpdoc and see if psalm can help us. It will also make sense to find a mechanism for key aliases, so we can phase out the unconventional keys without hard breakages

[susnux]

I was thinking that we can update the phpdoc and see if psalm can help us. It will also make sense to find a mechanism for key aliases, so we can phase out the unconventional keys without hard breakages

Maybe some kind of sysconfig lexicon that allows configure aliases

• if the config contains unknown keys we can warn
• if the config is writable we can migrate using it on update
• if the config is readonly we can show that warning and remap the values

[susnux]

Looks good.

Only thing one could argue would be to use __ instead of _ to separate subsystem and key, to not confuse with separating words. E.g.:

• localstorage_umask
• localstorage_unlink_on_truncate

VS

• localstorage__umask
• localstorage__unlink_on_truncate

[szaimen]

Only thing one could argue would be to use __ instead of _ to separate subsystem and key, to not confuse with separating words. E.g

Yes, thought the same but this would mean even more changes to config keys, so I wouls argue it is better to leave it like it is currently with one separator...

[szaimen]

LGTM

[ChristophWurst]

Only thing one could argue would be to use __ instead of _ to separate subsystem and key, to not confuse with separating words

Yes, it makes sense, but at the same time I'm with @szaimen that it means we have to change even more keys. Additionally, I have not worked with any other systems so far that would use double underscore, so it seems a bit unusual to do.

[kesselb]

Only thing one could argue would be to use __ instead of _ to separate subsystem and key, to not confuse with separating words

That sounds like a good idea.

[nickvergessen]

Yes, it makes sense, but at the same time I'm with szaimen that it means we have to change even more keys. Additionally, I have not worked with any other systems so far that would use double underscore, so it seems a bit unusual to do.

We can also have old system for old keys.

I had the thought yesterday already that we could allow NC_app__$APPID__$CONFIGKEY then to overwrite app configs via the env?

[ChristophWurst]

Additionally, I have not worked with any other systems so far that would use double underscore, so it seems a bit unusual to do.

https://learn.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?view=aspnetcore-9.0#naming-of-environment-variables 🌝

[susnux]

Yes, thought the same but this would mean even more changes to config keys, so I wouls argue it is better to leave it like it is currently with one separator...

We would not have to change them if they work. Kind of legacy dept.

My biggest problem with the environment config is that it does not allow scoped configuration, where we use arrays.
For example:

'redis.cluster' => [
   'seeds' => [
   	'localhost:7000',
   	'localhost:7001',
   ],
   'timeout' => 0.0,
],

[nickvergessen]

Well for me the main question is:
Should all values be env-override-able or not? I don't think the answer is a clear yes, but I also lack the idea when/why an instance would use an environment overwrite (is it temporary?) vs touching the config file? Something like the cache, DB connection, etc. sound like a "no" to me. I can see how one would try to do it with the DB password and other secrets, so they are not stored in the filesystem, etc. but then we can also simply check if we can "flatten" out those specific values, so they can be specifically overwritten instead of having to translate the full array to ENV?

[ChristophWurst]

@susnux so that example would translate to this

NC_REDIS_CLUSTER__SEEDS__0=localhost:7000
NC_REDIS_CLUSTER__SEEDS__1=localhost:7001
NC_REDIS_CLUSTER__TIMEOUT=0.0

right?

Should all values be env-override-able or not?

I'd say it's a good thing to allow it. It's very useful in container setups.

[nickvergessen]

The bigger question with arrays is how replacing works. Let's say file has:

        "log.condition": {
            "apps": {
                "1": "admin_audit",
                "50": "calendar-appointments",
                "51": "appointments",
                "52": "calendar",
                "200": "spreed-bfp",
                "501": "imaginary"
            }
        },

would:

NC_log_condition__apps__0=perf_debug

result in:

        "log.condition": {
            "apps": {
                "0": "perf_debug"
            }
        },

or:

        "log.condition": {
            "apps": {
                "0": "perf_debug",
                "1": "admin_audit",
                "50": "calendar-appointments",
                "51": "appointments",
                "52": "calendar",
                "200": "spreed-bfp",
                "501": "imaginary"
            }
        },

if the later, how could env be used to unset one key?

[ChristophWurst]

Probably the second one, because of this:

php > $config = ["1" => "foo", "2" => "bar"];
php > $config[0] = "replaced";
php > var_dump($config);
array(3) {
  [1]=>
  string(3) "foo"
  [2]=>
  string(3) "bar"
  [0]=>
  string(8) "replaced"
}

[nickvergessen]

Time to get it in?

[ChristophWurst]

To which extent? Some of the ideas won't work right now and need changes in server

[susnux]

To which extent? Some of the ideas won't work right now and need changes in server

At least your changes here. They already work :)

[ChristophWurst]

/backport to stable31