Problem/Motivation

Following discussion s about #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects with @xjm, @GaborHojtsy, @dawehner, @bircher, @ademarco, and @berdir,@fago, we need to ensure that config overrides (a) don't bleed into configuration when you save it (b) config overrides are used when they should be. This problem has highlighted that it is very easy for config overrides to bleed into configuration.

For example:

// Override name in settings.php
$config['system.site']['name'] = 'My Drupal site';

// Do something along the lines of:
$name = \Drupal::config('system.site')->get('name');
// Put the name in a form or something...
\Drupal::config('system.site')->set('name', $name)->save();

This is bad because:

  • The code has no control over what is overridden - anything can be overridden in settings.php
  • Potentially security sensitive information could end up unintentionally in configuration

Proposed resolution

  • By default return immutable configuration objects from the config factory.
  • Allow the developer to get mutable configuration objects from the config factory. Mutable objects should not have overrides.
  • Use mutable object where necessary.
  • Provide a trait so that forms can easy get mutable configuration objects - but also use runtime config with overrides.
  • Config entities when loaded as entities (e.g., via the class's load() method) remain mutable. HEAD already has other code in place to make those override free in config entity forms and listings. From outside those forms and listings, it remains possible to get a mutable, with-overrides config entity and write code that results in override bleed, but this is a less severe problem, since overriding config entity values via settings.php is an extreme edge case: it wasn't even possible to do in D7 at all.

Remaining tasks

  • Agree approach
  • Write patch
  • Review
  • Commit

User interface changes

None

API changes

An incomplete list:

  • ConfigFactory::rename() returns the configuration factory and not a config object.
  • ConfigFactory::get(), ConfigFactory::loadMultiple(), \Drupal::config(), FormBase::config() return a configuration object that can not be saved or changed.
  • ConfigFactory::getEditable() introduced to get editable configuration objects
  • New trait ConfigFormBaseTrait which provides a config() that can return mutable config objects for named configuration. The configuration names are provided by implementations of ConfigFormBaseTrait::getEditableConfigNames

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Critical because this is a blocker for providing a complete solution for #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects. This also has security implications. For example, if you override an API key in production there should be no danger of this being saved back to config.
Disruption Disruptive for core/contributed and custom modules/themes because it will require a BC break. The \Drupal::config(), FormBase::config() will no longer return saveable objects. However the disruption is worth it because of the increased security, reduced fragility of overriding configuration and predictability of the configuration system.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

alexpott’s picture

Status: Active » Needs review
FileSize
19.5 KB

First patch that makes configuration object returned from the factory by default immutable and ensure that minimal and standard still install.

A couple of interesting things are that StaticMenuLinkOverrides and SiteConfigureForm currently would bleed global overrides into the active config. I'm not convinced by my changes to StaticMenuLinkOverrides.

Sending to testbot to see how much breaks.

effulgentsia’s picture

The proposal makes sense to me.

Allow the developer to get mutable configuration objects from the config factory. Mutable objects should not have overrides.

Didn't CMI at one time have a $override_free variable/concept? I wonder if that (or $with_overrides) would be a better parameter name, because for example, an override-free object should be used for setting #default_value as well, even though doing so does not require mutability (directly). That's a minor point though that could also be discussed/changed when the patch is further along.

alexpott’s picture

re #6 - I did not force mutable config objects to be override free yet - I'm not sure about that.

Status: Needs review » Needs work

The last submitted patch, 5: 2392319.5.patch, failed testing.

ademarco’s picture

@alexpott: would it make sense to have:

\Drupal::config('core.extension', FALSE);

wrapped by a more descriptive

\Drupal::mutableConfig('core.extension');.

We can adopt a similar approach in other places too, in this way we kind of state that configuration is immutable when accessed directly via \Drupal::config which is kind of what we want, is that correct?

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.64 KB
23.88 KB

Tests should now at least run.

alexpott’s picture

FileSize
12.43 KB

So the approach in #5 and #10 is giving me nightmares about the amount that is going to have to change and whether or not it is really correct. The main issue here is that FormBase::config returns objects that can be changed. The patch attached to this comment swaps the logic around so the FormBase::config returns an immutable object but \Drupal::config() still returns a mutable object. It is still worth checking where #10 fails because it might surface other classes like StaticMenuLinkOverrides where overrides can potentially bleed into active configuration.

I have not supplied an interdiff because this patch is an invert of the logic in #10 so that would be largely irrelevant.

dawehner’s picture

Issue tags: +Ghent DA sprint

.

The last submitted patch, 10: 2392319.10.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: 2392319-new.11.patch, failed testing.

Gábor Hojtsy’s picture

Reviewed #11. Outside of code style (lack of docs, typos) I did not find any thing to complain about. The question as always is how far are we going to need to go to implement this approach.

alexpott’s picture

Status: Needs work » Needs review
FileSize
25.27 KB
15.65 KB

So StaticMenuLinkOverrides::saveOverride() is broken in HEAD because it cannot merge definitions as it says it can because the ID is double encoded. The patch attached makes this code simpler and tests it. All the other test fails should be fixed too.

Gábor Hojtsy’s picture

  1. +++ b/core/modules/contact/src/ContactFormEditForm.php
    @@ -17,13 +19,39 @@
     class ContactFormEditForm extends EntityForm {
    

    Why is this not based on ConfigEntityFormBase? Sounds like we do a lot of custom things here that would be given there.

  2. +++ b/core/modules/system/src/Form/ThemeAdminForm.php
    @@ -6,13 +6,13 @@
    -class ThemeAdminForm extends FormBase {
    +class ThemeAdminForm extends ConfigFormBase {
    

    Wow, good find.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record
  1. +++ b/core/lib/Drupal.php
    @@ -297,12 +297,14 @@ public static function lock() {
    +   *   (optional) Create an mutable configuration object. Defaults to TRUE.
    

    s/an/a/

  2. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -260,12 +260,14 @@ public function getCacheKeys() {
    +   *   Whether or not the object is mutable.
    

    s/object/configuration object/

  3. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -260,12 +260,14 @@ public function getCacheKeys() {
    +    return $name . ':' . implode(':', $this->getCacheKeys()) . ':' . ($mutable ? static::MUTABLE: static::IMMUTABLE);
    

    s/MUTABLE:/MUTABLE :/

  4. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -333,4 +335,22 @@ public function addOverride(ConfigFactoryOverrideInterface $config_factory_overr
    +   *   Determines whether a mutable or immutable config object is returned.
    

    s/config/configuration/ (if it fits in 80 cols)

  5. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryInterface.php
    @@ -15,6 +15,16 @@
    +   * Constant used in static cache keys for mutable config objects.
    ...
    +   * Constant used in static cache keys for immutable config objects.
    

    s/config/configuration/

  6. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryInterface.php
    @@ -37,11 +47,13 @@ public function getOverrideState();
    +   *   (optional) Create an mutable configuration object. Defaults to TRUE.
    
    @@ -51,11 +63,13 @@ public function get($name);
    +   *   (optional) Create an mutable configuration object. Defaults to TRUE.
    
    @@ -75,11 +89,13 @@ public function reset($name = NULL);
    +   *   (optional) Create an mutable configuration object. Defaults to TRUE.
    

    s/an/a/

  7. +++ b/core/lib/Drupal/Core/Config/ImmutableConfig.php
    @@ -0,0 +1,42 @@
    +class ImmutableConfig extends Config {
    

    Missing docs.

  8. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
    @@ -80,7 +80,9 @@ public function loadOverride($id) {
       public function deleteMultipleOverrides(array $ids) {
    -    $all_overrides = $this->getConfig()->get('definitions');
    +    // Get the original configuration without overrides to ensure that global
    +    // overrides are saved to configuration.
    

    This is a "delete overrides" method, yet the docs indicate "saving". Is this correct?

  9. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
    @@ -134,11 +136,15 @@ public function saveOverride($id, array $definition) {
    +      // Get the original configuration without overrides to ensure that global
    +      // overrides are saved to configuration.
    

    So… we get the configuration WITHOUT overrides to ensure that GLOBAL OVERRIDES are saved?
    That is very confusing.

  10. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
    @@ -265,4 +265,20 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +   * returned override free. This ensures that overrides do not pollute saved
    

    s/override free/override-free/ ?

alexpott’s picture

So #2393073: Helper issue has uncovered everything that we'd need to change if we made the config factory return immutable objects by default (this would include \Drupal::config() too). @Gábor Hojtsy has created #2395395: TestBase lacks a config method to be used consistently in tests.

Other issues identified:

If we decide that we what to make config by default immutable then the three additional issues listed in this comment should be critical as they block the implementation.

One interesting thing discovered whilst implementing is how we deal with form alters. For example, contact_form_user_admin_settings_alter() / contact_form_user_admin_settings_submit(). These functions handle adding a checkbox to the user admin settings page to enable the personal contact form for users by default. The patch in the helper method suggests changing:

  $form['contact']['contact_default_status'] = array(
    '#type' => 'checkbox',
    '#title' => t('Enable the personal contact form by default for new users'),
    '#description' => t('Changing this setting will not affect existing users.'),
    '#default_value' => \Drupal::config('contact.settings')->get('user_default_enabled'),
  );

to

  $form['contact']['contact_default_status'] = array(
    '#type' => 'checkbox',
    '#title' => t('Enable the personal contact form by default for new users'),
    '#description' => t('Changing this setting will not affect existing users.'),
    '#default_value' => $form_state->config('contact.settings')->get('user_default_enabled'),
  );

FormState::config() would delegate to the FormBase or ConfigFormBase's config() method - thereby ensuring that the correct override state / mutable or immutable object would be returned.

I think we need to somehow help people altering forms to do the right thing.

Gábor Hojtsy’s picture

I don't think we can box people into something in an alter hook. We can provide good examples that people will copy-paste anyway. Or make the whole config API abide the same rules of [im]mutability, etc.

alexpott’s picture

@Gábor Hojtsy - I'm not talking about boxing people in - just providing a recommended way to access configuration in the same way as the form object.

alexpott’s picture

discussed with @effulgentsia and @cilefen on the CMI call. We got a proposal that I'm going to write up and add here when I've got a moment.

alexpott’s picture

Notes from the meeting alluded to in #21.

We discussed the following places where configuration objects are retrieved.

ConfigFactory::get() \Drupal::config() FormBase::config() ConfigFormBase::config()
Override free No not by default No No Yes
Mutable No not by default No No Yes

Looking at the above table we think that implementing a new trait ConfigEdittableTrait that provides a config() function that returns a mutable, override free config object. Similar to the current ConfigFormBase::config(). However in order to get it to return mutable, override free config the class that implements the trait will need to implement a method or property that returns a list of all config that should be editable. The advantages of this are:

  • when a developer implements ConfigFormBase they will have to think about which config objects they want to be able to save
  • All other configuration access through the trait will have overrides applied thereby making it easy to use config reliably for runtime needs like sending an email in a form submit
  • If you need both overridden and editable config then the form will need to access the config factory directly
  • Using a trait means that other forms, services, controllers that needs this functionality can easily implement it
catch’s picture

I see how this helps people defining the actual forms.

What does this change for the contact form alter example though?

daffie’s picture

Issue tags: +Needs tests
alexpott’s picture

@catch you're right - the form alter needs situation needs thinking about / discussing. With the solution outlined in #23 this is will make it hard for them to do the right thing unless they can also easily a mutable override object of their choice.

effulgentsia’s picture

Initially my thought with form alter would be that the trait proposed in #23 would define its config() method and its addEditableConfig() method as public, so that following the example of #19, a form alter could do something like $form_state->getFormObject()->addEditableConfig('contact.settings'), at which point its later calls to $form_state->config() would return a mutable config. However, that's flawed, because it creates a WTF dependency between different modules' alters. For example, if modules A and B both alter a form, and A does so by adding a submit handler that wants to send an email to the appropriate overridden value while B wants to provide a textfield for editing the non-overridden value, that approach would clash, because simply the act of enabling module B ends up changing who receives the email sent by module A. In summary, #23's proposal of having the config() method choose whether to return an overridden or non-overridden object based on the value of a stateful property is problematic if multiple modules can act on that state.

So, rethinking this problem space a bit, here's my suggestion:

  1. Add an optional parameter ($editable) to ConfigFactory::get() that defaults to FALSE. Thereby any code has the ability to interact directly with the config factory (rather than one of the config() wrapper methods) in order to get an editable (override free) config.
  2. Leave the \Drupal::config() wrapper as only returning an immutable, with-overrides, config (i.e., invoke the factory's get() without the 2nd param). Outside of forms, which I'll cover below, procedural code needing an editable config should be rare, and in such cases, it can call Drupal::configFactory()->get() and pass the 2nd param.
  3. Same as above for the ControllerBase::config() wrapper, because same situation of rarity of needing editable config within non-form controllers.
  4. For forms (both FormBase and ConfigFormBase), a config() wrapper is frankly too ambiguous. You're in a form, and in both form building and in form submitting, sometimes you might want a with-overrides config and sometimes you might want an editable config. So I think the presence of an ambiguous config() wrapper method hurts more than it helps. So, I propose to remove it and instead have runtimeConfig() and editableConfig() wrapper methods to force explicit choice. This will break every already ported contrib module that has a form that interacts with config, which sucks, but I think forcing clarity around data integrity is worth it.
  5. Inspired by #19, I suggest we also add a FormState::config() wrapper. Within the context of $form_state, there is 0 ambiguity that what you want is editable config. That will allow the more common case of forms wanting editable config to call $form_state->config() rather than $this->editableConfig(), which I think is nicer DX. Additionally, it will allow form alters to also use $form_state->config(), thereby having consistent syntax between the usages within the form class and within the alters. Accessing with-override config would be different though: in the form class, it would be done as $this->runtimeConfig() and in the procedural alter implementations it would be done as \Drupal::config(). But I think that's ok, because if what you're accessing is runtime config, then even though you're in the context of a form, with respect to that config, you're not dealing with its "forminess", so there it's more important for the procedural code to be consistent with other non-form procedural code, and for the code within the form class to be unambiguous.

Thoughts?

alexpott’s picture

What was nice about the idea suggested in #23 is that developer declares there intent to edit a specific config file - and then we can use the same function to get any config file. What was really nice is that it guaranteed that both the ConfigFormBase::build() and ConfigFormBase::submit() would use the same config object for a specific config file. With the solution in #27 it possible to use runtimeConfig() in build() and editableConfig() in the submit().

That said, I'm okay with most of the solution in proposed in #27. I'm not sure about having FormBase::editableConfig(). I still think we should have editableConfig() as a trait and FormBase forms that need this should implement on an individual basis since it is a special case. I like renaming FormBase::config() to FormBase::runtimeConfig() since this will alert developers if they are building a form that edits config they need to use ConfigFormBase.

effulgentsia’s picture

What was really nice is [ #23 ] guaranteed that both the ConfigFormBase::build() and ConfigFormBase::submit() would use the same config object for a specific config file.

True. That's pretty important.

#23's proposal of having the config() method choose whether to return an overridden or non-overridden object based on the value of a stateful property is problematic if multiple modules can act on that state.

Discussed this problem with @alexpott, and here's what we came up with. The following is a complete proposal that fully supersedes #23 and #27. The last item is the one that addresses the form alter problem.

  1. Change ConfigFactory::get() to return an immutable object. Thereby, all existing config() wrapper methods (e.g., Drupal::config(), ControllerBase::config(), FormBase::config()) also return immutable objects by default.
  2. Add an extra method ConfigFactory::getEditable() so that all code has the ability to interact directly with the config factory (rather than one of the config() wrapper methods) in order to get an editable (override free) config.
  3. Add a ConfigEdittableTrait trait with a protected config() implementation and a protected $editableConfigs property. Make ConfigFormBase use this trait. Other classes can use this trait too. For any class that uses this trait, it can set $editableConfigs to the names of the config objects it wants to edit. config() will then return a mutable, override-free object for those names only, and fall back to the immutable, with-overrides object for other names. Note that because config() and $editableConfigs are protected, this is a local decision, so is free of the problem described at the beginning of #27. Per #28, this also ensures consistency between the form build and form submit: i.e., if a mutable object is used in the submit handler, then you also get an override-free object when setting #default_value in the build() method.
  4. For form alters that need to deal with editable config, we decide that the best practice is to turn that alter into an object. For example, contact_form_user_admin_settings_alter() can become the following:
    function contact_form_user_admin_settings_alter(&$form, FormStateInterface $form_state) {
      $alteration = UserAdminSettingsFormAlter::create(\Drupal::getContainer());
      $alteration->alterForm($form, $form_state);
      
      // @todo Put these in the procedural wrapper, as shown, or move them
      //   into alterForm()?
      $form['#validate'][] = array($alteration, 'validateForm');
      $form['#submit'][] = array($alteration, 'submitForm');
    }
    

    By doing this, the alteration can use the ConfigEdittableTrait trait, and specify the configs that it's responsible for editing, and benefit from the same consistency of alterForm() and submitForm() using the same override state for any given config. And again, this is a local decision, meaning two different alter classes remain independent with respect to what $this->config() returns for each.

Gábor Hojtsy’s picture

#2395395: TestBase lacks a config method to be used consistently in tests landed, so now all issues that postponed this one are done :)

alexpott’s picture

Status: Needs work » Needs review
FileSize
95.04 KB

Work done on the patch. Posting to get a test run and so if anyone else wants to take this on they can...

Still to add the ability to define which configuration is editable on the trait.

Status: Needs review » Needs work

The last submitted patch, 31: 2392319.31.patch, failed testing.

Status: Needs work » Needs review

alexpott queued 31: 2392319.31.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 31: 2392319.31.patch, failed testing.

The last submitted patch, 31: 2392319.31.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.45 KB
98.69 KB

This should fix the test failures - still need to implement the ability to define which configuration is editable on the trait.

alexpott’s picture

FileSize
43.83 KB
131.07 KB

The patch attached implements ability to define which configuration is editable on the trait and changes form alters to work in the way suggested in #29. I've implemented \Drupal::resolve() so we don't get \Drupal::getContainer proliferating everywhere.

Status: Needs review » Needs work

The last submitted patch, 37: 2392319.37.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
22.09 KB
144.98 KB

Fixed all the form alters are created #2401035: items_per_page in node.settings is no longer used to remove one.

alexpott’s picture

FileSize
144.53 KB

Rerolled now that #2401035: items_per_page in node.settings is no longer used landed.

There are a couple of @todo's in the patch - the most awful of which is:

+    // @todo this is pretty meaningless since we're using theme_get_settings
+    //   which means overrides can bleed into active config here.
+    $this->editableConfig = [$config_key];

The current theme settings form will allow overrides to bleed into configuration. And the work this patch does not protect against it. Considering this patch does not make the situation worse I think we could file a follow up for that?

Gábor Hojtsy’s picture

Started reviewing the interdiffs, this one is for #37. Sorry if some may be fixed since then.

  1. +++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
    @@ -41,9 +40,22 @@ public function config($name) {
    +   * @return array
    +   *   An array of configuration object names that the config
    

    Not finished

  2. +++ b/core/lib/Drupal/Core/Form/FormAlterInterface.php
    @@ -0,0 +1,18 @@
    +interface FormAlterInterface {
    +
    +  public function alter(array &$form, FormStateInterface $form_state);
    +
    +  public function validate(array &$form, FormStateInterface $form_state);
    +
    +  public function submit(array &$form, FormStateInterface $form_state);
    +}
    \ No newline at end of file
    

    Lacks docs, no newline.

  3. +++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
    @@ -100,6 +100,17 @@ public function getFormId() {
    +  protected function getEditableConfigNames() {
    +    return [
    +      'system.date',
    +      'system.site',
    +      'update.settings',
    +    ];
    +  }
    

    This is somewhat duplicated in system.config_translation.yml but I don't see a good way to match the route <-> config list to the class <-> config list mappings that are here. Also this points out that we have some incorrect mappings there.

  4. +++ b/core/modules/contact/contact.module
    @@ -147,33 +147,10 @@ function contact_mail($key, &$message, $params) {
     function contact_form_user_form_alter(&$form, FormStateInterface $form_state) {
    ...
    +  /** @var \Drupal\contact\Form\UserFormAlter $alter */
    +  $alter = \Drupal::resolve('Drupal\contact\Form\UserFormAlter');
    +  $alter->alter($form, $form_state);
    +  $form['actions']['submit']['#submit'][] = array($alter, 'submit');
     }
    

    Wow, that is a pretty huge API (at least best practice) change there...

  5. +++ b/core/modules/contact/src/Form/UserFormAlter.php
    @@ -0,0 +1,91 @@
    + * Adds the enable personal contact form to an individual user's account page.
    

    the checkbox / setting, etc. a word is missing from this sentence.

  6. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
    @@ -87,6 +101,9 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
    +    // @todo this is pretty meaningless since we're using theme_get_settings
    +    //   which means overrides can bleed into active config here.
    +    $this->editableConfig = [$config_key];
    

    I agree we can open a followup and need to update patch to refer to that issue number.

Gábor Hojtsy’s picture

Looking at #39, the string translation trait changes seem cosmetic, but then I realized those are in "new" alter class code, so they would show up as new code in the patch anyway. So fine...

I think the main question based on the recent changes is whether we want to change form altering so much as introduced here.

tim.plunkett’s picture

One thing I haven't grokked is what will happen if you try to use old-style form alters. Will it save things wrong? Will it silently ignore changes? Will it blow up?

alexpott’s picture

@tim.plunkett if you try and save config using \Drupal::config() it just won't work - you'll get an exception on the set(). Of course you could use \Drupal::configFactory()->getEditable() to get a mutable config object.

effulgentsia’s picture

I think the main question based on the recent changes is whether we want to change form altering so much as introduced here.

(See also #29.4 and #41.4)

This doesn't change every form alter: only ones that do their own direct manipulation of simple config (rather than via a config entity or plugin). Core has ~40 form alters, and this change is only needed for 5 of them. (If system_logging_settings implemented a plugin API that dblog and syslog could use, then that would go down to 3).

I agree that the change is significant for those cases, but I don't have any better ideas for how to give form alters an API for getting mutable config that ensures consistency between the build/alter and the submit: ensuring that consistency requires placing those two methods into the same object, I think.

effulgentsia’s picture

Of course you could use \Drupal::configFactory()->getEditable() to get a mutable config object.

Right. The risk is that you do that in the submit callback, because you have to to avoid the exception on set(), but then you keep using \Drupal::config() for the #default_value in the alter hook itself, because you forget to change that and there's no exception thrown to remind you. Then you get the settings.php leakage into active config that this issue is trying to solve. Which the trait solves, but requires an object to be attached to.

dawehner’s picture

Just a quick drive by review.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -71,6 +71,13 @@ class ConfigFactory implements ConfigFactoryInterface, EventSubscriberInterface
    +   * A list of configuration objects that make no sense to override.
    +   *
    +   * @var array
    +   */
    +  protected $unoverridable = ['core.extension'];
    +
    

    Note: chx planned at some point to override this configuration for mongodb.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -104,17 +111,45 @@ public function getOverrideState() {
    +  protected function _get($name, $immutable = TRUE) {
    

    maybe just doGet or something like that?

  3. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryInterface.php
    @@ -75,11 +103,13 @@ public function reset($name = NULL);
    -  public function rename($old_name, $new_name);
    +  public function rename($old_name, $new_name, $immutable = TRUE);
    

    It is a little bit confusing that you can rename a immutable config.

  4. +++ b/core/lib/Drupal/Core/Config/ImmutableConfigException.php
    @@ -0,0 +1,16 @@
    +/**
    + * Exception throw when an immutable config object is altered.
    + *
    + * @see \Drupal\Core\Config\ImmutableConfig
    + */
    +class ImmutableConfigException extends \RuntimeException {
    +}
    

    I'd be great if we somehow document in the exception message what people should do instead.

  5. +++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
    @@ -0,0 +1,61 @@
    +
    +trait ConfigFormBaseTrait {
    

    Let's describe when to use it.

  6. +++ b/core/lib/Drupal/Core/Form/FormAlterInterface.php
    @@ -0,0 +1,53 @@
    + *   $alter = \Drupal::resolve('CLASSNAME');
    + *   $alter->alter($form, $form_state);
    

    I really like that

  7. +++ b/core/modules/config/src/Tests/ConfigEntityTest.php
    @@ -19,7 +19,7 @@
    +class   ConfigEntityTest extends WebTestBase {
    

    more space :P

effulgentsia’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryInterface.php
    @@ -33,17 +43,33 @@ public function setOverrideState($state);
    +  public function getEditable($name, $override_free = TRUE);
    

    Ugh. I hate that we make it possible to get an editable config that has overrides applied. I was hoping this issue would put an end to that. I guess we need to for now because of ConfigEntityStorage::doSave(), but it would be great if we could find a way to remove needing that in a followup. In the meantime, how about removing the parameter, and creating a separate method like getOverriddenAndEditable() that is not in ConfigFactoryInterface, but instead is either not interface-defined at all, or is in some specialized interface with docs telling people to beware of it?

  2. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
    @@ -80,7 +80,7 @@ public function loadOverride($id) {
    -    $all_overrides = $this->getConfig()->get('definitions');
    +    $all_overrides = $this->getConfig()->getOriginal('definitions', FALSE);
    

    Would it make sense to create a child issue that just does the getOriginal() conversions? AFAICT, those make sense regardless of this larger issue, and at least for me, distract in grokking the larger patch.

effulgentsia’s picture

Additionally, what about splitting out FormAlterInterface and the 5 conversions to it (without the new trait though) into a child issue? Given #42, I think that could benefit from its own discussion thread, both the principle of it and details such as method names and whether the procedural hook or the alter() method should be the one that attaches the #validate and #submit. Also, the principle of an alter() and a submit() wanting access to a shared protected member is a valid principle independent of this use case, which I think would make that issue legitimately committable on its own.

yched’s picture

Sorry to add a name nitpick here but since this introduces \Drupal::resolve() :

Reading code like :

$foo = \Drupal::resolve('Some\Class');

it's not really intuitive that "resolving a class" means instantiating an object through a generic factory.
I know the underlying service is called "class resolver" (which never stroke me as obvious either), but not sure internal consistency is worth the unintuitive public API .

alexpott’s picture

@yched happy to have suggestions - not wedded to the name :)

larowlan’s picture

review in progress

  1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -71,6 +71,13 @@ class ConfigFactory implements ConfigFactoryInterface, EventSubscriberInterface
    +  protected $unoverridable = ['core.extension'];
    

    not sure this is even a word? overridable is a stretch, unoverridable might be a bridge too far. How about $cannotOverride instead?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -104,17 +111,45 @@ public function getOverrideState() {
    +  protected function _get($name, $immutable = TRUE) {
    

    Is there a reason for the _get? Our normal pattern is to prefix these kind of things with do - i.e. doGet()

alexpott’s picture

re #49 and #50 I've broken out the form alter changes into #2402445: Implement object oriented form alters since it is hard to have a proper discussion on them with all the immutable config code that surrounds them in this patch.

Going to implement this without the form alter changes.

alexpott’s picture

@larowlan #52.2 expediency - I was thinking just get the method out of the way :)

alexpott’s picture

FileSize
40.19 KB
119.95 KB

Thanks everyone for all the reviews! The patch attached addresses the points below and reverts the form alter changes and does the minimal there so we can discuss the OO-ification on another issue.

#41 review

  1. Fixed
  2. Fixed in #2402445: Implement object oriented form alters
  3. Yeah... hmmm.
  4. Moved debate to #2402445: Implement object oriented form alters
  5. Fixed in #2402445: Implement object oriented form alters though I believe this might have been a c&p
  6. Fixed - created #2402467: Prevent theme settings configuration overrides from bleeding into stored configuration

#47 review

  1. I think permitting overrides of this is just too complex - installing and uninstalling modules has side effects. When they have not occurred we have problems.
  2. Fixed
  3. Fixed - rename no longer returns a configuration object - the current function mixes up a command (rename) with a query (get me the object). That's a code smell.
  4. Fixed
  5. Fixed
  6. Moved the alter debate to #2402445: Implement object oriented form alters - I like it too
  7. Fixed

#48 review

  1. still @todo
  2. I don't think so - this issue is all about resolving when overrides should and should not apply.

#52 review

  1. Fixed
  2. Fixed

Status: Needs review » Needs work

The last submitted patch, 55: 2392319.55.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.95 KB
122.31 KB

Thinking some more about #48.1 we just don't need it - config already prevents overrides from bleeding in on save - so getting the config with overrides in ConfigEntityStorage::doSave() and then calling setData() is kinda pointless.

Hopefully fixed test fails.

Status: Needs review » Needs work

The last submitted patch, 57: 2392319.57.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
122.32 KB

This conflicted with #918538: Decouple cache tags from cache bins but git resolved it...

Status: Needs review » Needs work

The last submitted patch, 59: 2392319.59.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
122.89 KB
3.09 KB

Fixed phpunit fails.

effulgentsia’s picture

Thanks for implementing all that feedback. Still reviewing, but:

I don't think so - this issue is all about resolving when overrides should and should not apply.

Right, but get() -> getOriginal() changes more than just not applying overrides, it also means ignoring unsaved set()s. Which maybe is a no-op in the cases here (especially if only called on immutable config), but whether we have mutable or immutable config at the point that we call getOriginal() isn't immediately obvious.

effulgentsia’s picture

FileSize
122.89 KB

Just a reroll.

effulgentsia’s picture

More thorough review on all of the get() -> getOriginal() conversions (excluding the ones in ConfigOverrideTest):

  1. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
    @@ -80,7 +80,7 @@ public function loadOverride($id) {
    -    $all_overrides = $this->getConfig()->get('definitions');
    +    $all_overrides = $this->getConfig()->getOriginal('definitions', FALSE);
    @@ -135,7 +135,7 @@ public function saveOverride($id, array $definition) {
    -      $all_overrides = $this->getConfig()->get('definitions');
    +      $all_overrides = $this->getConfig()->getOriginal('definitions', FALSE);
    

    Why? $this->getConfig() returns editable and override-free config already.

  2. +++ b/core/modules/book/book.module
    @@ -559,9 +559,9 @@ function book_type_is_allowed($type) {
    -    $allowed_types = $config->get('allowed_types');
    +    $allowed_types = $config->getOriginal('allowed_types', FALSE);
    @@ -573,7 +573,7 @@ function book_node_type_update(NodeTypeInterface $type) {
    -    if ($config->get('child_type') == $type->getOriginalId()) {
    +    if ($config->getOriginal('child_type', FALSE) == $type->getOriginalId()) {
    

    Why? $config is already editable and override-free.

  3. +++ b/core/modules/color/color.module
    @@ -196,7 +196,10 @@ function color_scheme_form($complete_form, FormStateInterface $form_state, $them
    -  $current_scheme = \Drupal::config('color.theme.' . $theme)->get('palette');
    +  // Note: we use configuration without overrides since this information is used
    +  // in a form and therefore without doing this would bleed overrides into
    +  // active configuration.
    +  $current_scheme = \Drupal::config('color.theme.' . $theme)->getOriginal('palette', FALSE);
    

    Since this is used in a form, shouldn't this be \Drupal::configFactory()->getEditable('color.theme.' . $theme)->get('palette'); instead?

  4. +++ b/core/modules/locale/locale.module
    @@ -647,7 +647,7 @@ function locale_form_language_admin_edit_form_alter(&$form, FormStateInterface $
    -      '#default_value' => locale_translate_english(),
    +      '#default_value' => \Drupal::config('locale.settings')->getOriginal('translate_english', FALSE),
    

    Since this is used in a form, shouldn't this be \Drupal::configFactory()->getEditable('locale.settings')->get('translate_english'); instead?

effulgentsia’s picture

Partial review. Only got as far as language.module, and skipped over \Drupal\Core\Config

  1. +++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
    @@ -0,0 +1,71 @@
    + * override free and mutable config objects if the configuration name is in the
    + * array returned by the getEditableConfigNames() implementation.
    

    The sentence before this is great, but let's also add docs explaining why we're requiring explicit declaration of which config to make editable rather than doing all of them.

  2. +++ b/core/modules/aggregator/src/Plugin/AggregatorPluginSettingsBase.php
    @@ -25,6 +27,29 @@
    +  use ConfigFormBaseTrait;
    +
    +  /**
    +   * Contains the configuration object factory.
    +   *
    +   * @var \Drupal\Core\Config\ConfigFactoryInterface
    +   */
    +  protected $configFactory;
    +
    +  /**
    +   * Constructs a Drupal\Component\Plugin\PluginBase object.
    +   *
    +   * @param array $configuration
    +   *   A configuration array containing information about the plugin instance.
    +   * @param string $plugin_id
    +   *   The plugin_id for the plugin instance.
    +   * @param mixed $plugin_definition
    +   *   The plugin implementation definition.
    +   */
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory) {
    +    parent::__construct($configuration, $plugin_id, $plugin_definition);
    +    $this->configFactory = $config_factory;
    +  }
    

    I don't see how this centralization helps the subclasses. From what I can see, it just makes them change their constructor signature for no benefit.

  3. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
    @@ -55,10 +56,13 @@ class ConfigSingleImportForm extends ConfirmFormBase {
    -  public function __construct(EntityManagerInterface $entity_manager, StorageInterface $config_storage) {
    +  public function __construct(EntityManagerInterface $entity_manager, StorageInterface $config_storage, ConfigFactoryInterface $config_factory) {
    

    Is adding DI of the config factory within this issue's scope? FormBase has a configFactory() method, so how about just using that, and making the DI of it a separate issue?

  4. +++ b/core/modules/config/tests/config_test/src/SchemaListenerController.php
    @@ -16,11 +18,30 @@
       /**
    +   * Constructs the SchemaListenerController object.
    +   *
    +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    +   *   The config factory.
    +   */
    +  public function __construct(ConfigFactoryInterface $config_factory) {
    +    $this->configFactory = $config_factory;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container->get('config.factory')
    +    );
    +  }
    +
    +  /**
        * Tests the WebTestBase tests can use strict schema checking.
        */
       public function test() {
         try {
    -      $this->config('config_schema_test.schemaless')->set('foo', 'bar')->save();
    +      $this->configFactory->getEditable('config_schema_test.schemaless')->set('foo', 'bar')->save();
    

    Same question.

  5. +++ b/core/modules/contact/contact.module
    @@ -194,7 +194,8 @@ function contact_form_user_admin_settings_alter(&$form, FormStateInterface $form
    -    '#default_value' => \Drupal::config('contact.settings')->get('user_default_enabled'),
    +    // @see \Drupal\Core\Form\ConfigFormBase::config()
    +    '#default_value' => \Drupal::configFactory()->getEditable('contact.settings')->get('user_default_enabled'),
    

    Do we need that @see at all? If so, should we point it to the trait instead?

  6. +++ b/core/modules/contact/src/ContactFormEditForm.php
    @@ -7,14 +7,43 @@
    +  /**
    +   * Constructs the ContactFormEditForm.
    +   *
    +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    +   *   The config factory.
    +   */
    +  public function __construct(ConfigFactoryInterface $config_factory) {
    +    $this->configFactory = $config_factory;
    +  }
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container->get('config.factory')
    +    );
    +  }
    

    Another case of factory injection as scope creep.

  7. +++ b/core/modules/language/language.module
    @@ -339,7 +339,8 @@ function language_negotiation_url_prefixes_update() {
    -  \Drupal::config('language.negotiation')
    +  // @fixme this is super dodgy - overrides can bleed.
    +  \Drupal::configFactory()->getEditable('language.negotiation')
    

    Let's file a followup and link to it. Would be good to explain what can bleed, cause it's not immediately obvious from quick code inspection.

  8. +++ b/core/modules/language/language.module
    @@ -355,7 +356,8 @@ function language_negotiation_url_domains() {
    -  \Drupal::config('language.negotiation')
    +  // @fixme this is super dodgy - overrides can bleed.
    +  \Drupal::configFactory()->getEditable('language.negotiation')
    

    Same.

  9. +++ b/core/modules/language/language.module
    @@ -445,19 +447,6 @@ function language_get_browser_drupal_langcode_mappings() {
     /**
    - * Stores language mappings between browser and Drupal language codes.
    - *
    - * @param array $mappings
    - *   An array containing browser language codes as keys with corresponding
    - *   Drupal language codes as values.
    - */
    -function language_set_browser_drupal_langcode_mappings($mappings) {
    -  $config = \Drupal::config('language.mappings');
    -  $config->setData($mappings);
    -  $config->save();
    -}
    

    Nice cleanup, but let's do it as a separate issue.

  10. +++ b/core/modules/language/language.module
    @@ -505,7 +494,9 @@ function language_form_alter(&$form, FormStateInterface $form_state) {
    -  \Drupal::config('system.site')->set('langcode', $form_state->getValue('site_default_language'))->save();
    +  // @fixme can config overrides bleed here? Default language is injected into
    +  //   the container.
    +  \Drupal::configFactory()->getEditable('system.site')->set('langcode', $form_state->getValue('site_default_language'))->save();
    

    Unless the answer to this is a quick "nope, we're fine", let's open a followup (or add it to the earlier one).

  11. +++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
    @@ -17,7 +16,7 @@
    -class ContentLanguageSettingsForm extends ConfigFormBase {
    +class ContentLanguageSettingsForm extends FormBase {
    

    (plus all the related changes) Seems out of scope.

  12. +++ b/core/modules/language/src/Form/NegotiationBrowserDeleteForm.php
    @@ -25,6 +27,25 @@ class NegotiationBrowserDeleteForm extends ConfirmFormBase {
    +   * Constructs the NegotiationBrowserDeleteForm object.
    +   *
    +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    +   *   The config factory.
    +   */
    +  public function __construct(ConfigFactoryInterface $config_factory) {
    +    $this->configFactory = $config_factory;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container->get('config.factory')
    +    );
    +  }
    

    And another out of scope DI addition.

alexpott’s picture

Issue summary: View changes
FileSize
21.55 KB
117.03 KB

Patch needed another reroll plus I've fixed the StaticMenuLinkOverridesTest to run (and hopefully) pass again.

#64 review

  1. Fixed - also after discussion with @dawehner in IRC i've made the static menu link override config impossible to override
  2. Legacy of previous version of this patch - removed so fixed. However this highlights an issue with hooks and config - implementations are going to have to be super-aware about overrrides.
  3. This again highlights the issue of how override aware developers are going to need to be. Overriding color configuration makes no sense whatsoever - since this is used to write out css files etc. have a look at color_get_palette() this is used in the form and in the rewriting of stylesheets. I've changed everything to use getEditable() which has revealed a lack of test coverage (no surprise). I have no idea what to do with the config accessed in color_library_info_alter() and color_preprocess_page(). Also I guess we should move color_form_system_theme_settings_alter() into a form alter object too.
  4. Fair enough. Fixed.

#65 review

  1. Improved the docs.
  2. Because we're using ConfigFormBaseTrait on the base class. I guess we could push the trait usage upto the concrete implementations... doing that. These plugins based on simple config are freaky.
  3. Fair enough - fixed.
  4. There is no configfactory() method here. I think we should keep the injection and this is a test.
  5. No need for the @see - fixed
  6. Sure we have the method here because of FormBase - fixed
  7. Created #2403229: language.negotiation configuration can have overrides bleed in
  8. Created #2403229: language.negotiation configuration can have overrides bleed in
  9. Why? This code is broken by this patch and it contains the fix. I've removed the injection of config factory and used the formbase::configFactory() method.
  10. Removed comment - if people override the default language with settings.php they are responsible for handling the side effects. Might consider moving this to a config file that we prevent overrides of.
  11. Well it was related because it would have had to implement an empty getEditableConfigNames() method but I've already got it fixed in #2403101: ContentLanguageSettingsForm is not a config form
  12. Fixed.
alexpott’s picture

Issue tags: -Needs tests
FileSize
121.89 KB
7 KB

Added unit tests for ImmutableConfig and ConfigFormBaseTrait - I think that means we have full test coverage. Discovered some bugs in ImmutableConfig :) yay for phpunit exception testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -104,17 +117,43 @@ public function getOverrideState() {
    +  /**
    +   * Returns a configuration object for a given name.
    +   *
    

    Just curiuos, can we can the (im)mutability into the description?

  2. +++ b/core/lib/Drupal/Core/Config/ImmutableConfig.php
    @@ -0,0 +1,61 @@
    +    throw new ImmutableConfigException(String::format('Can not set values on immutable configuration !name:!key', ['!name' => $this->getName(), '!key' => $key]));
    

    just in general should those exceptions point to configfactory->getEditable?

  3. +++ b/core/modules/aggregator/src/Form/SettingsForm.php
    index f6c1412..f51a403 100644
    --- a/core/modules/aggregator/src/Plugin/aggregator/processor/DefaultProcessor.php
    
    --- a/core/modules/aggregator/src/Plugin/aggregator/processor/DefaultProcessor.php
    +++ b/core/modules/aggregator/src/Plugin/aggregator/processor/DefaultProcessor.php
    

    I'm confused here ... on actual runtime we still rely on cf->get() so we get the overridden version, which itself though is also immutable, okay confusion cleared.

alexpott’s picture

FileSize
8.41 KB
122.71 KB

re #68

  1. Seems unnecessary - this is a protected method and the whether or not the caller wants a mutable object is a parameter
  2. Sure why not. I've also changed the root exception type to a LogicException since according to the PHPdocs - they are an "Exception that represents error in the program logic. This kind of exceptions should directly lead to a fix in your code."

This patch also removes the cannotOverride property and canOverride method from the ConfigFactory after discussion with @catch in IRC. Whilst it is totally unadvisable to override core.extension since the raw config is read in several places to avoid circular dependencies we can just document this in default.settings.php. The override system in Drupal has always been powerful. D8 it is super powerful since you can also override config entities, installed modules - anything stored in config. And yes it will be very easy to hose your site BUT that is through editing settings.php or using the API in crazy ways.

effulgentsia’s picture

And yes it will be very easy to hose your site BUT that is through editing settings.php or using the API in crazy ways.

Opened #2404245: Add a config override validator to throw an exception when settings.php or a module override something they shouldn't? for discussion on that. Not a blocker for this issue.

effulgentsia’s picture

Why? This code is broken by this patch and it contains the fix.

Because it's possible to fix without removing the function, and during the beta phase, removing an ancillary public function seems like something that should have its own discussion rather than sneaking into a 100k patch: #2404407: language_set_browser_drupal_langcode_mappings() is a useless wrapper, so remove it.

effulgentsia’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -132,11 +158,11 @@ public function get($name) {
    -  public function loadMultiple(array $names) {
    +  public function loadMultiple(array $names, $immutable = TRUE) {
    ...
    +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -355,7 +355,7 @@ public function updateLockedLanguageWeights() {
    -    foreach ($this->configFactory->loadMultiple($config_ids) as $config) {
    +    foreach ($this->configFactory->loadMultiple($config_ids, FALSE) as $config) {
    

    This looks like a vector for override bleed, since the $immutable parameter isn't paired with a setOverrideState(FALSE). Would it make sense to do the same here as for get()/getEditable()/doGet() and introduce loadMultiple()/loadMultipleEditable()/doLoadMultiple()?

  2. +++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
    @@ -0,0 +1,77 @@
    +    /** @var \Drupal\Core\Config\ConfigFactoryInterface $config_factory */
    +    if (method_exists($this, 'configFactory')) {
    +      $config_factory = $this->configFactory();
    +    }
    +    elseif (property_exists($this, 'configFactory')) {
    +      $config_factory = $this->configFactory;
    +    }
    +    if (!isset($config_factory) || !($config_factory instanceof ConfigFactoryInterface)) {
    +      throw new \LogicException('No config factory available for ConfigFormBaseTrait');
    +    }
    

    This looks like unused complexity if the trait is only ever used by classes that extend FormBase, since that has a configFactory() method. What about making the trait declare an abstract configFactory(), making that explicitly part of the contract for using the trait? Doesn't seem like an unreasonable requirement even for a case of a non-FormBase class wanting to use the trait.

alexpott’s picture

FileSize
122.17 KB

re #71 that issue has now landed. It is hardly sneaking in when we are discussing it on issue.

re #72

  1. Nope that is not a vector for override bleeds. Overrides are ignored in HEAD when config is saved - the override bleed problem only occurs when the value is being set on the config. The following code (with drupal HEAD not this patch) does not result in the site name override being stored in configuration:
    // in settings.php
    $config['system.site']['name'] = 'My Drupal site';
    
    // in some code somewhere...
    $config = \Drupal::config(''system.site');
    $config->set('mail', 'me@example.com')->save();
    
  2. This is not unused. It is used in the Aggregator plugins and it will be used in the form alters - unless you want even more boilerplate code in the OO form alters. And I've added extensive testing.

Patch rerolled.

Gábor Hojtsy’s picture

Fun fact. I just found #2095289: Make getEditableConfigNames() public and use it in config_translation where @tstoeckler ventured into introducing basically the getEditableConfigNames() introduced here. It got some good reviews from @effulgentsia and others but then got forever postponed. Marking as duplicate of this one now.

tstoeckler’s picture

I don't think that one is duplicate. getEditableConfigNames() is protected here, but we need to call it from the outside, i.e. in config translation module.

This is also missing the bubbling up of the config names from forms which contain plugin configurations, which is discussed over there in depth.

Gábor Hojtsy’s picture

I think making it public or private is a tiny change, would not consider that a big difference. Would we not need to bubble up all plugin config names though if we need them to be mutable?

tstoeckler’s picture

I haven't followed this patch enough to be able to answer that question.

But sure if we can make that method public and add an interface as part of that issue, then that would indeed make the other issue obsolete.

Gábor Hojtsy’s picture

No wait, I did not want to extend the scope of this issue, not by a single bit. I hoped this gets far enough towards #2095289: Make getEditableConfigNames() public and use it in config_translation that a miniscule diff does not necessitate that issue but the consumer issue could do that adjustment instead. I'll reopen #2095289: Make getEditableConfigNames() public and use it in config_translation postponed on this one. I still think there are lots of similarities and #2095289: Make getEditableConfigNames() public and use it in config_translation will need to be totally reworked if it is ever to be worked on again.

effulgentsia’s picture

I discussed #73.1 with alexpott, he'll post the conclusion of that. I agree with #73.2. I re-reviewed the entire patch again, and other than #73.1, here's all I found to complain about:

  1. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
    @@ -135,7 +140,7 @@ public function saveOverride($id, array $definition) {
    -      $all_overrides = $this->getConfig()->get('definitions');
    +      $all_overrides = $this->getConfig()->get('definitions', FALSE);
    

    get() doesn't have a 2nd param.

  2. +++ b/core/modules/color/color.module
    @@ -169,7 +169,9 @@ function color_get_palette($theme, $default = FALSE) {
    -  return \Drupal::config('color.theme.' . $theme)->get('palette') ?: $palette;
    +  // Getting a mutable override-free object because overriding color
    +  // configuration makes no sense.
    +  return \Drupal::configFactory()->getEditable('color.theme.' . $theme)->get('palette') ?: $palette;
    

    I don't understand this comment. Why does overriding color config make no sense, and why is it up to this function to decide that?

  3. +++ b/core/modules/color/color.module
    @@ -196,7 +198,12 @@ function color_scheme_form($complete_form, FormStateInterface $form_state, $them
    -  $current_scheme = \Drupal::config('color.theme.' . $theme)->get('palette');
    +  // Note: we use configuration without overrides since this information is used
    +  // in a form and therefore without doing this would bleed overrides into
    +  // active configuration.
    +  // Getting a mutable override-free object because overriding color
    +  // configuration makes no sense.
    +  $current_scheme = \Drupal::configFactory()->getEditable('color.theme.' . $theme)->get('palette');
    

    Do we need that second sentence if we have the first?

  4. +++ b/core/modules/locale/locale.module
    @@ -665,6 +665,8 @@ function locale_form_language_admin_edit_form_alter_submit($form, FormStateInter
    + *
    + * @fixme remove this method
      */
     function locale_translate_english() {
    

    If this is important to do, let's turn it into a @todo with an issue link.

  5. +++ b/core/modules/simpletest/src/KernelTestBase.php
    @@ -202,7 +202,10 @@ protected function setUp() {
    -    // In order to use theme functions default theme config needs to exist.
    +    // In order to use theme functions default theme config needs to exist. This
    +    // configuration is not actually saved because it does not need to be. If it
    +    // was saved then all KernelTestBase tests would need to install the system
    +    // module of the schema would be missing.
    

    Why does it not need to be saved? Because of cache? If so, let's say that. Also, on the last line should "of" be "or"?

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -104,15 +104,41 @@ public function getOverrideState() {
+  public function getEditable($name, $override_free = TRUE) {

Oops. Missed this in #79, but that 2nd param is no longer used, and isn't in the interface, so can be removed.

alexpott’s picture

Issue tags: +Needs issue summary update
FileSize
121.48 KB
4.35 KB

re #79

  1. Fixed
  2. Because the function is only used in forms of in functions that write out css. Config overrides of color configuration make no sense because everything is written to css.
  3. Fixed.
  4. These config wrappers are dangerous because they are so tempting to call from forms. But this also falls into the realm of config that should be overridden so I've just removed the @fixme
  5. Improved documentation

#80.1 - fixed

We need to bring the issue summary up-to-date with the current state of the patch. Going to work on #73.1 next.

alexpott’s picture

Here's a summary of opinions I recently expressed to @effulgentsia.

I don't like this issue and the way we are dealing with overrides whilst administrating the site. It adds heaps of complexity both for the developer and the administrator. I don't like that an administrator does not know when configuration is overridden and they are seeing one thing a form edit field and another as a runtime effect. It's all feels extremely complex and confusing. I think the ability to manually set the override state from outside the factory is super dangerous.

If you have overrides in settings.php - you know you have them - deal with it. The project has chosen to implement the side effects and core should not have to deal with that. If you are using language overrides or some other override vector (eg domain) then edit your site in the default language and on the default domain.

One possible solution is to preserve D7's override bleeding in the administration system but disable all other overrides.

If we continue the way we are going I think I might repurpose https://www.drupal.org/node/1934152 to add a form element wrapper to the ConfigFormBaseTrait that indicates a value is overridden and what the override is.

On the other hand...

Multiple people working on a project - not everyone sees everything - so this makes it easier to manage.

aspilicious’s picture

Multiple people working on a project - not everyone sees everything - so this makes it easier to manage.

Certainly when a project gets moved from one person/team to another. For example when it shifts from development to maintenance.

effulgentsia’s picture

Status: Needs review » Needs work

I agree with #83.

Re #82, all good points. But it seems to me like when weighing the pros and cons, you're not suggesting that we change course on this issue. If I'm wrong about that, please reset this to Needs Review, but otherwise:

We need to bring the issue summary up-to-date with the current state of the patch. Going to work on #73.1 next.

Setting to needs work for both of those plus the change record. More eyes on the patch wouldn't hurt, but I'm already comfortable with RTBCing this once those remaining items are done.

If we continue the way we are going I think I might repurpose https://www.drupal.org/node/1934152 to add a form element wrapper to the ConfigFormBaseTrait that indicates a value is overridden and what the override is.

+1

alexpott’s picture

Last night I was coding in my dreams and I realised we have a problem with config events and overrides - the config factory at this point with be override free for saves and deletes but not renames - so if the event wants to do something with runtime config then it has to set the config factory back to using overrides and set it back to override free. This is horrible.

alexpott’s picture

Gábor Hojtsy’s picture

I think we have rich experiences in Drupal 7 about override problems. We had them in Drupal 7 and we wanted to shoot to avoid them in Drupal 8. We needed to build in overrides as a core thing in Drupal 8 because we did not have a better way to implement translations, dev/prod environment specific settings, spaces, groups, domains, etc. And we needed this in core so that contrib knows that they may be there and they need to intelligently deal with it. That in itself does not predestinate the API complexity needed of course.

What we did on the API level is overrides are treated separately in the config objects so a config load, config set and config save will NOT bleed overrides to the configuration file. The realization that lead to this issue is forms, REST API endpoints, etc. are not immune to that same problem because we possibly load the data with overrides and then when we get the data back we write the original data over.

We have/had admin upcasting without overrides and the config() method in ConfigFormBase load without overrides to try to avoid that but on the course of this issue Alex found several problem areas where we did not apply our principles right in core. So the whole idea of strict immutability would be to force people to do the right thing instead of letting them do the natural thing which would likely lead to override bleeding. The proposal of this issue is that we need to declare when we get the config object that we want to write it AND we need to consistently do that in the provider (eg. form) and the updater (eg. submit). Those then would not have overrides so we don't run into this problem.

As for how common maintaining overrides separate from admin forms where admin forms let you edit the original values here is a video snippet from strongarm: https://www.youtube.com/watch?v=007YfTbiHHE#t=334. The module page says this module is used on 120k Drupal 7 sites. I am not sure telling site admins to "go to the original language" version of the config form, because that may still be on the wrong domain, in the wrong group, etc. There is a matrix of overrides by different conditions that may apply. So the forms and admin listings need to not use the overrides. If they would use them then the bleeding of sensitive data like API keys would be inevitable. We can tell site owners we told you so, but if they got used to the ways strongarm supported them to work with (see above video), that was exactly the protection we are looking at here for overrides to be able to achieve. So if that would not happen with admin lists and forms, that would be a step back. I guess a strongarm like module could still override the listing classes and the upcasting, but doing that consistently sounds like a nightmare.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
6.13 KB
124.44 KB

So re #72.1 let's just remove the ability to get multiple config objects to save. We only have one use case and it's incorrect! See #2407093: Refactor ConfigurableLanguageManager::updateLockedLanguageWeights() to save configuration entities rather than raw config

I've included the patch on #2407093: Refactor ConfigurableLanguageManager::updateLockedLanguageWeights() to save configuration entities rather than raw config in the patch attached.

Status: Needs review » Needs work

The last submitted patch, 88: 2392319.88.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
776 bytes
125.19 KB

A test has got in using \Drupal::config().

alexpott’s picture

Created the change record https://www.drupal.org/node/2407153 - please review / improve :)

Gábor Hojtsy’s picture

Issue tags: -Needs change record

Added a bunch of text on the dangers averted as well as the reasons this is being done and how the form code helps with doing the right thing. Also added a whole new section on form altering and what happens there. See https://www.drupal.org/node/2407153/revisions/view/8013861/8014005. Also mentioned the exception in that last section, but we can mention it sooner as well. I think this way the text explains the problem in more detail. It may repeat itself a bit, but I don't think that is a problem per say. Its a difficult problem and worth explaining from multiple angles.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Issue summary, change record, and patch all look great. If a committer would prefer to commit #2407093: Refactor ConfigurableLanguageManager::updateLockedLanguageWeights() to save configuration entities rather than raw config separately, then this will need a reroll after that lands. Otherwise, since the patch there is small, non-disruptive, RTBC, and included in the latest patch here, I'd be fine with this being committed as-is, and then that issue closed. Therefore, RTBC.

alexpott’s picture

Issue tags: +Triaged D8 critical

This critical has been traiged by @xjm, @webchick, @catch, @effulgentsia and me.

See:

  • Beta evaluation in issue summary
  • Discussions about the solution in comments 23 to 29
  • Discussions about whether we should do this in 83 to 87

[Edit: to show where in the issue some of this discussion is represented]

alexpott queued 90: 2392319.90.patch for re-testing.

effulgentsia’s picture

Title: Config objects should by default be immutable » Config objects (but not config entities) should by default be immutable
Issue summary: View changes

Clarified title and summary to cover config entities.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Have been reviewing this as it goes, and while this doesn't feel completely comfortable I think it's definitely the best we can do at the moment.

Extremely minor nitpick fixed on commit:

diff --git a/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
index 11249f8..1cbac3c 100644
--- a/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
+++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
@@ -20,7 +20,7 @@
  * configuration overrides to the stored configuration since overrides are often
  * environment specific. Default values of form elements should be obtained from
  * override free configuration objects. However, if a form reacts to
- * configuration in anyway, for example sends an email to the system.site:mail
+ * configuration in any way, for example sends an email to the system.site:mail
  * address, then it is important that the value comes from a configuration
  * object with overrides. Therefore, override free and editable configuration
  * objects are limited to those listed by the getEditableConfigNames() method.

This is going to require contrib modules to update so better to get it in sooner than later.

Committed/pushed to 8.0.x, thanks!

  • catch committed 73069e0 on 8.0.x
    Issue #2392319 by alexpott, effulgentsia: Config objects (but not config...
larowlan’s picture

Published change record

Xano’s picture

Shiny! Now we have ConfigFactoryInterface::getEditable(), do we still need the config helper methods on form base classes/traits? I'm asking because it looks like if you get a config object in a form with the intention to update with saved form values, using ConfigFactoryInterface() won't work and ImmutableConfig will throw useful exceptions anyway.

Gábor Hojtsy’s picture

@Xano: the config() method on the config form base class is exactly there to help you load the right version with little effort. See the form example in the change record: https://www.drupal.org/node/2407153

Xano’s picture

I am familiar with that method. However, ConfigFormBaseTrait feels like we have painted ourselves in a corner a bit, especially because getEditableConfigNames() means one cannot load a mutable *and* an immutable version of the same config object. I admit I have not yet run into problems, but I can imagine situations where one would like to get both variants of a config object for things like inline previews.

TLDR; Because these helpers are not all optional, it doesn't just feel like we're giving developers advice, but also that we're forcefully making them do things our way.

Gábor Hojtsy’s picture

@Xano: nothing makes it impossible to use the injected config factory if you need the same object for other reasons. I don't think we force people into a corner, they still have the injected config factory handy to do whatever they want. The resulting code will make it evident if they explicitly used it yes.

Xano’s picture

Ah, I see. The injected config factory used to be private, but that's no longer the case. That solves most of my concerns. I don't like the fact that getEditableConfigNames() is abstract, though. Is there something we can do about that?

Gábor Hojtsy’s picture

@Xano: ConfigFormBase is for editing configuration. If you want to implement a form that does not edit configuration, you can use FormBase. If your form edits configuration, it will have at least one key that it would take as editable. The trait has no way to know what that key will be so it can only define it as abstract.

Xano’s picture

I know what ConfigFormBase is for and I loved it until it introduced an abstract method for something I do not need. That's why I asked if we can undo this DX regression in such a way that we keep the trait functionality that people apparently like.

Gábor Hojtsy’s picture

@Xano: You have a ConfigFormBase form and you do not need to edit configuration in that form? What do you use it for?

Xano’s picture

ConfigFormBaseTrait does not add any value to ConfigFormBase in my opinion. I do not like it and prefer not to use it, but instead inject the config factory and call it myself. However, that is not what I am trying to discuss here.
I like that ConfigFormBase lets met create a config form that looks like any other config form, but if I want to continue using ConfigFormBase for this, I will now also have to implement a method that I don't need, simply because others feel it makes their life easier. While making lives easier is an admirable thing, I don't like that this particular situation makes my life harder and I would like to find a solution for that. For instance, can we provide a default implementation of that method?

Gábor Hojtsy’s picture

@Xano: that its necessary to implement getEditableConfigNames() is exactly to make you not go into that trap where you get non-editable config that you cannot save, which you may band-aid by using getEditableConfig() in your submit handler (because that is where you get the exception), which will "work" but will spill overrides into config possibly leading to security issues and go exactly against the goals of this solution. If we un-abstract it then that is what we get. Feel free to use FormBase and inject a config factory for your use.

effulgentsia’s picture

@Xano: perhaps open a followup for making ConfigFormBaseTrait::getEditableConfigNames() not abstract and default to returning an empty array? My current thinking is that I agree with Gábor that it's not a good idea, because extending ConfigFormBase without providing at least one form element that edits some configuration isn't good, and having a form element that edits configuration without using $this->config() in both the #default_value assignment and the submit handler to ensure consistency across the two also isn't good. But current thinking can always change. Another possibility to consider in that follow-up is maybe making the default return value some kind of marker that says "all"?

alexpott’s picture

Knowing what configuration if being used in which forms is also useful for translation see #2095289: Make getEditableConfigNames() public and use it in config_translation

YesCT’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

vijaycs85’s picture

Looks like this one in comment missed as well

diff --git a/core/modules/system/tests/modules/system_mail_failure_test/src/Plugin/Mail/TestPhpMailFailure.php b/core/modules/system/tests/modules/system_mail_failure_test/src/Plugin/Mail/TestPhpMailFailure.php
index a4c6a34..a18a783 100644
--- a/core/modules/system/tests/modules/system_mail_failure_test/src/Plugin/Mail/TestPhpMailFailure.php
+++ b/core/modules/system/tests/modules/system_mail_failure_test/src/Plugin/Mail/TestPhpMailFailure.php
@@ -16,7 +16,7 @@
  * This class is for running tests or for development. To use set the
  * configuration:
  * @code
- *   \Drupal::config('system.mail')->set('interface.default', 'test_php_mail_failure')->save();
+ *   \Drupal::configFactory()->getEditable('system.mail')->set('interface.default', 'test_php_mail_failure')->save();
  * @endcode
  *
  * @Mail(
vijaycs85’s picture

Priority: Critical » Normal
Status: Closed (fixed) » Needs review
FileSize
811 bytes

Here is a patch for #115

catch’s picture

Priority: Normal » Critical
Status: Needs review » Closed (fixed)

Please open a new issue for the follow-up to preserve issue metadata and commit history.

vijaycs85’s picture

Xano’s picture

xjm’s picture