core icon indicating copy to clipboard operation
core copied to clipboard

config:app:set has no "type" parameter

Open phil-davis opened this issue 5 years ago • 8 comments

occ config:system:set has --type parameter and you can set a setting to boolean true or false.

occ config:app:set has no --type parameter. Do something like:

php occ config:app:set files enable_lock_file_action --value true

Actually the setting is string "true". Code that gets the setting ends up understanding that it is boolean true because it is non-zero.

php occ config:app:set files enable_lock_file_action --value false

The setting is string "false". Code that gets the setting ends up understanding that it is boolean true because it is non-zero. But users get confused, it looks false but the functionality is still enabled.

php occ config:app:set files enable_lock_file_action --value 0

Setting to 0 really switched off the setting.

occ config:app:delete files enable_lock_file_action

Deleting the setting allows it to go back to its default of false.

It would be less confusing if we could do:

php occ config:app:set files enable_lock_file_action --type boolean --value false

If the backend database field is only storing a string, then --type boolean --value false could store "0" and --type boolean --value true could store "1".

phil-davis avatar Jul 17 '20 03:07 phil-davis

I see that many of these app settings that are "boolean" on/off switches have code like https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/Capabilities.php#L85

$roPasswordEnforced = $this->config->getAppValue('core', 'shareapi_enforce_links_password_read_only', 'no') === 'yes';

expects the value to be set to the string "yes" or "no".

But https://github.com/owncloud/core/blob/master/apps/files/lib/App.php#L51 :

$enableLockFileAction = (boolean)($config->getAppValue('files', 'enable_lock_file_action', false));

interprets the value as a boolean.

So maybe we actually just need to adjust that sort of code to use the "yes"/"no" string convention?

phil-davis avatar Jul 17 '20 04:07 phil-davis

Just got some feedback: Would it be possible in this context to sneak in a checkbox in the Admin settings "Additional" section (where the file locking expiration settings are, under "Manual File Locking") that allows an admin to enable/disable the frontend components?

=> "Enable manual file locking in the web interface" or similar.

@phil-davis maybe?

pmaier1 avatar Jul 17 '20 08:07 pmaier1

Maybe my thinking is too easy, but why not adding a --type parameter for occ config:app:set ? This would make the behaviour the same to occ config:system:set where this option exists.

For me, if I would administer a system, one time using true/false and one time using yes/no is for the same idea not understandable and confusing.

mmattel avatar Jul 17 '20 09:07 mmattel

Possibly at the moment there are no places that set an app config to a "boolean" value, and this new enable_lock_file_action setting tries to use a boolean value. But, as we have discovered, internally it is not really boolean anyway.

Other existing settings that are on-offf switches are using "yes" <=> "on" and anything else ("no") <=> "off".

So for enable_lock_file_action it is going to be more consistent to make it behave in the yes-no way, like other existing options.

But yes, as a general thing someone could investigate if there is a case for adding --type to the command.

phil-davis avatar Jul 17 '20 09:07 phil-davis

Q: what is the command to list available config:app variables? The only thing I found was:

./occ config:list -- files

{
    "apps": {
        "files": {
            "cronjob_scan_files": "500",
            "enabled": "yes",
            "installed_version": "1.5.2",
            "types": "filesystem"
        }
    }
}

But it does not return any variables to change like enable_lock_file_action. (the above example is taken after switching to the adjust-enable_lock_file_action branch)

There is no command like ./occ config:app:list So how would one know what to look for and what the proper values for these variables are?

mmattel avatar Jul 17 '20 12:07 mmattel

I don't think there is any command. Each app can use whatever setting key-values it likes, so it is the app that "knows". There would have to be some callback/hook to the app for it to provide a list of known valid settings keys.

I can set whatever keys I like, e.g.

$ php occ config:app:set files zzz --value "abc"
Config value zzz for app files set to abc
$ php occ config:list -- files
{
    "apps": {
        "files": {
            "cronjob_scan_files": "500",
            "enable_lock_file_action": "yes",
            "enabled": "yes",
            "installed_version": "1.5.2",
            "types": "filesystem",
            "zzz": "abc"
        }
    }
}

and "zzz" is set to "abc" for the files app. It effectively does nothing because the files app never uses it.

This can be a problem when someone accidentally mistypes a key:

$ php occ config:app:set files enabl_lock_file_action --value "yes"

That command succeeds and they do not get told that the key is not a known valid key.

phil-davis avatar Jul 17 '20 12:07 phil-davis

That command succeeds and they do not get told that the key is not a known valid key.

There should be a command like ./occ config:app:list where you can optionally specify: the app, known internal variables and explicitly set variables.

This is not only an admin but also a documentation nightmare. Nobody will remember config:app variables days after implementaion without a list command and documentation!

mmattel avatar Jul 17 '20 12:07 mmattel

Still open this issue?

ibonkonesa avatar Oct 04 '22 06:10 ibonkonesa