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

Switch cypht config hm3.* to dotenv #823

Merged
merged 5 commits into from
Dec 14, 2023
Merged

Conversation

Shadow243
Copy link
Member

Task Description: Integrate Symfony Dotenv Package into Cypht Project

Details:
In response to GitHub Issue #813, the Symfony Dotenv package has been added to the Cypht project. This integration is part of an effort to standardize the handling of environment variables across different projects, including Cypht, Tiki, and Tiki Manager.

Next Steps:

  1. Confirm the successful integration of the Symfony Dotenv package into the Cypht project.
  2. Verify that environment variables are appropriately managed and utilized within the project.
  3. Consider extending this standardized approach to other related projects like Tiki for better alignment.

@Shadow243 Shadow243 force-pushed the symfony-dotenv branch 2 times, most recently from faf567b to 24182cd Compare November 10, 2023 12:31
@Shadow243
Copy link
Member Author

should i perhaps call

$environment->load();//load env files

in index.php ? after this:

/* get configuration */
$config = new Hm_Site_Config_File(CONFIG_FILE);

/* setup ini settings */
if (!$config->get('disable_ini_settings')) {
   require APP_PATH.'lib/ini_set.php';
}

@marclaporte marclaporte requested a review from kroky November 10, 2023 15:43
@kroky
Copy link
Member

kroky commented Nov 14, 2023

The #813 requests the ability to load config variables through env variables. We don't need dotenv package or .env file to do this. We just need to look at the ENV array or getenv function first before falling back to hm3.ini ones loaded in site config.
I don't recommend we do a partial switch to dotenv. We should either replace hm3.ini and all related ini file functionality with dotenv (which also includes unit tests, Tiki integration, config generation and probably other aspecs of the app) and be sure to keep the existing verbose comments in that config file OR keep the old ini file with db credentials and allow some of the variables to be overridden by the env. First approach is a big refactor while second one only addresses the issue request.

@Shadow243 Shadow243 marked this pull request as draft November 16, 2023 21:18
@Shadow243 Shadow243 force-pushed the symfony-dotenv branch 3 times, most recently from dee9f5a to c9c7a65 Compare November 18, 2023 00:15
@Shadow243 Shadow243 force-pushed the symfony-dotenv branch 2 times, most recently from 7dd7fb5 to 14f11a3 Compare November 22, 2023 00:00
@Shadow243 Shadow243 marked this pull request as ready for review November 22, 2023 00:04
@Shadow243
Copy link
Member Author

Shadow243 commented Nov 22, 2023

I created a folder config with *.php returning each file an array, and migrated all hm3.ini with comments inside to keep the same structure as requested, we keep the same behavior (for example to enable a module we just have to uncomment it inside config/app.php, no need to re-run the config_gen.php script). All configurations have been switched to dotenv.

INSTALL file not yet updated to support new changes.

@Shadow243 Shadow243 changed the title Allow database authentication using .env file Switch cypht config hm3.* to dotenv Nov 22, 2023
@kroky
Copy link
Member

kroky commented Nov 23, 2023

Wow, quite a refactor! I like the approach but can we streamline 2 issues here?

  1. The repeated calls to merge_config_files with params passing the config folder location - no need to repeat all this. I think we should simply parse the config folder location anywhere Hm_Config_... is initialized or used - in fact, it can be part of this class initialization process.
  2. All module .ini files should follow the same syntax as we use now for app config - either .php config files or .env files.

@Shadow243
Copy link
Member Author

This commit addresses two key issues in the current codebase to enhance the configuration handling process. The proposed changes aim to improve code organization, eliminate redundancy, and promote consistency in the configuration structure.

Changes

Issue 1: Duplicated Config Folder Location

The repeated calls to merge_config_files with manually passed configuration folder locations are removed. Instead, the refactor suggests parsing the config folder location within the initialization process of the Hm_Config_... class. This approach centralizes the configuration folder handling logic, improving code readability and maintainability.

Issue 2: Standardizing Module .ini Files

All module .ini files are refactored to follow the same syntax used for app configuration. This involves adopting either .php config files or .env files for module configurations, ensuring a consistent structure throughout the application.

Constructor Parameter for PHPUnit Tests

In the Hm_Site_Config_File class, a constructor parameter has been added:

public function __construct($all_configs = []) {
    $this->load(empty($all_configs) ? merge_config_files(APP_PATH.'config') : $all_configs, false);
}

This parameter allows for handling PHPUnit tests that use custom config files other than those found in config/*.php. It provides flexibility for testing scenarios where alternative configurations are necessary.

lib/framework.php Outdated Show resolved Hide resolved
lib/framework.php Outdated Show resolved Hide resolved
scripts/config_gen.php Outdated Show resolved Hide resolved
@kroky
Copy link
Member

kroky commented Dec 4, 2023

Looks good to me code-wise. Can maybe @josaphatim or somebody from the team test it a bit more - enable/disable some modules, regenerate config, change config/.env settings, etc.

Also, @Shadow243 once this is merged, the first thing we should do with Tiki integration master branch is get the latest Cypht and update the integration to use the latest .env setup.

@josaphatim
Copy link
Member

Looks good to me code-wise. Can maybe @josaphatim or somebody from the team test it a bit more - enable/disable some modules, regenerate config, change config/.env settings, etc.

Also, @Shadow243 once this is merged, the first thing we should do with Tiki integration master branch is get the latest Cypht and update the integration to use the latest .env setup.

@kroky I will test this

.env.dist Outdated Show resolved Hide resolved
.env.dist Outdated Show resolved Hide resolved
@josaphatim
Copy link
Member

@kroky I tested this and It is working. I enabled/disabled modules and also ran php unit tests.

@Shadow243 Just one more thing, there was a hm3.sample.ini file and hm3.ini that we used for our own configurations and present in .gitignore file. But in this new architecture there is only .env.dist. I think we should have a .env.example and then change the INSTALL file according to this.

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

Successfully merging this pull request may close these issues.

4 participants