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

Allow plugin configs to override settings more automagically #1096

Closed
angrybrad opened this issue Jan 31, 2017 · 15 comments
Closed

Allow plugin configs to override settings more automagically #1096

angrybrad opened this issue Jan 31, 2017 · 15 comments

Comments

@angrybrad
Copy link
Member

Created by: Timothy Kelty (timkelty@gmail.com) on 2015/04/02 12:33:05 +0000
Votes at time of UserVoice import: 3


It seems a common pattern to define all plugin configuration as "settings" (in the db, configurable though cp), but have the all be overridable via a plugin config file.

It's easy enough to do, but I think it would be nice if this was either: the default (my preference), or just easier to flip on.

Currently I do something like:

public function getSettings()
{
  $settings = parent::getSettings();
  foreach ($settings as $name => $value) {
      $configValue = craft()->config->get($name, 'pluginHandle');
      $settings->$name = is_null($configValue) ? $value : $configValue;
  }

  return $settings;
}

But it would be great if I could just set:

    public function allowSettingsConfigOverrides()
    {
        return true;
    }

Or instead of true you could even set an array of keys to allow overriding for.

@timkelty
Copy link
Contributor

timkelty commented Mar 17, 2017

I'd argue that this should be the default behavior with plugin settings.

It would also be nice if there were a way to defineSettings directly in your config.php, as boilerplate code like this gets tedious:

    {
        return [
            'disqusShortname' => [AttributeType::String, 'label' => 'Disqus Site Short Name', 'default' => craft()->config->get('disqusShortname', 'disqus')],
            'useSSO' => [AttributeType::Bool, 'label' => 'Use Single Sign On', 'default' => craft()->config->get('useSSO', 'disqus')],
            'disqusPublicKey' => [AttributeType::String, 'label' => 'Disqus Public Key', 'default' => craft()->config->get('disqusPublicKey', 'disqus')],
            'disqusSecretKey' => [AttributeType::String, 'label' => 'Disqus Secret Key', 'default' => craft()->config->get('disqusSecretKey', 'disqus')],
            'customLogin' => [AttributeType::Bool, 'label' => 'Use Custom Login/Logout URLs', 'default' => craft()->config->get('customLogin', 'disqus')],
            'loginName' => [AttributeType::String, 'label' => 'name', 'default' => craft()->config->get('loginName', 'disqus')],
            'loginButton' => [AttributeType::String, 'label' => 'button', 'default' => craft()->config->get('loginButton', 'disqus')],
            'loginIcon' => [AttributeType::String, 'label' => 'icon', 'default' => craft()->config->get('loginIcon', 'disqus')],
            'loginUrl' => [AttributeType::String, 'label' => 'url', 'default' => craft()->config->get('loginUrl', 'disqus')],
            'loginLogoutUrl' => [AttributeType::String, 'label' => 'logout', 'default' => craft()->config->get('loginLogoutUrl', 'disqus')],
            'loginWidth' => [AttributeType::String, 'label' => 'width', 'default' => craft()->config->get('loginWidth', 'disqus')],
            'loginHeight' => [AttributeType::String, 'label' => 'height', 'default' => craft()->config->get('loginHeight', 'disqus')],
        ];
    }

You can clean this up a bit, but it's still a lot of boilerplate that would be a nice default:

    protected function defineSettings()
    {
        $settings = [
            'disqusShortname' => [AttributeType::String, 'label' => 'Disqus Site Short Name'],
            'useSSO' => [AttributeType::Bool, 'label' => 'Use Single Sign On'],
            'disqusPublicKey' => [AttributeType::String, 'label' => 'Disqus Public Key'],
            'disqusSecretKey' => [AttributeType::String, 'label' => 'Disqus Secret Key'],
            'customLogin' => [AttributeType::Bool, 'label' => 'Use Custom Login/Logout URLs'],
            'loginName' => [AttributeType::String, 'label' => 'name'],
            'loginButton' => [AttributeType::String, 'label' => 'button'],
            'loginIcon' => [AttributeType::String, 'label' => 'icon'],
            'loginUrl' => [AttributeType::String, 'label' => 'url'],
            'loginLogoutUrl' => [AttributeType::String, 'label' => 'logout'],
            'loginWidth' => [AttributeType::String, 'label' => 'width'],
            'loginHeight' => [AttributeType::String, 'label' => 'height'],
        ];

        $settings = array_map(function($key, $value) {
            $value['default'] = craft()->config->get($key, 'disqus');
            
            return $value;
        }, array_keys($settings), $settings);

        return $settings
    }

