Skip to content

Commit 2aba48e

Browse files
Chartman123Chartman123
committed
refactor(config): replace IConfig with IAppConfig for improved type safety and clarity
Signed-off-by: Christian Hartmann <chris-hartmann@gmx.de>
1 parent 87fde1c commit 2aba48e

9 files changed

Lines changed: 116 additions & 63 deletions

File tree

‎lib/Controller/ConfigController.php‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
use OCP\AppFramework\Http\Attribute\FrontpageRoute;
1616
use OCP\AppFramework\Http\Attribute\OpenAPI;
1717
use OCP\AppFramework\Http\DataResponse;
18-
use OCP\IConfig;
18+
use OCP\AppFramework\Services\IAppConfig;
1919
use OCP\IRequest;
2020

2121
use Psr\Log\LoggerInterface;
@@ -25,7 +25,7 @@ class ConfigController extends ApiController {
2525
public function __construct(
2626
protected $appName,
2727
private ConfigService $configService,
28-
private IConfig $config,
28+
private IAppConfig $appConfig,
2929
private LoggerInterface $logger,
3030
IRequest $request,
3131
) {
@@ -46,11 +46,11 @@ public function getAppConfig(): DataResponse {
4646
* Admin required, thus not checking separately.
4747
*
4848
* @param string $configKey AppConfig Key to store
49-
* @param mixed $configValues Corresponding AppConfig Value
49+
* @param mixed $configValue Corresponding AppConfig Value
5050
*
5151
*/
5252
#[FrontpageRoute(verb: 'PATCH', url: '/config')]
53-
public function updateAppConfig(string $configKey, $configValue): DataResponse {
53+
public function updateAppConfig(string $configKey, mixed $configValue): DataResponse {
5454
$this->logger->debug('Updating AppConfig: {configKey} => {configValue}', [
5555
'configKey' => $configKey,
5656
'configValue' => $configValue
@@ -65,8 +65,27 @@ public function updateAppConfig(string $configKey, $configValue): DataResponse {
6565
return new DataResponse('Mail server is not configured', Http::STATUS_BAD_REQUEST);
6666
}
6767

68-
// Set on DB
69-
$this->config->setAppValue($this->appName, $configKey, json_encode($configValue));
68+
// Set on DB with typed setters
69+
switch ($configKey) {
70+
case Constants::CONFIG_KEY_ALLOWPERMITALL:
71+
case Constants::CONFIG_KEY_ALLOWPUBLICLINK:
72+
case Constants::CONFIG_KEY_ALLOWSHOWTOALL:
73+
case Constants::CONFIG_KEY_RESTRICTCREATION:
74+
try {
75+
$this->appConfig->setAppValueBool($configKey, $configValue);
76+
} catch (\InvalidArgumentException $e) {
77+
return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST);
78+
}
79+
break;
80+
81+
case Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS:
82+
try {
83+
$this->appConfig->setAppValueArray($configKey, $configValue);
84+
} catch (\InvalidArgumentException $e) {
85+
return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST);
86+
}
87+
break;
88+
}
7089

7190
return new DataResponse();
7291
}

‎lib/Migration/Version0010Date20190000000007.php‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
namespace OCA\Forms\Migration;
99

1010
use OCP\DB\ISchemaWrapper;
11-
use OCP\IConfig;
1211
use OCP\IDBConnection;
1312
use OCP\Migration\IOutput;
1413
use OCP\Migration\SimpleMigrationStep;
@@ -22,16 +21,11 @@ class Version0010Date20190000000007 extends SimpleMigrationStep {
2221
/** @var IDBConnection */
2322
protected $connection;
2423

25-
/** @var IConfig */
26-
protected $config;
27-
2824
/**
2925
* @param IDBConnection $connection
30-
* @param IConfig $config
3126
*/
32-
public function __construct(IDBConnection $connection, IConfig $config) {
27+
public function __construct(IDBConnection $connection) {
3328
$this->connection = $connection;
34-
$this->config = $config;
3529
}
3630

3731
/**

‎lib/Migration/Version010200Date20200323141300.php‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
use OCP\DB\ISchemaWrapper;
1212
use OCP\DB\QueryBuilder\IQueryBuilder;
1313
use OCP\DB\Types;
14-
use OCP\IConfig;
1514
use OCP\IDBConnection;
1615
use OCP\Migration\IOutput;
1716

@@ -26,9 +25,6 @@ class Version010200Date20200323141300 extends SimpleMigrationStep {
2625
/** @var IDBConnection */
2726
protected $connection;
2827

29-
/** @var IConfig */
30-
protected $config;
31-
3228
/** Map of questionTypes to change */
3329
private $questionTypeMap = [
3430
'radiogroup' => 'multiple_unique',
@@ -40,11 +36,9 @@ class Version010200Date20200323141300 extends SimpleMigrationStep {
4036

4137
/**
4238
* @param IDBConnection $connection
43-
* @param IConfig $config
4439
*/
45-
public function __construct(IDBConnection $connection, IConfig $config) {
40+
public function __construct(IDBConnection $connection) {
4641
$this->connection = $connection;
47-
$this->config = $config;
4842
}
4943

5044
/**

‎lib/Service/ConfigService.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,22 +34,22 @@ public function __construct(
3434
* Load the single values, decode, have default values
3535
*/
3636
public function getAllowPermitAll(): bool {
37-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPERMITALL, 'true'));
37+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWPERMITALL, true);
3838
}
3939
public function getAllowPublicLink(): bool {
40-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPUBLICLINK, 'true'));
40+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWPUBLICLINK, true);
4141
}
4242
public function getAllowShowToAll() : bool {
43-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWSHOWTOALL, 'true'));
43+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWSHOWTOALL, true);
4444
}
4545
private function getUnformattedCreationAllowedGroups(): array {
46-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS, '[]'));
46+
return $this->appConfig->getAppValueArray(Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS, []);
4747
}
4848
public function getCreationAllowedGroups(): array {
4949
return $this->formatGroupsForMultiselect($this->getUnformattedCreationAllowedGroups());
5050
}
5151
public function getRestrictCreation(): bool {
52-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_RESTRICTCREATION, 'false'));
52+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_RESTRICTCREATION, false);
5353
}
5454

