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

Remove config/addon-sample.config.php file in favor of per-addon configuration files #12225

Merged
merged 2 commits into from Nov 20, 2022

Conversation

MrPetovan
Copy link
Collaborator

@MrPetovan MrPetovan commented Nov 20, 2022

Address #12135 (comment)

I got rid of the unique config/addon-sample.config.php file in favor of per-addon configuration file given #12135 (comment) where the documentation assumes some pre-existing knowledge about the config files that shouldn't be necessary. This change won't have any effect on existing installs as the existing config/addon.config.php file will still be loaded as part of the config/ folder. We just don't advertise its existence in the documentation anymore.

@nupplaphil
Copy link
Collaborator

nupplaphil commented Nov 20, 2022

I think it's now more confusing than before and I'm not sure what regressions are now possible because of the double call (because manually reloading the config inside App, it's not even sure if we catch every occurrence - like for console commands).

There are currently 5 (ordered) kind of config sources

  • SOURCE_STATIC => values from default.config.php and settings.config.php
  • SOURCE_FILE => any value from a custom *.config.php file from config/ or addon/*/config/
  • SOURCE_DB => any value from the config table
  • SOURCE_ENV => any value from an Environment variable
  • SOURCE_FIX => any "fixed" value, a config could set manually in the code (currently not used)

This order is strict and ensures the order static < file < db < env

Now the other dimension is the timing and I would like to introduce the static folder in the addons as well (instead of reloading the config again), so we would have:

  • Loading static/ values
  • Loading addon/*/static/ values
  • Loading config/ values
  • Loading addon/*/config/ values
  • Loading the DB
  • Loading the Environment

For me, it's better understandable than reloading the core config again :-)

[edit] Therefor I'd say let's move the Core\Hook::callAll('load_config'); inside the Config class to avoid this reloading ...

@nupplaphil
Copy link
Collaborator

But I think I get it ... örks ...
Loading the Addons static files would needed to be after loading the db, but before loading the config, which isn't possible because the DB config is loaded through the "normal" config loading .. omg ..

@MrPetovan
Copy link
Collaborator Author

I got to the same point as you and got stuck there.

From the priority list apparently I didn't need to reload the env since it would be still be priority over the file that are loaded again.

@nupplaphil
Copy link
Collaborator

nupplaphil commented Nov 20, 2022

Ah I think this should work:

  • create a static/ folder for each addon with default values
  • Each addon with static folder should create a addon_load_static_config() function and load it's static files with the parameter SOURCE_STATIC (=> so it won't override the config/ values anymore)
  • add a new Core\Hook::callAll('load_static_config', $loader); call at App

@MrPetovan
Copy link
Collaborator Author

Oooh, shiny. However the only thing we need is to set SOURCE_STATIC in the current hook and we're good. I'll get on it.

@MrPetovan
Copy link
Collaborator Author

Here you go.

@MrPetovan MrPetovan changed the title Load config/ files again after calling load_config hook Remove config/addon-sample.config.php file in favor of per-addon configuration files Nov 20, 2022
…iguration files

- Update documentation and .gitignore
doc/Config.md Outdated Show resolved Hide resolved
Co-authored-by: Philipp <admin+Github@philipp.info>
@nupplaphil nupplaphil merged commit 41be03a into friendica:develop Nov 20, 2022
@MrPetovan MrPetovan deleted the bug/10188-addon-config branch November 20, 2022 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants