[RFC] Removing the Config class

[SOF3]

Before anyone does it,

DO NOT JUST USE THE :+1:/:-1: REACTION.

This issue contains many points, and we don't know which point you are voting on. If you want to express your opinion, no matter you agree or disagree, please post a comment below stating what you think and why you think so (it is fine even if it is duplicate). Put :+1:/:-1: on the comments below, not this issue body.

This is an RFC. See the guidelines for RFCs in CONTRIBUTING.md.

Content

Motivation

The Config class is a mix of unrelated methods on multiple serialization formats, having caused API unsoundness (such as ->set($key, true) on the LIST type) and undocumented behaviour (such as fixYAMLIndices) and difficulty in accessing data (see lots of people on forums and discord asking how to push to mapping list property). It is proposed that the actual features of Config be split down into actual documented utility functions for higher reusability and maintainability.

Changes

Deprecate the Config class

Deprecate and eventually remove Config in API 4.0.0. Most usages can be directly replaced by yaml_parse_file.

The getConfig() method in plugins is very often replaced by custom invocations on yaml_parse, and keeping it in the PluginBase class has little to no value. Since default config is not automatically saved by getConfig() anyway, saveResource("config.yml") followed by $this->config = yaml_parse_file($this->getDataFolder() . "config.yml"); would be equivalent. (I would furthermore want to remove saveDefaultConfig() as an unnecessary specialization, but that is not relevant to this issue).

Polyfilling existing behaviour

getNested and setNested

Provide util functions getNested and setNested, which act on an actual arrays rather than forced on a Config object.

Note that getNested and setNested are mostly useful for dynamic user input. From the developer perspective, it is more sound to use $array["foo"]["bar"] than Utils::getNested($array, "foo.bar").

fixYAMLIndices

This method is very poorly documented. It is possible to keep it as an util function, but given the unreliable nature of regex replacement (doesn't work with {x: 1, y: 2, y: 3} inline mappings), it is probably more reliable to get a fix at the ext-yaml level or adoption of another YAML library (or use NEON?).

Properties format

The Config class was primarily used to parse INI files. This behaviour is useful for server.properties and other possible INI files users might want to read. Provide util functions parseProperties and writeProperties which read properties into an associative array.

List format

The Config::LIST format is rarely used, and even if it is used, it can be trivially replaced with a file() call (along with array_map("trim")). I do not see any motivation to keep it.

Filling defaults

Filling defaults was a useful feature back in 2013 when there was no ?? operator, but this is probably no longer useful. Furthermore, this leads to duplicate definition of default values. (Default config.yml and get() call are already two duplicates, and new Config would be the third, which is why nobody uses it anymore)

Possible enhancement

In the future, type-checked config parsing can be provided as a standalone library bundled in PocketMine and used in pocketmine.yml. This feature is possibly better implemented after PHP 7.4 with class property type hints.

[dresnite]

I agree with all of the reasons stated by SOFe. This class doesn't comply with the SRP, and the little user-level utility it has can be replaced with PHP features like the null coalescing operator. In my opinion, this class is a symptom of bad code design.

[JaxkDev]

I also agree with this, firstly from my POV reviewing plugins so many abuse the config class for storing data and other useless 💩 that doesn’t need to be a config class it’s simply used because it’s there and just copy paste, I believe deprecating & removing Config will lead ‘new’ developers to find more appropriate ways of handling data from files whether that’s through raw parsing or possibly virions which introduce more availability of other formats such as MySQL, SQL files, JSON etc. While this is still possible without removing Config I think removing it would ‘force’ people as such to find correct methods of handling large blobs of data.

Secondly extending what quartz has said, I also see a lot of getAll() and then just setAll() and save(), this can all be replaced with yaml_parse_file and yaml_emit_file respectively, as mentioned it was primarily for INI files (don’t know why) but I have never seen a plugin use the INI format so I find no reason to keep the Config class if that was its primary purpose

PS PHP 8 is introducing the Null safe operator which would make getting nested values even easier !

[SOF3]

it was primarily for INI files (don’t know why) but I have never seen a plugin use the INI format so I find no reason to keep the Config class if that was its primary purpose> @JaxkDev The INI format is used in server.properties. Even if no plugins use it, we still need it for backward compatibility.

See also #2532 on the issue of split server configuration.

[dktapps]

Another point to mention is that Config just gets in the way when using stuff like respect/validation or netresearch/jsonmapper. Particularly jsonmapper, which provides its own much more robust way to interact with data that static analysers can understand.

[provsalt]

Bye bye the old YAML abuse where people would store data in Configs as they can't be bothered to use a sql or nosql databases.

[dktapps]

Removing Config won't solve that problem.

[alvin0319]

Removing Config won't solve that problem.

But Config has many useless functions...

In Korea, Many people use config like this, $config = new Config($this->getDataFolder() . "Config.yml", Config::YAML, []);.

In this case, some functions in Config class (e.g Config->setJsonOption()) become useless

It could be a different story, but many people just use Config to load and save data. For example, many people use Config->getAll() in onEnable() to get all the data as an array, and in onDisable() Config->setAll($array) and Config->save() to save data. If you use this method, functions such as Config->setNested() and Config->getNested() in the Config class become useless.

[dktapps]

this is irrelevant to my point, which is that removing the Config class will not stop people using yaml as a database.

[SOF3]

This RFC is of no intention to deprecate usage of YAML, JSON or properties as data serialization formats. The whole point is to, as stated in the Motivation section, to minimize API unsoundness and undocumented behaviour caused by reinventing the wheel. "Encouraging" certain selections of technologies is never a reason for an API break.

[SOF3]

Regarding concerns over the hasChanged function, according to this search on all plugins tracked by Poggit, there are only three usages of Config::hasChanged(), as well as PocketMine's own server.properties logic. Nevertheless, it is unrealistic to keep an array wrapper only for the sake of tracking whether the file is changed, and so I don't think it is a good reason against the removal of Config class.