PHP constants handling in V3 about wpstarter HOT 9 CLOSED

Why the change

Let me first describe the reasons this is a breaking change has been introduced.

What does "all" variables means?

v2 declared a constant for each env var, v3 declares a constant only for the env variables whose name match WordPress constant name.

The first reason behind the change is that, to be precise v2 declared a constant for each env var that was declared in an env file.

In v3 developers are encouraged don't use env files outside of development ennvironment, but use real environment in production and production-like envs. Which means that there's no way to determine in advance "all" the variables to declare constants for.

Types

That said, assuming we could find a way env variables are always strings, meaning that if WP Starter declares constants for each of them, there could be issue with types.
Let's take the example of a plugin tht expect a config like const MY_FOO = true and then pass the value of this constant to a function that type-declare bool. By setting an env variable MY_FOO=true (MY_FOO=1) this would be turned into a string causing a type error, and WP Starter would have no way to know that MY_FOO as to be declared in a constant as boolean.

Security

Assuming we found a way to determine all env variables, how do we exclude those that are not supposed to be used in the web application?
Let's take the example of a system that has an env variable with API keys, and that is converted to a constant. A simple get_defined_constants() could leak that info.
Yes that is true also in case of WP constants like DB contants, but if I'm a plugin developer and I'm going to output the content of get_defined_constants() somewhere I know that I have to exclude constants holding DB configuration (and probably also nonces), but I can't know that there are other dangerous constants.

Conflicts

Env variables might be present in the system from several sources, even something that has nothing to do with WordPress. Let's take the example of assets builder that requires an env var DEFAULT_USER. Then let's assume that a webiste project decides to use a constant named in the exact same way. Normally constants should be prefixed with something unique, but how do we warranty that?

I would not go back

So I don't hink that going back to v2 behavior is doable. If not anything else, the requirement of .env files instead of real environment was one of the major reasons v3 was written at all.

Alternative solution

I would avoid to use composer.json to define the env var to load, it would be impossible to define constants per environment and honestly not that ideal to maintain.

I am thinking about having a list of "white-listed" env vars in an env variable like:

WP_STARTER_ENV_TO_CONST="MY_PLUGIN_CONSTANT,MY_OTHER_CONSTANT"

and then in wp-config, after WordPress constants are setup (so after this line: https://github.com/wecodemore/wpstarter/blob/dev/templates/wp-config.php#L36), there could be a code like:

$toDefine = explode(',', $envLoader->read('WP_STARTER_ENV_TO_CONST') ?: '');
foreach ($toDefine as $envVar) {
 	$envLoader->defineCustomConstant($envVar);
}

where defineCustomConstant() would be very similar to the current WordPressEnvBridge::defineWpConstant().

This workflow would be very flexible, quite safe, and compatible with current workflow and caching system.

The only thing still impossible to fix would be the possibility to store types other than string in constants, something that we can do for WordPress because the expected type of WordPress constants is known (https://github.com/wecodemore/wpstarter/blob/dev/src/Env/WordPressEnvBridge.php#L18-L177).

Maybe we could imagine an optional "type" info to be passed in the env var, e.g.:

WP_STARTER_ENV_TO_CONST="ONE_CONFIG:INT,ANOTHER,YET_ANOTHER,LAST:BOOL"

How this solution look?

from wpstarter.

Merged and released in https://github.com/wecodemore/wpstarter/releases/tag/3.0.0-beta.3

from wpstarter.

My bad. There were still the early hooks file in place that interfere with the constant declaring. Tested with cache and without and it works as expected.

from wpstarter.

Thanks for the detailed elaboration. Your suggestion of a WP_STARTER_ENV_TO_CONST variable is a good idea. Do you accept a PR or want to implement it on your own?

from wpstarter.

@dnaber-de I can't work on it next few days, if you have time and will to do it, feel free to send a PR. And thanks!

from wpstarter.

@dnaber-de I realized this needs more work than originally thought. I will take care of it because I need to be compliant with other changes I'm working on.

from wpstarter.

@dnaber-de this is implemented in dev branch. Tests are very welcome.

It still needs documentation, but it is implemented as described above: you need a WP_STARTER_ENV_TO_CONST env var that contains a comma-separated list of env vars you want to turn into constants.

from wpstarter.

Tested (without cache) and it works. However, it would be more useful if the constant doesn't get defined in case the env variable is not defined. That way you can fall back to defaults by just comment out a env variable in .env. Right now I have to comment the variable out and edit a (probably very long) string value of WP_STARTER_ENV_TO_CONST.

from wpstarter.

Do you mean if onw of the env variables defined in WP_STARTER_ENV_TO_CONST is not defined?

In that case no constant is defined.

As you can see here: https://github.com/wecodemore/wpstarter/blob/dev/src/Env/WordPressEnvBridge.php#L613-L619

If reading the env returs null and that's the case if no env var is set, then define never happen.

Or I missed something?

from wpstarter.