@khalwat
Copy link
Contributor

khalwat commented Mar 17, 2017

This would be really nice, so that we could override anything on a per-environment basis. If this was baked into the Craft 3 craft\base\Plugin->getSettings() then all plugins could be overridden by default.

@sjelfull
Copy link
Contributor

I'm a big fan of this.

@timkelty
Copy link
Contributor

I think the biggest win here is predictability for plugin users - knowing they can always set any setting via config, regardless of how the plugin is authored.

@khalwat
Copy link
Contributor

khalwat commented Mar 17, 2017

Even more useful would be if it could tell you that the setting was being overridden by the config file, the way Craft does This would probably need to be on the Settings model however

@khalwat
Copy link
Contributor

khalwat commented Mar 17, 2017

Some kind of Craft provided a way to bottleneck and standardize all of this would be really helpful.

Plugin developers wouldn't have to reinvent the wheel, and all plug-ins would operate in an understood and predictable manner

@timkelty
Copy link
Contributor

Even more useful would be if it could tell you that the setting was being overridden by the config file, the way Craft does This would probably need to be on the Settings model however

Agreed, this lets plugin authors display that settings are being overridden when displaying the CP field. Currently authors seem to implement this themselves for every plugin.

@engram-design
Copy link
Contributor

+1!

@brandonkelly
Copy link
Member

Slipped this in over the weekend, along with lots of other changes to the Config service. Going forward, all plugins that want to have settings will need to create a settings model and instantiate it with a protected createSettingsModel() method, and that model will be populated with whatever is in the DB (if the plugin has CP settings), merged with whatever is in config/pluginhandle.php (if it exists).

This is a breaking change for plugins that were previously providing default values in config.php and calling the Config service to get their PHP file-based settings. That config.php file can now be deleted (move the default values into the settings model), and calls to Craft::$app->config->get('settingName', 'pluginhandle') should be replaced with MyPlugin::getInstance()->settings->settingName.

Here’s an example of how our Element API plugin was affected by the changes, if that helps: craftcms/element-api@dd65ee4

@khalwat
Copy link
Contributor

khalwat commented Mar 20, 2017

I assume that a pluginhandle.php file in the craft/config folder is still how to allow people to override things in a multi-environment friendly manner?

@timkelty
Copy link
Contributor

@khalwat, yeah there's just no need to set your defaults in the pluginhandle/config.php any longer.

@khalwat
Copy link
Contributor

khalwat commented Mar 20, 2017

Geez, @brandonkelly said it right there in his original comment, too. I don't know how I missed that.

@timkelty I'll probably still include the config.php file for people to rename & move into craft/config, so they know what settings they can change.

The one thing this doesn't solve is a plugin being able to report when a setting is overriding something in the db via a pluginhandle.php file.

@brandonkelly
Copy link
Member

@khalwat Craft doesn’t define the HTML for the CP plugin settings; plugins do. So it would be up to the plugin to add those warnings if they wanted.

Plugins can find which keys the config is defining like so:

$overrideKeys = array_keys(Craft::$app->config->getConfigFromFile('pluginhandle'));

or it might make more sense to get it directly from your settings template:

{% set overrideKeys = craft.app.config.getConfigFromFile('pluginhandle')|keys %}

@khalwat
Copy link
Contributor

khalwat commented Mar 20, 2017

Yeah the macro I've been using in the past is:

{% macro configWarning(setting) -%}
    {% if (craft.config.get(setting, 'disqus')) |length %}
        {{ "This is being overridden by the {setting} config setting in your disqus.php config file." |t({
            setting: setting
        }) |raw }}
    {% endif %}
{%- endmacro %}
{% from _self import configWarning %}

I'll figure out how to adapt it once it rolls out in beta 8

@brandonkelly
Copy link
Member

One caveat to be aware of though – if a setting is defined in the config file, the file’s value is going to override the DB-stored value, so your plugin (including its settings page) will never know what the actual DB-stored value is.. This is also the case when editing volume settings in the CP, if you override them in config/volumes.php. The CP settings will show the PHP file’s values, and if you save them, the PHP file’s values will end up in the DB as well.

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

No branches or pull requests

6 participants