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

Do not use F3 for configuration management #1261

Merged
merged 5 commits into from
Apr 27, 2021
Merged

Do not use F3 for configuration management #1261

merged 5 commits into from
Apr 27, 2021

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Apr 27, 2021

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.

@jtojnar jtojnar added this to the 2.19 milestone Apr 27, 2021
@jtojnar jtojnar force-pushed the config-container branch 2 times, most recently from c536cd7 to 860f07c Compare April 27, 2021 01:39
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.
while validating the changes from the previous commit.
It requires an extra option so let’s warn about that. And also do the same for wallabag.
It has been years since Wallabag 2 has been released so let’s allow people to remove wallabag_version=2 from their configs by making it the default value. This is potentially breaking change but hopefully, no one uses Wallabag 1 any more.
This will allow us to share them with tests and also make PHPStan aware of them.
@desbest
Copy link

desbest commented May 29, 2021

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

@jtojnar
Copy link
Member Author

jtojnar commented May 29, 2021

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
Copy link
Member Author

jtojnar commented Jul 10, 2024

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 🤦‍♀️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants