Config cleanup - OC_Preferences refactoring #12406

MorrisJobke · 2014-11-25T08:44:53Z

cc @icewind1991 @DeepDiver1975 @PVince81 @LukasReschke

removal of all OC_Preferences references in core and apps I find in this github org.

MorrisJobke · 2014-11-25T08:45:21Z

ref #12260

MorrisJobke · 2014-11-25T09:05:41Z

I will tag this a the WIP branch for all of my config related cleanups

LukasReschke · 2014-11-28T12:37:27Z

lib/private/allconfig.php

@@ -61,7 +71,7 @@ public function getAppKeys($appName) {
 	 * @param string $value the value that should be stored
 	 */
 	public function setAppValue($appName, $key, $value) {
-		\OCP\Config::setAppValue($appName, $key, $value);
+		\OC::$server->getAppConfig()->setValue($appName, $key, $value);


Inject AppConfig?

Crashes the DI and I need to figure out why.... this is still WIP, but thanks for the comment.

nickvergessen · 2014-12-01T08:26:49Z

apps/files_encryption/lib/util.php

@@ -63,10 +66,10 @@ public function __construct($view, $userId, $client = false) {
 		$this->client = $client;
 		$this->userId = $userId;

-		$appConfig = \OC::$server->getAppConfig();
+		$this->config = \OC::$server->getConfig();


Can this be injected instead of being grabbed from global OC::

Yes, but there are currently that many occurences of the create logic of this class, that we also could put this in here. Once this is handled by an dependency container we can easiely add the parameter

LukasReschke · 2014-12-03T19:23:33Z

13:53:37 SSSSSSSSSSSSSSSSSSSSSSSSSSSSSS.........................PHP Fatal error:  Call to undefined method OC\Preferences::deleteApp() in /var/jenkins/workspace/pull-request-analyser-ng-simple@6/label/vm-slave-02/tests/lib/preferences.php on line 146

MorrisJobke · 2014-12-04T08:56:34Z

My last task for this PR will be the removal of all OC_Preferences references in core and apps I find in this github org.

DeepDiver1975 · 2014-12-04T11:58:48Z

My last task for this PR will be the removal of all OC_Preferences references in core and apps I find in this github org.

and fix the unit tests:

Test Result (3 failures / +3)

    Test\TestAllConfig.testDeleteUserValue
    Test\TestAllConfig.testDeleteAllUserValues
    Test\TestAllConfig.testDeleteAppFromAllUsers

MorrisJobke · 2014-12-04T14:58:21Z

Test Result (3 failures / +3) Test\TestAllConfig.testDeleteUserValue Test\TestAllConfig.testDeleteAllUserValues Test\TestAllConfig.testDeleteAppFromAllUsers

They work locally :( Let me have a look at the jenkins output.

MorrisJobke · 2014-12-07T20:56:39Z

hmmm ... why?

Many duplicated lines of code in the tests, because of the limits of the not fully mockable unit tests. Will come later.

LukasReschke · 2014-12-08T14:00:50Z

lib/private/preferences.php

-	 * @param string $value
+	 * @param $app
+	 * @param $key
+	 * @param $value


Missing type annotations

Oh ... they seems to have gone while refactoring. Will add them.

LukasReschke · 2014-12-08T14:12:17Z

👍

MorrisJobke · 2014-12-08T14:21:14Z

Just added type annotations for a deprecated interface. So jenkins can be ignored

* introduce SystemConfig to avoid DI circle (used by database connection which is itself needed by AllConfig that itself contains the methods to access the config.php which then would need the database connection - did you get it? ;)) * use DI container and use that method in legacy code paths (for easier refactoring later) * create and use getSystemConfig instead of query() in DI container

* keep old static methods - mapped to new ones and deprecated * removed deleteApp, getUsers, getApps because they are unused * make AllConfig unit tests more robust against not cleaned up environments

…pdate

* this needs to be properly fixed by a proper organisation of the base.php * introduced fixDIInit() in AllConfig that moves the injection of DatabaseConnection to a later point in time * problems mostly because of the autoconfig setup

MorrisJobke · 2014-12-08T21:35:14Z

New jenkins PR: #12710

MorrisJobke · 2014-12-08T21:41:41Z

Damn :( Rebase fuckup -.-

* files_encryption * files_versions * files_trashbin * tests * status.php * core * server container

MorrisJobke · 2014-12-08T21:46:07Z

@DeepDiver1975 @LukasReschke Ready for final review. The rebase just required the deletion of settings/ajax/userlist.phpand the adjustment of lib/private/user/user.php as there was code moved to. (both by @LukasReschke user list REST PR)

scrutinizer-notifier · 2014-12-08T21:47:43Z

The inspection completed: 214 new issues, 34 updated code elements

ghost · 2014-12-08T22:33:29Z

Waiting for jenkins in ... #12714

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/4070/
🚀 Test PASSed. 🚀

Config cleanup - OC_Preferences refactoring

MorrisJobke added 3 - To Review technical debt labels Nov 25, 2014

MorrisJobke added this to the 8.0-current milestone Nov 25, 2014

MorrisJobke changed the title ~~drop OC_Preferences::getUsers and getApps~~ More config cleanups Nov 25, 2014

MorrisJobke mentioned this pull request Nov 25, 2014

migrated OC_Preferences::deleteAppFromAllUsers to deleteApp in \OCP\ICon... #12366

Closed

MorrisJobke added 1 - To develop and removed 3 - To Review labels Nov 25, 2014

MorrisJobke changed the title ~~More config cleanups~~ [WIP] More config cleanups Nov 25, 2014

MorrisJobke added 2 - Developing and removed 1 - To develop labels Nov 27, 2014

MorrisJobke force-pushed the drop-getApps-getUsers branch from 47f0200 to 42460e0 Compare November 27, 2014 16:59

LukasReschke reviewed Nov 28, 2014
View reviewed changes

MorrisJobke force-pushed the drop-getApps-getUsers branch 3 times, most recently from 7ad355e to 343697e Compare November 30, 2014 17:29

nickvergessen reviewed Dec 1, 2014
View reviewed changes

DeepDiver1975 changed the title ~~[WIP] More config cleanups~~ More config cleanups Dec 1, 2014

DeepDiver1975 changed the title ~~More config cleanups~~ [WIP] More config cleanups Dec 1, 2014

MorrisJobke force-pushed the drop-getApps-getUsers branch from 38c9166 to d78a56b Compare December 3, 2014 18:35

MorrisJobke force-pushed the drop-getApps-getUsers branch from d78a56b to 71d2d3f Compare December 4, 2014 08:40

MorrisJobke mentioned this pull request Dec 5, 2014

Cleanup config handling - deprecate old ways to interact with config #12268

Closed

MorrisJobke force-pushed the drop-getApps-getUsers branch 2 times, most recently from 6b48c8e to ff5aee0 Compare December 5, 2014 15:25

MorrisJobke mentioned this pull request Dec 8, 2014

OCS API for server-to-server sharing #12452

Merged

1 task

LukasReschke reviewed Dec 8, 2014
View reviewed changes

MorrisJobke added 10 commits December 8, 2014 22:29

@deprecated messages for \OCP\Config and \OCP\AppConfig

d6da627

use old methods and redirect in deprecated methods

b01c59b

introduce proper interface for deleteAppValue

985b15f

introduce deleteAllUserValues

7190088

migrate \OC\AllConfig to \OCP\IConfig

a9e411e

fix base.php for not installed systems

f164161

Refactoring of OC_Preferences to AllConfig

f0b1032

* keep old static methods - mapped to new ones and deprecated * removed deleteApp, getUsers, getApps because they are unused * make AllConfig unit tests more robust against not cleaned up environments

introduce preCondition for setUserValue to provide atomic check-and-u…

af91ee9

…pdate

Workaround to fix the too early init dilemma

2d5fc9c

* this needs to be properly fixed by a proper organisation of the base.php * introduced fixDIInit() in AllConfig that moves the injection of DatabaseConnection to a later point in time * problems mostly because of the autoconfig setup

MorrisJobke force-pushed the drop-getApps-getUsers branch from e2d1fbd to 16099f3 Compare December 8, 2014 21:34

MorrisJobke mentioned this pull request Dec 8, 2014

Jenkins drop get apps get users #12710

Closed

MorrisJobke added 2 commits December 8, 2014 22:42

reduce OC_Preferences, OC_Config and \OCP\Config usage

0d4f0ab

* files_encryption * files_versions * files_trashbin * tests * status.php * core * server container

drop unused isDebugMode and setDebugMode of OC_Config

9a7362d

MorrisJobke force-pushed the drop-getApps-getUsers branch from 16099f3 to 9a7362d Compare December 8, 2014 21:43

MorrisJobke mentioned this pull request Dec 9, 2014

Jenkins drop get apps get users #12714

Merged

DeepDiver1975 added a commit that referenced this pull request Dec 9, 2014

Merge pull request #12406 from owncloud/drop-getApps-getUsers

c36bac3

Config cleanup - OC_Preferences refactoring

DeepDiver1975 merged commit c36bac3 into master Dec 9, 2014

DeepDiver1975 deleted the drop-getApps-getUsers branch December 9, 2014 08:36

lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config cleanup - OC_Preferences refactoring #12406

Config cleanup - OC_Preferences refactoring #12406

MorrisJobke commented Nov 25, 2014

MorrisJobke commented Nov 25, 2014

MorrisJobke commented Nov 25, 2014

LukasReschke Nov 28, 2014

MorrisJobke Nov 28, 2014

nickvergessen Dec 1, 2014

MorrisJobke Dec 1, 2014

LukasReschke commented Dec 3, 2014

MorrisJobke commented Dec 4, 2014

DeepDiver1975 commented Dec 4, 2014

MorrisJobke commented Dec 4, 2014

MorrisJobke commented Dec 7, 2014

LukasReschke Dec 8, 2014

MorrisJobke Dec 8, 2014

LukasReschke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

scrutinizer-notifier commented Dec 8, 2014

ghost commented Dec 8, 2014

Config cleanup - OC_Preferences refactoring #12406

Config cleanup - OC_Preferences refactoring #12406

Conversation

MorrisJobke commented Nov 25, 2014

MorrisJobke commented Nov 25, 2014

MorrisJobke commented Nov 25, 2014

LukasReschke Nov 28, 2014

Choose a reason for hiding this comment

MorrisJobke Nov 28, 2014

Choose a reason for hiding this comment

nickvergessen Dec 1, 2014

Choose a reason for hiding this comment

MorrisJobke Dec 1, 2014

Choose a reason for hiding this comment

LukasReschke commented Dec 3, 2014

MorrisJobke commented Dec 4, 2014

DeepDiver1975 commented Dec 4, 2014

MorrisJobke commented Dec 4, 2014

MorrisJobke commented Dec 7, 2014

LukasReschke Dec 8, 2014

Choose a reason for hiding this comment

MorrisJobke Dec 8, 2014

Choose a reason for hiding this comment

LukasReschke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

scrutinizer-notifier commented Dec 8, 2014

ghost commented Dec 8, 2014