Discussion: Should we encourage Config::get?

[tangrufus]

• I've read the guidelines for Contributing to Roots Projects
• This request isn't a duplicate of an existing issue
• I've read the docs and followed them (if applicable)
• This is not a personal support request that should be posted on the Roots Discourse community

Discussion

Should we encourage Config::get?
Or, should we encourage constant() as single source of truth?

wp-config/src/Config.php

Lines 34 to 42 in 2ae55cc

	public static function get($key)
	{
	if (!array_key_exists($key, self::$configMap)) {
	$class = self::class;
	throw new UndefinedConfigKeyException("'$key' has not been defined. Use `$class::define('$key', ...)`.");
	}
	return self::$configMap[$key];
	}

Values in Config::$configMap is not guaranteed to be define-d as constants (See: Config::remove and #4).

Therefore, usages of Config::get('SENTRY_DSN') and similar in #3 (comment) is unsafe.

cc @kellymears

[kellymears]

i feel like get, as a verb, especially attached to a public method, strongly implies that this is the getting place. i think if the method is lacking in some way that should be addressed but the method name is just too dang friendly to simply discourage the use of it. i could see transitioning the current get to a different method that is named more discouragingly and replacing it with something that provides more assurance.

one thing i love about it is that it provides an api for accessing config values. i get that it is duplicative and that there are other ways to handle this in PHP, included libs, and WP — but by making it a general practice to only use constants directly once (when setting them) one minimizes the API surface area for potential problems. config::get is the most visible candidate and thus quite likely the first method most will reach for having come to a similar conclusion.

ultimately, the accessibility of the method, taken in hand with the simplicity of the method name and the desirability of what it provides, make me think discouraging people from using it might be a bit like fighting the tide. it would be better to assume people are going to use it this way and try to make it safer for them.

[tangrufus]

If we are going to keep Config::get, we can:

• prevent Config::get before apply, and
• prevent Config::define and Config::remove after apply.

Something like (of course, there are different patterns to implement it):

class Config
{
    /** @var ConfigInterface **/
    protected static $config;

    public static function init(): void
    {
       // maybe throw exception if not null.
       if (static::$config === null) {
              static::$config = new PendingConfig();
       }
    }

    // Delegate to  static::$config;
    public static function define(string $key, $value): void

    // Delegate to  static::$config;
    public static function get(string $key)
    
    // Delegate to  static::$config;
    public static function remove(string $key): void

    // maybe delegate to  static::$config; and let static::$config return a new object.
    public static function apply()
    {
        if (static::$config instanceof PendingConfig) {
            // Same as v1 Config::apply
            
            // Then,
            static::$config = new AppliedConfig();
        } else {
            throw new Exception('xxxx');
        }
    }
}

class PendingConfig implements ConfigInterface
{
    /**
     * @var array<string, mixed>
     */
    protected $configMap = [];

    // Same as v1 Config::define
    public function define(string $key, $value): void

    public function get(string $key) 
    {
        throw new ConfigNotYetAppliedException('xxxx');
    }
    
    // Same as v1 Config::remove
    public function remove(string $key): void
}

class AppliedConfig implements ConfigInterface
{
    public function define(string $key, $value)
    {
        throw new ConfigAlreadyAppliedException('xxxx');
    }

    public function get(string $key) 
    {
        return defined($key) 
        ? constant($key) 
        : throw new ConfigNotFoundException('xxxx');
    }
    
    public function remove(string $key): void
    {
        throw new ConfigAlreadyAppliedException('xxxx');
    }
}

---

If we are going to remove Config::get, then we can tell everyone to use constant.

---

I vote for removing Config::get and make constant the single source of truth because the goals for this package (roots/bedrock#380) is to define constants in a fail safe way. wp-config's purpose is fulfilled once constants are defined. Thus, wp-config should end after Config::apply is invoked.

Let's discuss.

[BWBama85]

Right now Bedrock conflicts with at least one popular WordPress plugin, https://ithemes.com/ iThemes Security, because iThemes also defines DISALLOW_FILE_EDIT: https://wordpress.org/support/topic/unable-to-install-on-local-development-using-trellis-bedrock/

Since Bedrock is defining before plugins can attach their constants, there needs to be more than an exception thrown when there is a conflict.

If a plugin has already set a constant, Bedrock should not mess with it, in my opinion. So simply checking if each constant Bedrock defines already exists and then doing nothing if it does should be best practice. Thoughts?