Skip to content

Commit 58d6ffe

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 d8f2b3f commit 58d6ffe

9 files changed

Lines changed: 116 additions & 65 deletions

File tree

‎lib/Controller/ConfigController.php‎

Lines changed: 23 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
@@ -61,8 +61,25 @@ public function updateAppConfig(string $configKey, $configValue): DataResponse {
6161
return new DataResponse('Unknown appConfig key: ' . $configKey, Http::STATUS_BAD_REQUEST);
6262
}
6363

64-
// Set on DB
65-
$this->config->setAppValue($this->appName, $configKey, json_encode($configValue));
64+
// Set on DB with typed setters
65+
switch ($configKey) {
66+
case Constants::CONFIG_KEY_ALLOWPERMITALL:
67+
case Constants::CONFIG_KEY_ALLOWPUBLICLINK:
68+
case Constants::CONFIG_KEY_ALLOWSHOWTOALL:
69+
case Constants::CONFIG_KEY_RESTRICTCREATION:
70+
try {
71+
$this->appConfig->setAppValueBool($configKey, $configValue);
72+
} catch (err) {
73+
return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST);
74+
}
75+
76+
case Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS:
77+
try {
78+
$this->appConfig->setAppValueArray($configKey, $configValue);
79+
} catch (err) {
80+
return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST);
81+
}
82+
}
6683

6784
return new DataResponse();
6885
}

‎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: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
use OCA\Forms\Constants;
1212

13-
use OCP\IConfig;
13+
use OCP\AppFramework\Services\IAppConfig;
1414
use OCP\IGroup;
1515
use OCP\IGroupManager;
1616
use OCP\IUser;
@@ -21,7 +21,7 @@ class ConfigService {
2121

2222
public function __construct(
2323
protected string $appName,
24-
private IConfig $config,
24+
private IAppConfig $appConfig,
2525
private IGroupManager $groupManager,
2626
IUserSession $userSession,
2727
) {
@@ -32,22 +32,22 @@ public function __construct(
3232
* Load the single values, decode, have default values
3333
*/
3434
public function getAllowPermitAll(): bool {
35-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPERMITALL, 'true'));
35+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWPERMITALL, true);
3636
}
3737
public function getAllowPublicLink(): bool {
38-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPUBLICLINK, 'true'));
38+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWPUBLICLINK, true);
3939
}
4040
public function getAllowShowToAll() : bool {
41-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWSHOWTOALL, 'true'));
41+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWSHOWTOALL, true);
4242
}
4343
private function getUnformattedCreationAllowedGroups(): array {
44-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS, '[]'));
44+
return $this->appConfig->getAppValueArray(Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS, []);
4545
}
4646
public function getCreationAllowedGroups(): array {
4747
return $this->formatGroupsForMultiselect($this->getUnformattedCreationAllowedGroups());
4848
}
4949
public function getRestrictCreation(): bool {
50-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_RESTRICTCREATION, 'false'));
50+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_RESTRICTCREATION, false);
5151
}
5252

5353
/**

‎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\AppFramework\Services\IAppConfig;
1515

1616
/**
1717
* This tests that the API respects all admin settings
@@ -221,9 +221,14 @@ public function testAllowUpdate(): void {
221221
* @dataProvider forbidUpdateAdminSettingsData
222222
*/
223223
public function testForbidUpdate(array $accessValue, array $adminConfigKeys): void {
224-
$config = \OCP\Server::get(IConfig::class);
224+
$config = \OCP\Server::get(IAppConfig::class);
225225
foreach ($adminConfigKeys as $key => $value) {
226-
$config->setAppValue(Application::APP_ID, $key, $value);
226+
if (is_array($value)) {
227+
$config->setAppValueArray($key, $value);
228+
} else {
229+
$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
230+
$config->setAppValueBool($key, $bool);
231+
}
227232
}
228233

229234
$resp = $this->http->request(
@@ -322,7 +327,7 @@ public function testListFormsAllowed(): void {
322327
*/
323328
public function testListFormsNoAdminPermission(): void {
324329
// Disable global access
325-
\OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, Constants::CONFIG_KEY_ALLOWPERMITALL, 'false');
330+
\OCP\Server::get(IAppConfig::class)->setAppValueBool(Constants::CONFIG_KEY_ALLOWPERMITALL, false);
326331

327332
$resp = $this->http->request(
328333
'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\AppFramework\Services\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->setAppValueArray($key, $value);
187+
} else {
188+
$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
189+
$config->setAppValueBool($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->setAppValueBool(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->setAppValueArray($key, $value);
228+
} else {
229+
$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
230+
$config->setAppValueBool($key, $bool);
231+
}
222232
}
223233

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

‎tests/Integration/IntegrationBase.php‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99

1010
use OCA\Forms\AppInfo\Application;
1111
use OCA\Forms\Constants;
12+
use OCP\AppFramework\Services\IAppConfig;
1213
use OCP\DB\QueryBuilder\IQueryBuilder;
13-
use OCP\IConfig;
1414
use OCP\IDBConnection;
1515
use OCP\IUserManager;
1616
use Test\TestCase;
@@ -19,8 +19,7 @@
1919
* @group DB
2020
*/
2121
class IntegrationBase extends TestCase {
22-
/** @var Array */
23-
protected $testForms;
22+
protected array $testForms;
2423

2524
/**
2625
* Users that are needed by this test case
@@ -35,9 +34,9 @@ class IntegrationBase extends TestCase {
3534
public function setUp(): void {
3635
parent::setUp();
3736

38-
$config = \OCP\Server::get(IConfig::class);
37+
$appConfig = \OCP\AppFramework\Services::get(IAppConfig::class);
3938
foreach (Constants::CONFIG_KEYS as $key) {
40-
$config->deleteAppValue(Application::APP_ID, $key);
39+
$appConfig->deleteAppValue($key);
4140
}
4241

4342
$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)