Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup I*Config classes #12260

Closed
3 of 5 tasks
PVince81 opened this issue Nov 18, 2014 · 14 comments
Closed
3 of 5 tasks

Cleanup I*Config classes #12260

PVince81 opened this issue Nov 18, 2014 · 14 comments
Milestone

Comments

@PVince81
Copy link
Contributor

So far we seem to have lots of classes and interfaces just to read the config, some legacy and some new.

We need to clean up this mess.

Here are a few:

  • IConfig
  • OC\Preferences
  • IAppConfig
  • OC_Config
  • OC_AppConfig

@icewind1991 which ones do we want to keep and which ones should go ?

@BernhardPosselt
Copy link
Contributor

You need to split the configs based on their dependencies:

  • You can always read the config/config.php
  • In order to read app and user values you need to first read the config/config.php

->

  • Keep IConfig as is and let it read all values but inject the class that is required to read and write the config/config.php into it so you can also set system values. getSystemValue and setSystemValue on the IConfig class just call through to the one that reads the config/config.php
  • Get rid of all the other stuff that makes sense

@MorrisJobke

@MorrisJobke
Copy link
Contributor

see also stuff like #13736 and #13104

@MorrisJobke
Copy link
Contributor

Drop of OC_Preferences: #13869

@MorrisJobke
Copy link
Contributor

Drop of \OC\Preferences #13870

@DeepDiver1975
Copy link
Member

While reviewing #13938 I really wonder if IAppConfig shall die ❓

It's an isolated and easy to use object.

@BernhardPosselt
Copy link
Contributor

Stuff that is only available in IAppConfig:

  • Delete all entries for an app
  • Delete a key

The rest is code duplication. We might want to add those methods to the IConfig interface and then kill it, that is if those methods are actually used in our code

@MorrisJobke
Copy link
Contributor

@Raydiation see #13938 ;)

@MorrisJobke
Copy link
Contributor

@DeepDiver1975 We can also handle it all in IAppConfig, but we shouldn't provide two ways of doing this. Either in IConfig OR in IAppConfig.

@MorrisJobke
Copy link
Contributor

The "or" is an exclusive one ;) Read it as an XOR ;)

@BernhardPosselt
Copy link
Contributor

IMHO makes no sense to put the setting of app config values in one class and the deletion of values in another class :)

@MorrisJobke
Copy link
Contributor

IMHO makes no sense to put the setting of app config values in one class and the deletion of values in another class :)

I moved all missing methods over to IConfig and deprecated IAppConfig with #13938 ;) The question here is if we want to do it in this way, or if we want the AppConfig be a separate interface, but then I would remove the methods from IConfig.

@BernhardPosselt
Copy link
Contributor

Separate interface makes only sense if we have the exact same signatures. That way we could keep backwards compatibility and let iconfig inherit from iappconfig.

@MorrisJobke MorrisJobke added this to the 8.2-next milestone Apr 7, 2015
@ghost ghost modified the milestones: 9.0-next, 8.2-current Sep 10, 2015
@MorrisJobke
Copy link
Contributor

Latest cleanups went into 9.0 -> closing

@MorrisJobke MorrisJobke removed their assignment Jan 8, 2016
@lock
Copy link

lock bot commented Aug 7, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants