Configuration Refactor & .env Integration#653
Configuration Refactor & .env Integration#653JohnVillalovos merged 4 commits intoLibreBooking:developfrom
Conversation
|
@lucs7 Thanks. One question. What do you think about having all of the environment variables prefixed with I'll work on finding some time over the weekend to review the whole PR. |
|
Added the prefix 'LB_' similar to how the docker imgage works |
871ddb5 to
3f7621c
Compare
98a5f09 to
fd0ff77
Compare
b3a3e32 to
3e78076
Compare
8912b58 to
d8d8443
Compare
b9e9499 to
92a08d1
Compare
|
@JohnVillalovos |
b7efc00 to
195423b
Compare
JohnVillalovos
left a comment
There was a problem hiding this comment.
Thanks @lucs7
I did my best to review this but it was very large. In the future we should try to have smaller PRs and smaller commits.
But I will approve this.
1. Support for .env file loading via phpdotenv
- Added EnvHelper to load environment variables
- Example .env file provided in .env.example
- Configurator now loads environment variables from .env
2. Major restructure of config to nested array for main config and plugin configs
- Renamed configs keys to be more consistent
- Support of legacy structure and naming through ConfigKeys::$legacyKeyMap
- Scheme and validation through ConfigKeys
- Updated Configurator to use ConfigKeys for templating
Example ConfigKeys
public const APP_TITLE = [
'key' => 'app.title',
'legacy' => 'old.key',
'type' => 'string',
'default' => 'LibreBooking',
'label' => 'App title',
'description' => '...',
'choices' => ['option 1' => 'Option 1', 'option 2' => 'Option 2'],
];
keys are now called by their Config key without section through getKey. all section magic is handled automatically
ConfigTest.php and FakeConfig.php previously had methodnotfound errors that have been fixed
195423b to
d9b801e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors LibreBooking's configuration system to modernize PHP config files and integrate environment variable support using .env files. The changes enable better deployment practices and maintain backward compatibility.
- Modernizes config files from legacy
$confarray format to return-based PHP arrays - Introduces .env file support for environment-based configuration overrides
- Updates plugin configuration system with new ConfigKeys classes for better type safety
Reviewed Changes
Copilot reviewed 267 out of 268 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tpl/Admin/Configuration/manage_configuration.tpl | Updates admin configuration UI with dynamic field rendering and environment variable awareness |
| tests/fakes/FakeConfig.php | Refactors test configuration to support new config definition format |
| tests/data/* | Adds test data files for new config formats and environment variable testing |
| tests/WebServices/* | Updates test files to use new ConfigKeys constants |
| tests/Presenters/* | Updates presenter tests to use new configuration format |
| tests/Infrastructure/Config/ConfigTest.php | Adds comprehensive tests for new configuration system |
| tests/Domain/* | Updates domain tests for new config format |
| tests/Application/* | Updates application tests for new config format |
| plugins/Authentication/* | Refactors all authentication plugins to use new ConfigKeys classes |
| plugins/PreReservation/* | Updates pre-reservation plugins for new config format |
Comments suppressed due to low confidence (3)
tpl/Admin/Configuration/manage_configuration.tpl:37
- [nitpick] The property name 'hasEnv' doesn't follow consistent naming conventions. Consider renaming to 'HasEnv' to match the other property naming pattern used in the template.
{assign var="disabled" value=$setting->hasEnv ? 'disabled' : ''}
tests/fakes/FakeConfig.php:65
- The error message 'Config definition not found' is misleading. It should clarify that the config definition must be an array with a 'key' field, not that it wasn't found.
if (!is_array($configDef) || !isset($configDef['key'])) {
tests/Infrastructure/Config/ConfigTest.php:97
- The test should clean up environment variables after the test runs to avoid affecting other tests. Consider using tearDown() or try-finally to ensure putenv() changes are reverted.
putenv('DEFAULT_TIMEZONE=Europe/Berlin');
| private function GetConfig($configDef, $converter = null) | ||
| { | ||
| return Configuration::Instance()->File(self::CONFIG_ID)->GetKey($keyName, $converter); | ||
| return Configuration::Instance()->File(WordPressConfigKeys::CONFIG_ID)->GetKey($configDef, $converter); |
There was a problem hiding this comment.
The method signature change from GetConfig($keyName) to GetConfig($configDef) is inconsistent across plugins. Some plugins still use $keyName parameter name while others use $configDef. This could confuse developers and should be standardized.
| return true; | ||
| }; | ||
| } | ||
| ; |
There was a problem hiding this comment.
Unnecessary semicolon after closing brace. This is likely a leftover from refactoring and should be removed for cleaner code.
| ; |
| $pair = explode('=', trim($attributePair)); | ||
| $attributes[trim($pair[0])] = trim($pair[1]); | ||
| if (count($pair) === 2) { | ||
| $attributes[trim($pair[0])] = trim($pair[1]); |
There was a problem hiding this comment.
The code assumes explode will always return an array with at least 2 elements when checking count($pair) === 2, but the closing brace and return statement are outside this if block. This could lead to incomplete attribute mapping if the format is incorrect.
| $attributes[trim($pair[0])] = trim($pair[1]); | |
| $attributes[trim($pair[0])] = trim($pair[1]); | |
| } else { | |
| error_log("Invalid attribute mapping pair: '{$attributePair}' in CAS attribute mapping configuration."); |
|
@lucs7 I rebased this and fixed the conflicts. |
config.*.php
configuration.php
ConfigKeys.php
.env
plugins