5555
public function getAllowConfirmationEmail(): bool {

‎tests/Integration/Api/RespectAdminSettingsTest.php‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
use OCA\Forms\AppInfo\Application;
1212
use OCA\Forms\Constants;
1313
use OCA\Forms\Tests\Integration\IntegrationBase;
14-
use OCP\IConfig;
14+
use OCP\IAppConfig;
1515

1616
/**
1717
* This tests that the API respects all admin settings
@@ -225,9 +225,14 @@ public function testAllowUpdate(): void {
225225
* @dataProvider forbidUpdateAdminSettingsData
226226
*/
227227
public function testForbidUpdate(array $accessValue, array $adminConfigKeys): void {
228-
$config = \OCP\Server::get(IConfig::class);
228+
$config = \OCP\Server::get(IAppConfig::class);
229229
foreach ($adminConfigKeys as $key => $value) {
230-
$config->setAppValue(Application::APP_ID, $key, $value);
230+
if (is_array($value)) {
231+
$config->setValueArray(Application::APP_ID, $key, $value);
232+
} else {
233+
$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
234+
$config->setValueBool(Application::APP_ID, $key, $bool);
235+
}
231236
}
232237

233238
$resp = $this->http->request(
@@ -326,7 +331,7 @@ public function testListFormsAllowed(): void {
326331
*/
327332
public function testListFormsNoAdminPermission(): void {
328333
// Disable global access
329-
\OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, Constants::CONFIG_KEY_ALLOWPERMITALL, 'false');
334+
\OCP\Server::get(IAppConfig::class)->setValueBool(Application::APP_ID, Constants::CONFIG_KEY_ALLOWPERMITALL, false);
330335

331336
$resp = $this->http->request(
332337
'GET',

‎tests/Integration/DB/SharedFormsTest.php‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
use OCA\Forms\Constants;
1313
use OCA\Forms\Db\FormMapper;
1414
use OCA\Forms\Tests\Integration\IntegrationBase;
15-
use OCP\IConfig;
15+
use OCP\IAppConfig;
1616

1717
/**
1818
* @group DB
@@ -180,9 +180,14 @@ public function testPublicSharedForms() {
180180
* @dataProvider dataForbidPublicShowAccess
181181
*/
182182
public function testShowNoSharedFormsIfDisabled(array $configValues) {
183-
$config = \OCP\Server::get(IConfig::class);
183+
$config = \OCP\Server::get(IAppConfig::class);
184184
foreach ($configValues as $key => $value) {
185-
$config->setAppValue(Application::APP_ID, $key, json_encode($value));
185+
if (is_array($value)) {
186+
$config->setValueArray(Application::APP_ID, $key, $value);
187+
} else {
188+
$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
189+
$config->setValueBool(Application::APP_ID, $key, $bool);
190+
}
186191
}
187192

188193
$formMapper = \OCP\Server::get(FormMapper::class);
@@ -199,8 +204,8 @@ public function testShowNoSharedFormsIfDisabled(array $configValues) {
199204
* Test that a form with public access can be accessed even if show permissions are not granted (can fill out but not see in sidebar)
200205
*/
201206
public function testAllowPublicAccessOnDeniedPublicVisibility(): void {
202-
$config = \OCP\Server::get(IConfig::class);
203-
$config->setAppValue(Application::APP_ID, Constants::CONFIG_KEY_ALLOWSHOWTOALL, json_encode(false));
207+
$config = \OCP\Server::get(IAppConfig::class);
208+
$config->setValueBool(Application::APP_ID, Constants::CONFIG_KEY_ALLOWSHOWTOALL, false);
204209

205210
$formMapper = \OCP\Server::get(FormMapper::class);
206211
$forms = $formMapper->findSharedForms('user1', filterShown: false);
@@ -216,9 +221,14 @@ public function testAllowPublicAccessOnDeniedPublicVisibility(): void {
216221
* @dataProvider dataForbidPublicAccess
217222
*/
218223
public function testShowNoSharedFormsAccessIfDisabled(array $configValues): void {
219-
$config = \OCP\Server::get(IConfig::class);
224+
$config = \OCP\Server::get(IAppConfig::class);
220225
foreach ($configValues as $key => $value) {
221-
$config->setAppValue(Application::APP_ID, $key, json_encode($value));
226+
if (is_array($value)) {
227+
$config->setValueArray(Application::APP_ID, $key, $value);
228+
} else {
229+
$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
230+
$config->setValueBool(Application::APP_ID, $key, $bool);
231+
}
222232
}
223233

224234
$formMapper = \OCP\Server::get(FormMapper::class);

‎tests/Integration/IntegrationBase.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
use OCA\Forms\AppInfo\Application;
1111
use OCA\Forms\Constants;
1212
use OCP\DB\QueryBuilder\IQueryBuilder;
13-
use OCP\IConfig;
13+
use OCP\IAppConfig;
1414
use OCP\IDBConnection;
1515
use OCP\IUserManager;
1616
use Test\TestCase;
@@ -35,9 +35,9 @@ class IntegrationBase extends TestCase {
3535
public function setUp(): void {
3636
parent::setUp();
3737

38-
$config = \OCP\Server::get(IConfig::class);
38+
$config = \OCP\Server::get(IAppConfig::class);
3939
foreach (Constants::CONFIG_KEYS as $key) {
40-
$config->deleteAppValue(Application::APP_ID, $key);
40+
$config->deleteKey(Application::APP_ID, $key);
4141
}
4242

4343
$userManager = \OCP\Server::get(IUserManager::class);

‎tests/Unit/Controller/ConfigControllerTest.php‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111

1212
use OCA\Forms\Service\ConfigService;
1313
use OCP\AppFramework\Http\DataResponse;
14-
use OCP\IConfig;
14+
use OCP\AppFramework\Services\IAppConfig;
1515
use OCP\IRequest;
1616

1717
use PHPUnit\Framework\MockObject\MockObject;
@@ -27,7 +27,7 @@ class ConfigControllerTest extends TestCase {
2727
/** @var ConfigService */
2828
private $configService;
2929

30-
/** @var IConfig|MockObject */
30+
/** @var IAppConfig|MockObject */
3131
private $config;
3232

3333
/** @var LoggerInterface|MockObject */
@@ -40,7 +40,7 @@ public function setUp(): void {
4040
parent::setUp();
4141

4242
$this->configService = $this->createMock(ConfigService::class);
43-
$this->config = $this->createMock(IConfig::class);
43+
$this->config = $this->createMock(IAppConfig::class);
4444
$this->logger = $this->createMock(LoggerInterface::class);
4545
$this->request = $this->createMock(IRequest::class);
4646

@@ -69,18 +69,18 @@ public function testGetAppConfig() {
6969

7070
public static function dataUpdateAppConfig() {
7171
return [
72-
'booleanConfig' => [
72+
'booleanAllowPermitAll' => [
7373
'configKey' => 'allowPermitAll',
7474
'configValue' => true,
7575
'strConfig' => 'true'
7676
],
77-
'booleanConfig' => [
77+
'booleanAllowShowToAll' => [
7878
'configKey' => 'allowShowToAll',
7979
'configValue' => true,
8080
'strConfig' => 'true'
8181
],
82-
'arrayConfig' => [
83-
'configKey' => 'allowPermitAll',
82+
'arrayCreationAllowedGroups' => [
83+
'configKey' => 'creationAllowedGroups',
8484
'configValue' => [
8585
'admin',
8686
'group1'
@@ -100,9 +100,15 @@ public function testUpdateAppConfig(string $configKey, $configValue, string $str
100100
$this->logger->expects($this->once())
101101
->method('debug');
102102

103-
$this->config->expects($this->once())
104-
->method('setAppValue')
105-
->with('forms', $configKey, $strConfig);
103+
if (is_array($configValue)) {
104+
$this->config->expects($this->once())
105+
->method('setAppValueArray')
106+
->with($configKey, $configValue);
107+
} else {
108+
$this->config->expects($this->once())
109+
->method('setAppValueBool')
110+
->with($configKey, $configValue);
111+
}
106112

107113
$this->assertEquals(new DataResponse(), $this->configController->updateAppConfig($configKey, $configValue));
108114
}
@@ -112,7 +118,9 @@ public function testUpdateAppConfig_unknownKey() {
112118
->method('debug');
113119

114120
$this->config->expects($this->never())
115-
->method('setAppValue');
121+
->method('setAppValueBool');
122+
$this->config->expects($this->never())
123+
->method('setAppValueArray');
116124

117125
$this->assertEquals(new DataResponse('Unknown appConfig key: someUnknownKey', 400), $this->configController->updateAppConfig('someUnknownKey', 'storeThisValue!'));
118126
}

0 commit comments

Comments
 (0)