search

inpsyde / vip-composer-plugin

A Composer plugin to ease deployment to wordpress.com VIP servers alongside Composer-based local development.
MIT License
12 stars 0 forks source link

Add support for custom env config files in 'vip-composer-plugin-env-config' packages #14

Closed kuliebiakin closed 8 months ago

kuliebiakin commented 11 months ago

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

This PR introduces a feature that enables a flexible list of configuration files for environments.

What is the current behavior? (You can also link to an open issue here)

The current list of environments for vip-composer-plugin-env-config packages is limited to ["all", "development", "staging", "production"]. This limitation requires complex configurations in scenarios involving alternative naming conventions or combining of configurations for additional environments.

What is the new behavior (if this is a feature change)?

vip-composer.env-configs is adding to have a control over environments files from vip-composer-plugin-env-config package. This configuration contains an array of non-empty strings with environment names.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

There should be no breaking change since fallback to old value exists as a default configuration.

Other information:

It also makes it possible to use values other than objects in extra.vip-composer configuration.

kuliebiakin commented 10 months ago

Can I ask what is the logic that you would like to use to load the custom environments?

There is one extra environment used on project besides list from wp_get_environment_type. We prefer to have clear, separate files with configurations instead of trying to find workaround on how to handle specific constants, values etc. for a given environment.

However, I see mentioned issue with loading of these configuration files, but I would like to suggest possible solution with pluggable function or constant or mix of both:

if (! function_exists('vip_load_env_config')) {
    function vip_load_env_config() {
        $env = defined('VIP_ENVIRONMENT_TYPE') ? VIP_ENVIRONMENT_TYPE : wp_get_environment_type();

        if (file_exists("/path/to/envs/{$env}.php")) {
            require_once "/path/to/envs/{$env}.php";
        }

        if (file_exists("/path/to/envs/all.php")) {
            require_once "/path/to/envs/all.php";
        }
    }
}

Such approach could make it possible to just set custom environment name through VIP_ENVIRONMENT_TYPE constant or implement own way on how to load configuration files e.g. loading multiple configuration files for one environment or skipping all.php in some cases etc.

What do you think?

gmazzap commented 9 months ago

@kuliebiakin We have an installation where we have 7 environments, plus "local" that's 8.

What we do is map all environments that are not a match to wp_get_environment_type() to "staging".

So we have:

VIP ENV WP ENV
local local
development development
staging staging
env1 staging
env2 staging
env3 staging
env4 staging
production production

After that, the staging.php configuration file looks like this:

<?php

$vipEnv = defined('VIP_GO_APP_ENVIRONMENT') ? VIP_GO_APP_ENVIRONMENT : null;
if ($vipEnv && ($vipEnv !== 'staging') && file_exists(__DIR__ . "/{$vipEnv}.php")) {
    require_once __DIR__ . "/{$vipEnv}.php";
    return;
}

// Rest of config here...

This allow use to have env-specific config files for those "non-standard" environment with a "fallback" on staging.php. So this is possible today as well without custom environments.

Taking example of this we could have a function like:

function vip_load_env_config(): void
{
    $wpEnv = wp_get_environment_type();
    $vipEnv = defined('VIP_GO_APP_ENVIRONMENT') ? VIP_GO_APP_ENVIRONMENT : null;

    $envs = (is_string($vipEnv) && ($vipEnv !== '')) ? [$vipEnv] : [];
    $envs[] = $wpEnv;

    foreach ($envs as $env) {
        if (file_exists("/path/to/envs/{$env}.php")) {
            require_once "/path/to/envs/{$env}.php";
            break;
        }
    }

    if (file_exists("/path/to/envs/all.php")) {
        require_once "/path/to/envs/all.php";
    }
}

(IMO no need to be pluggable. The fact the function exists it does not mean you have to use it)

And that function would work well with the custom config you're proposing here. I guess it makes sense.

I'll have a better look at code, then.

gmazzap commented 8 months ago

@kuliebiakin I went ahead and implemented the changes we discussed in comments, because I want to have some more changes and would like to merge this first. Would you mind to have a look before we merge? Maybe even test for your use case?

kuliebiakin commented 8 months ago

@kuliebiakin I went ahead and implemented the changes we discussed in comments, because I want to have some more changes and would like to merge this first. Would you mind to have a look before we merge? Maybe even test for your use case?

@gmazzap , yes, sure. I'll test it tomorrow.

kuliebiakin commented 8 months ago

I checked and it worked as expected for me.

Steps that taken for testing