fossar / selfoss

multipurpose rss reader, live stream, mashup, aggregation web application
https://selfoss.aditu.de
GNU General Public License v3.0
2.36k stars 343 forks source link

Do not use F3 for configuration management #1261

Closed jtojnar closed 3 years ago

jtojnar commented 3 years ago

Fatfree is badly designed and should not have lived past 2005. Most notably, it relies on a single huge singleton object, which makes testing difficult. What is worse, the unholy things it does to error handling makes PHPStan and PHPUnit crash. For these reasons and other pains, selfoss will be gradually removing its use. Configuration values are read throughout the codebase so I decided to tackle those first.

Ideally, we would construct a configuration container using something like https://gitlab.com/Khartir/typed-config but that requires more recent PHP version than we support. Other configuration libraries either suffer from the same problem, are untyped like f3 was (domains of the options are known so we should not have to cast the string loaded from the ini file at the point of use), or are completely undocumented. Custom solution had to be implemented.

This patch adds helpers\Configuration class, which will load data from config.ini and environment variables and store them in properties. The class is injected by Dice into whichever other class that needs access to configuration. In addition to reducing the percentage of code paths tainted by F3, it will also bring better developer experience since editors and other tools can make use of the property annotations.

Since the default values of configuration option are now stored as default values of the class properties, defaults.ini is no longer used. It will still be included in the distribution zipball for convenience (generated from the code.

In the future, we can move the option descriptions into the class and have the options documentation generated as well.

desbest commented 3 years ago

Out of curiosity, what is wrong with Fat Free Framework? Why don't you recommend it?

jtojnar commented 3 years ago

There is just so many issues and problems with it I had to work around in selfoss. Many of them just stem out of its old age and are impossible to fix without breaking backwards compatibility and massive rewrite unfortunately.

These are just a few I remember at the top of my head:

  1. It is monolithic – ­you need to pull in the whole framework, even if you do not want to use some of its components. And what is worse, you cannot avoid using some components like error handler, and the default one is extremely invasive.
    • It is curious that the framework calls itself Fatfree when there is so much code I would cut out. One could take great inspiration from how Nette framework split and also Symfony split.
  2. It has questionable design:
    • It uses a singleton class that works as a configuration container. Well, you can create a new instance of the Base class but it will still internally work with the singleton behind your back. That makes testing hard since you cannot just inject a fresh F3 instance into a test.
    • The container stores random untyped values so you have no benefit of static checking.
    • The Base class modifies many PHP configuration values (error handler) on its construction. And what is worse, the file containing the class automatically creates the Base class instance. This means that when you even mention F3 and autoloader includes it, your PHP state changes (breaking error handlers PHPUnit set up, for example). ← This was the last straw for me.
  3. The code base is messy and unorganized – just look at it.
    • The code trades clarity for terseness (e.g. lack of brackets for control structures, complex predicates in if statement conditions, assigning variables deep within those…). That makes debugging unnecessarily harder.
    • It also makes it much easier to introduce bugs.
  4. It does not use namespaces so we cannot use some class names or we get name clashes.
    • Okay, using namespaces in selfoss that would allow us to have Image class. But sometimes I want to test an old library (I would add namespace support to it when I decide to adopt them) and it clashes too.
jtojnar commented 2 months ago

Fuck, I forgot we still use this piece of work. Just spent almost three hours debugging why Symfony console in #1283 fails with Command "/" is not defined.. Turns out Fatfree adds / to $_SERVER['argv'] when no arguments are provided 🤦‍♀️