Cms: parseEnv enabled settings should be parsed on retrieval

[timkelty]

Code consuming env-enabled settings shouldn't have to explicitly parse them, it should happen on retrieval, either via __get or a getter.

Otherwise plugin devs have to explicitly know which settings are env-enabled, and keep up with any additions, which seems unnecessary.

Example:
I shouldn't have had to do this: https://github.com/nystudio107/craft-imageoptimize-thumbor/blob/develop/src/imagetransforms/ThumborImageTransform.php#L270

We'll have to be careful that getSettings is overwritten so that the parsed values are not stored in the database.

[timkelty]

What might be an even better idea would be to introduce this in craft\base\SavableComponent, adding a SavableComponent::envSettings prop that was an array of attribute to automatically parse for env on __get.

This way, plugin devs could employ the same prop in their settings models and automatically have env settings on the given props.

[khalwat]

I too would love to see this formalized, rather than having each plugin developer that wants to support env vars re-invent the wheel (sometimes not the best way).

[mattstein]

I'm not sure if this is off topic, but it could also be nice to have an optional default in case the variable doesn't parse.

Example: I need REST API keys in a plugin, and using ParseEnv I've also started to check whether the user-provided variable is parsed. Previously a non-empty string would suffice, but now I need to be sure a user-provided variable like $API_KEY parses into some meaningful string and not literally $API_KEY: https://github.com/workingconcept/snipcart-craft-plugin/blob/master/src/models/Settings.php#L394-L433

While I might only be illustrating @khalwat's questionable-wheel-reinventing, the extra handling feels like it should be higher up in the chain.

[brandonkelly]

The main reason Craft doesn’t parse environmental setting values further up the chain is because it’s important that each component is able to remember their raw setting values ($VAR_NAME) when displaying current values in settings forms, and when saving their settings values to the project config / database.

That said, we probably could come up with a way for components to support environmental settings with less code, and provide an API that’s easier for other components to work with.

[khalwat]

Potentially a good use for scenarios + an interface?

When you want the env-parsed values from a model, you could just do something like:

$model->setScenario('parseEnv');

...and a little magic under the hood could take care of the rest.

This would probably be the easiest/least invasive, as everything would continue to work as is does now... you just get this added scenario that does the parseEnv magic under the hood if you choose to use it.

So normally you'd do something like:

class MySettingsModel extends Model implements ParseEnv;

$model->secret;

...and it spits out the unparsed value as normal. But if you do:

$model->setScenario('parseEnv');
$model->secret;

Now we get the parsed value without having to change any code, just setting the scenario.

Anyway lots of ways to do it, but this might be one.

[brandonkelly]

Yeah something along those lines, but not using scenarios, which are for choosing which attributes should be mass-settable (via setAttributes()) and get validated.

We already provide the EnvAttributeParserBehavior, which assumes that the properties will be set to the raw $VAR_NAME values, and replaces them with the parsed values during validation (so validation occurs on the parsed values). I could see us tweaking that so it stores the raw values somewhere accessible for setting templates, and swaps out parsed values with raw values for getSettings() (which is how components communicate their savable setting values for project config), but keep the parsed values in place for normal usage and validation.

[putyourlightson]

^ That sounds ideal, so something like a new getRawSettings() method for use in settings templates? Or perhaps the opposite approach of a new getParsedSettings() method, to avoid introducing a potentially breaking change, although I tend to prefer the former.

[khalwat]

I realize scenarios are for returning different fields, but you can check a model's scenario, and do different things based on it whenever you like... but yeah maybe stretching it beyond the intended purpose, you're right.

@brandonkelly I think you do something kinda sorta similar for Asset sources ya? Where you pass in a context like:

Asset::sources('settings')

^ where 'settings' is the context. Maybe something similar here, to keep the same patterns?

https://github.com/craftcms/cms/blob/develop/src/fields/Assets.php#L158

[sjelfull]

Don't know what the Yii-specific way of doing this is, but I would like for it to happen automatically and in a seamless fashion.

The way it is now, most plugin devs (including Commerce) forgets or haven't added parsing across the board.

Maybe something like a property similar to datetimeAttributes() could work.

I suggest: a property or method that can be either bool, to toggle parsing of all properties, or a array of properties to parse

[leevigraham]

👀

[mediabeastnz]

👀

[tudorradubarbu]

👀

[brandonkelly]

Just established a new convention in craft\models\Site for Craft 3.6, which resolves this.

• Removed the public $baseUrl property (so $site->baseUrl will now end up calling getBaseUrl())
• Added a $parse arg to getBaseUrl(), which is true by default, but can be set to false to access the unparsed base URL when needed (e.g. from the Edit Site page template)
• Added setBaseUrl() setter method
• EnvAttributeParserBehavior::$attributes can now be set as key/value pairs, where the value is a callable that returns the raw value, if the unparsed value can’t be obtained from the attribute directly (e.g. $site->baseUrl).

Here’s a simplified example of what that looks like, which you can use as a template when adding environment variable support to other things:

class Site extends Model
{
    private $_baseUrl;

    public function getBaseUrl(bool $parse = true): ?string
    {
        return $parse ? Craft::parseEnv($this->_baseUrl): $this->_baseUrl;
    }

    public function setBaseUrl(?string $baseUrl): void
    {
        $this->_baseUrl = $baseUrl;
    }

    public function behaviors()
    {
        $behaviors = parent::behaviors();
        $behaviors['parser'] = [
            'class' => EnvAttributeParserBehavior::class,
            'attributes' => [
                'baseUrl' => function() {
                    return $this->_baseUrl;
                },
            ],
        ];
        return $behaviors;
    }

    public function getConfig(): array
    {
        return [
            'baseUrl' => $this->_baseUrl,
            // ...
        ];
    }
}

(Note that your plugin will need to require Craft 3.6+ if you’re going to take advantage of PHP 7.1/7.2 syntax and the EnvAttributeParserBehavior::$attributes change.)

[brandonkelly]

Craft 3.6 RC2 is out now with this change.

[brandonkelly]

Craft 3.6 has now been officially released ✨