feat(core): cast valid booleans when setting system configs

[skjnldsv]

I think we could be smarter with primitive values like those 🙈

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)
• Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
• Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

• The content of this PR was partly or fully generated using AI

[come-nc]

But that would mean when setting a configuration to "false" or "true" it would automagically decide that it’s a boolean even if we indicated type string?
That seems dangerous to me.
Maybe output a warning but do not change the type on the fly.

Otherwise how would one set a configuration to the string "true", it would not be possible anymore?

[skjnldsv]

But that would mean when setting a configuration to "false" or "true" it would automagically decide that it’s a boolean even if we indicated type string? That seems dangerous to me. Maybe output a warning but do not change the type on the fly.

Otherwise how would one set a configuration to the string "true", it would not be possible anymore?

We do not have any
https://github.com/nextcloud/server/blob/bb4c9ecc0e316782227570257856891995b1e369/config/config.sample.php

[artonge]

I just saw that Côme had the same comment. Given your answer, I say let's go!

[come-nc]

But that would mean when setting a configuration to "false" or "true" it would automagically decide that it’s a boolean even if we indicated type string? That seems dangerous to me. Maybe output a warning but do not change the type on the fly.
Otherwise how would one set a configuration to the string "true", it would not be possible anymore?

We do not have any https://github.com/nextcloud/server/blob/bb4c9ecc0e316782227570257856891995b1e369/config/config.sample.php

We do not have any what? String in the configuration?

If the dbuser is named "true" or it’s the smtp password or whatever, now the command will silently set it to a boolean despite being called with --type string. That’s misleading. It should at the very least output a warning when the type was explicitly set and it does an auto-conversion.
I’m fine with guessing the type when --type was not passed, but silently not respecting an explicit option is bad behavior.

[joshtrichards]

Given we already cast at the explicit direction of the user prior to this if type is specified, it seems a little weird, at least under those circumstances:

server/core/Command/Config/System/SetConfig.php

Lines 61 to 66 in d26bf95

	$configValue = $this->castHelper->castValue($input->getOption('value'), $input->getOption('type'));
	$updateOnly = $input->getOption('update-only');
	// If value is string false or true, cast them too
	if (is_string($configValue['value'])) {
	$lowerValue = strtolower($configValue['value']);

e.g. --type string --value="true"

Also little weird if we only do this for system, not app. ;-)

I’m fine with guessing the type when --type was not passed, but silently not respecting an explicit option is bad behavior.

This ^^^.

And --type must never be overridden -- even with a warning (except for when it's impossible/unrealistic).

P.S. Also it hadn't occurred to me how distinct OC\Core\Command\Config\App and OC\Core\Command\Config\System's implementations are until just now:

server/core/Command/Config/App/SetConfig.php

Lines 95 to 100 in d26bf95

	$type = $typeString = null;
	if ($input->hasParameterOption('--type')) {
	$typeString = $input->getOption('type');
	$type = $this->appConfig->convertTypeToInt($typeString);
	}

server/core/Command/Config/App/SetConfig.php

Lines 120 to 138 in d26bf95

	if ($type !== null && $type !== $this->appConfig->getValueType($appName, $configName) && $typeString !== null && $this->ask($input, $output, $typeString)) {
	$updated = $this->appConfig->updateType($appName, $configName, $type) || $updated;
	}
	} else {
	/**
	* If --type is specified in the command line, we upgrade the type in database
	* after a confirmation from admin.
	* If not we get the type from current stored value or VALUE_MIXED as default.
	*/
	try {
	$currType = $this->appConfig->getValueType($appName, $configName);
	if ($type === null || $typeString === null || $type === $currType || !$this->ask($input, $output, $typeString)) {
	$type = $currType;
	} else {
	$updated = $this->appConfig->updateType($appName, $configName, $type);
	}
	} catch (AppConfigUnknownKeyException) {
	$type = $type ?? IAppConfig::VALUE_MIXED;
	}

[come-nc]

Also config.sample.php is not complete, see #59911
twofactor_enforced is a string boolean.