Lock configuration

[therouv]

Lock configuration options in system configuration when settings are maintained via this module.

[peterjaap]

@therouv I was actually planning on building this, so could you re-open this issue?

I think we should create an after plugin for this method; \Magento\Config\Model\Config\Reader\Source\Deployed\SettingChecker::isReadOnly.

If the result of that method is false, we should check whether this config value is present in any of the config files for this module. If that is the case, it should return true.

This would probably involve extracting this logic into a separate class for reusability in both the Processors and this plugin.

[therouv]

@peterjaap Alright, issue reopened 😄

I was wondering, though, if locking these values makes sense for every config option. It definitely makes sense for critical configuration options. But for non-critical configuration options, merchants/agencies/developers might want to quickly change them in the admin UI and deploy the new value via the next deployment (in their release cycle).

So, maybe we should create a configuration option with which merchants/agencies/developers can decide if (and/or which, e.g. by path) config options they want to make "read-only"?

[peterjaap]

@therouv yeah good idea, let's make this opt-in. What would be a way to configure this? A separate file? Or some notation in the already existing yaml, like append :locked to the config value or config key?

[therouv]

@peterjaap I'm not a big fan of appending something to the config paths. But since we support JSON+YAML, something like this – a specific child node – would also work / make sense:

YAML:

path/to/config:
    locked: true # (or 0/1)
    default:
        0: 1
    websites:
        base: 0
        test: 0

JSON:

{
  "path/to/config": {
    "locked": true,
    "default": {
      "0": true
    },
    "websites": {
      "base": false,
      "test": false
    }
  }
}

What do you think?

Also, regarding naming: locked or would readonly make more sense?

[peterjaap]

Yeah I like that approach!

Although Magento calls it readonly in their codebase, the flag to acutally lock a config value is --lock, see here. So I'd vote for locked.

[therouv]

Sounds good!

[wilfriedwolf]

Great discussion, just checking out the module and the @therouv approach sounds great to me.
Nevertheless I do not get the answer from @peterjaap

I think we should create an after plugin for this method; \Magento\Config\Model\Config\Reader\Source\Deployed\SettingChecker::isReadOnly.

If the result of that method is false, we should check whether this config value is present in any of the config files for this module. If that is the case, it should return true.

Wouldn't that be skipping the import if the setting is present in the env.php or config.php, whereas the rest of the discussion would rather yield "locked" backend fields to prevent "naive" users to wreck things in changing crucial things?
This could be helpful to warn you that trying to save a config value will not change anything, since the value is locked, but else?
Or maybe I just misunderstand something?

By looking through the code i.m.h.o. the easiest way to realize the locking in env.php (if desired) would be to check if the "locked" option is set for a config and then just call \Magento\Config\Console\Command\ConfigSet\LockProcessor::process afterwards with the appropriate values?

[peterjaap]

@wilfriedwolf I think you wouldn't run into that problem if you only define the plugin for the adminhtml scope. Then the CLI would still lock it for env.php / config.php.