Problem/motivation

There are common APIs to access configuration across different parts of Drupal:

1. FormBase::config() proxies your call to the configuration factory.
2. ConfigFormBase::config() does the same but ...

The behaviour is very different. ConfigFormBase::config() will disable all overrides with the assumption that you deal with a configuration form to display and save non-overriden configuration. The original behavior of FormBase::config() is overriden, so there is no direct way to access configuration with all overrides applied.

Let's say you have an admin form that sends an email when a setting is updated. It sends to the site email address. You override that of course in $global['conf']['system.site']['mail'] = 'localdev@example.com'; so your customer will not get the emails from your developer site. You don't want this local override to get into the active configuration so that it is not applied when you edit admin settings is good. However in the form that sends email, it will also not be applied for sending the email. So pooof, you sent emails to your customer even though there is nothing evident in your code, that the config() method that in the other class applied overrides did not apply overrides in this class.

This is an example that some people mentioned, but I suspect there are probably other examples that I did not think of yet.

Proposed resolution

A. Make global overrides apply all the time. This would make local dev overrides end up in active configuration so not sure developers want to need to review those changes all the time?
B. Make the API explicit about which config methods return possibly overriden values and which return raw original values all the time. (Note that this may have side effects for how we solve entity listings, see #2136559: Config entity admin lists show configuration with overrides (eg. localized)).
C. Decide this is not a big problem. We are not concerned with dev environment emails from admin forms and other possible side effects that may come.

Remaining tasks

Discuss.

User interface changes

None.

API changes

TBD.

#1929136: Fix override-free context, move global config overrides back to an event listener
#1763640: Introduce config context to make original config and different overrides accessible
#1646580: Implement Config Events and Listeners, and storage realms for localized configuration
#1932336: Use override free configuration context for SystemConfigFormBase
#2136559: Config entity admin lists show configuration with overrides (eg. localized)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Posted this to the security team:

Hey,

In Drupal 8 if you put something in global $conf, that overrides your config by default but it may be ignored if the code specifically asked for an "override free context" or if there are translations for the value, those would trump global overrides. It would be important to get this right and I think there are people using global $conf as a security enforcement system (beyond admin permissions), which is NOT currently possible in Drupal 8 due to how it is applied. I think the way it works now makes sense for use cases where you want a soft fallback default that cannot be edited, but if we consider global $conf overrides as a security measure, we should somehow support that use case too, or maybe restore global $conf to be an enforcement system like it was in Drupal 7 and not have soft overrides or have them in a different way.

Your feedback welcome in http://drupal.org/node/1934152

Gábor

And tweeted this to get more people from the larger community: https://twitter.com/gaborhojtsy/status/308971595563679744 (earlier in https://twitter.com/gaborhojtsy/status/308969857951608832 which was removed due to a spelling error).

Hope this gets us some insight into what people want :)

Gábor Hojtsy’s picture

Issue tags: +Security
DamienMcKenna’s picture

Issue tags: -Security

Being able to override settings per-hostname is extremely important for many developers and many sites. The settings.php $conf array is how I ensure that production sites have the correct production-worthy settings enabled while my local install has all caching disabled, uses alternative API keys, etc. This must be possible. How can I help? :-)

DamienMcKenna’s picture

Issue tags: +Security

sorry.

Gábor Hojtsy’s picture

@DamienMcKenna: yeah, so overriding for different host names is an interesting use case. Do you mean overriding conditionally in settings.php or just using different settings.php files in different environments? global $conf is still used and applied EXCEPT if the code is in an "override free" environment which is only applied for admin forms. So if you go edit your site setup, your $conf overriden values will not be present there and not saved. Which is logical if you use logic in settings.php to apply to different domains, but probably irrelevant if you just use static settings.php overrides on different setups.

DamienMcKenna’s picture

@Gábor: I've used multiple combinations of this:

  • Different values for a "true" multisite install, e.g. different API keys for different Mailchimp accounts.
  • Different values for different instances of the same site, e.g. API keys for the production vs dev Acquia Network accounts to avoid corrupting the production search index :) and different performance tuning settings.

I've had a blog post kicking around in my head for 2+ years on how I handle variables, I really need to write it down..

Gábor Hojtsy’s picture

So the question is if its a bigger problem if those overrides creep into settings forms and get saved as original config on sites, or if its a bigger problem if they don't creep into forms but then they are not applied when editing those config (so eg. if the global override changes the theme that would not be possible on the admin editing form). Which is the bigger problem? :)

Owen Barton’s picture

I have done many of the same things as @DamienMcKenna - I think the API key use case is one to consider for sure - how to we ensure that "production only" config doesn't leak to staging and vice versa (you don't want to get your real and testing Paypal API keys mixed up, trust me!). If there is not already a good way to ensure this, $conf might be a reasonable solution.

I have also used the case of having multiple hostnames all using sites/default (and the same database/files etc) and using $conf to switch out some key variables (frontpage, language, site name etc) based on the hostname. This is all stuff I could have done (less efficiently) with using sites/hostname or other means however.

Another use case to consider is for automated hosting setups - allowing the host to set the database and reverse proxy configuration dynamically (as Acquia does, for instance), and if necessary change it - it seems preferable if this can be done without needing to trigger a config reload or other operation that may have side effects.

catch’s picture

Another use case to consider is for automated hosting setups - allowing the host to set the database and reverse proxy configuration dynamically (as Acquia does, for instance), and if necessary change it - it seems preferable if this can be done without needing to trigger a config reload or other operation that may have side effects.

Anything that can't actually be stored in the database/CMI is in $settings now - this distinguishes between things that are exclusively configured in settings.php vs. overrides. There's likely a grey area where something is used after CMI becomes available but you don't ever want a UI for it, I'd lean towards $settings for those too.

I think it's OK that conf overrides don't show up in forms and aren't saved back, that's arguably a bug fix compared to Drupal 7, there's two things that could use clarifying though:

1. Should we warn people on the form somewhere that values are overridden in $conf and their changes won't take effect?

2. We should put a huge warning on the 'no context' context to make it clear that $conf overrides are disabled for that context. I can't really think of a reason to use it except for the forms and maybe the new drush vget.

DamienMcKenna’s picture

@Gábor: With my dev workflow, nothing is changed via the admin UI on production, so these overridden settings being exported to the config files elsewhere doesn't matter, the changes won't be committed unless I've reviewed every single line and given it my blessing.

attiks’s picture

my 2 cents: I use it a lot to differentiate between environments (all running on top of aegir)

Some things that can be in there besides API keys:

  global $conf;
  $conf['https'] = TRUE;
  unset($conf['cache']);          // disable hardcoded caching
  unset($conf['preprocess_css']); // disable hardcoded css aggregation
  unset($conf['preprocess_js']);  // disable hardcoded js aggregation
  $conf['xmlsitemap_submit'] = 0; // disable XML Sitemap for dev.domain
  $conf['xmlsitemap_update'] = 0; // disable XML Sitemap for dev.domain

agentrickard’s picture

I do address a similar issue over in Domain Access, where I have a small hook that can be used to declare which admin forms contain domain-sensitive values. In an ideal world, the config system might detect these hard overrides and flag them for the user via a message.

That would, however, involve some trickery to match $conf values with form config values. Perhaps if we disallowed putting "loose" $conf variables in settings.php and instead allowed for a standard function call for overrides:

e.g. Drupal 7:

// settings.php
// Custom settings.php lines
$conf['foo'] = 'Bar';

Drupal 8:

// settings.php
// Load config variable overrides within settings.php
$conf = settings_override();

function settings_override() {
  return array('foo' => 'Bar');
}

Then the config form process could invoke settings_override() and look for matches.

/me stops thinking out loud.

[edit] And notes that catch said something similar in #9 while I was writing this.

Gábor Hojtsy’s picture

I don't think there is a common element in config forms, etc. views, node types and site settings don't have much in common, also form structure is not related to config structure (as in views, or even the 403/404/front path config is entirely differently presented on the form vs. the config file). It was a point of the CMI initiative *not* to deal with automated form generation. D8MI needs to generate such forms, so we have http://drupal.org/project/config_translation which was so far deemed not a fit for Drupal 8 core. There it is possible to mark elements, because all forms are autogenerated from the schema/data combined. I don't see a reliable way to mark elements in Drupal forms overall where original config is edited.

moshe weitzman’s picture

I think it is a big win to disable form elements where the admin's changes would not make any difference. It is not appropriate IMO to offer a non-overidden value for editing without telling the user whats going on. His edit is likely not to make any difference. We have always done it that way and it has always sucked :(

If config_translation project gets us there, perhaps we should consider it for core.

Gábor Hojtsy’s picture

@moshe: I'm not sure such programatic backreferences are possible. In Drupal 7, system_settings_form() pages were known to store their values in variables and their form elements were named equal to the variables. So you could look at the form element name and put on warnings if it was overridden. Eg. variable module used this to put on notes if the variable was multilingual.

In Drupal 8, there is no central store like variables, and it is not really known where in config would the value be stored. So while you can get the form element name, you don't really know where in config it would end up. For example the user account settings page uses two config files to store settings, one for "base" settings like anonymous user name or whether registration is allowed and one for all the user email subjects and bodies. All are edited on one form altogether. Menus are config entities and the menu editing form merges editing the menu itself with menu items, so it is quicker for the user. So to be able to tell for specific form fields whether they are overridden (globally or in some language), we'd need to have this information on each form element. That is lots of added info to form structures, that is not there now. And then finally, the question of how we generally disable an element comes up. There are potentially complex form elements. Eg. if the site logo upload field would have an inline preview of the logo, how would you make that form element disabled? If you enforce date formats to be in a specific setup, then multiple admin pages would be entirely disabled / pointless. You could try to add new formats, but that would not work.

The config initiative specifically opted to not deal with automated form generation and wanted to leave forms as one-off implementations on top of config. Later config entities somewhat structured this for whatever is implemented through config entities, so you kind of have some structure (and config vs. form relation somewhat programatically discoverable), but still then the form item vs. config key relation does not necessarily work. See menus.

What config translation (contrib) module does it takes the config schema, that describes the structure of the config and generates translation forms based on that. It totally ignores the original forms for config, because it just cannot pull out fields from the various views forms or from the middle of a config entity or cherry-pick elements from different levels from the structure of the user account settings form. So that does not even attempt to do this relation, because it looks pointless. Dries spelled out earlier he sees this module as a parallel form API and not a fit for Drupal 8. Also, this module needs to build up its own relation system between admin paths and config keys, so it can put translate tabs on the right places for example (and go and alter random forms of various structures to put in translate operations to summary tables).

moshe weitzman’s picture

OK, you've convinced me that showing disabled form elements is desireable, but impossible. Sigh.

I guess we need an "override free" state so that we don't save translated values of site slogan and config like that. Can't we just neuter the LocaleConfigOverride in this case? In my world, all conf overrides would *always* be in effect, even during admin forms. This preserves D7 behavior and satisfies the security concern described by @pounard.

Gábor Hojtsy’s picture

Yes, it is totally our decision, whether we want to make global overrides always apply for example, if those showing up in forms is a smaller issue vs. those not being applied on some admin forms. If we add a feature so overrides can optionally apply in override free contexts too, then one can do logic based overrides (such as domain based overrides in contrib) with a system separate from global $conf and it is their choice if it applies in this context or not. We will need a better name for this context then too :)

David_Rothstein’s picture

Title: Figure out override priorities and whether we want a kind of global override to stick » Figure out override priorities and whether we want global overrides to stick (settings.php overrides don't work on all pages)
Priority: Normal » Critical

I believe I just ran into a version of this, and the current behavior is bizarro broken.

You can look at #1926228-5: Performance page provides incorrect/incomplete information about CSS and JS compression for a really simple patch I tried to write which changes the value of some text on an admin form based on a config setting (one which we encourage people to set in settings.php). The patch didn't work and I assume this "don't apply overrides on admin forms" behavior is the reason?

Or here's a simpler example you can try:

  1. Go into settings.php and uncomment the $conf['system.site']['name'] = 'My Drupal site'; line.
  2. Make sure you aren't using the Seven theme (this step is only required because Seven doesn't actually display the site name on the page).
  3. Navigate around the admin pages of your site and observe how some pages say "My Drupal site" at the top and others don't.

Yeah...

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
10.91 KB

@David: sounds like the reason indeed. Looks like one more +1 from you to make global overrides stick. (Considering them creeping into forms not a bug like many people noted but a feature like many other people noted). Here is a possible solution. Will probably not pass tests yet.

1. I kept the override-free context, since it is used in internal config operations to access the original values for import & staging (see code in core/config.inc exclusively, which is not even touched in the patch). With this patch that is the only place where that is used.
3. Added the capability to force overrides. The default override handler will take any override either forced or not. The free context will not take any override, even if forced (may be a possibility for misunderstanding :) and there is a forced only context, which only takes forced values.
2. Added a config.context.only.forced on the DIC to expose this and made the admin forms use it. Made global overrides forced.

Status: Needs review » Needs work

The last submitted patch, global-overrides-forced-19.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

@Moshe:

I guess we need an "override free" state so that we don't save translated values of site slogan and config like that. Can't we just neuter the LocaleConfigOverride in this case? In my world, all conf overrides would *always* be in effect, even during admin forms. This preserves D7 behavior and satisfies the security concern described by @pounard.

Domain module will also use this override system. The overrides of domain module are conditional on which domain you are on, so saving them back to the original config might be an issue or at least confusing. There are unconditional overrides like sometimes global $conf (although I've seen conditions in settings.php files when setting config values in $conf), and there are dynamic overrides like translations or domain overrides. Looks like we want some of them apply to forms but not others. So the question is where is that decision made. The patch I proposed puts that decision with the override event listener and only the global overrides decide to "apply globally" in the patch. The domain overrides could then decide in contrib to apply globally (except when truly no overrides apply in internal config operations) or not.

Gábor Hojtsy’s picture

Issue tags: -Security, -D8MI, -language-config

#19: global-overrides-forced-19.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Security, +D8MI, +language-config

The last submitted patch, global-overrides-forced-19.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
11.51 KB
622 bytes

It helps if I follow the new interface definition in implementations :)

Also as for why the overrides creeping in to forms and therefore current config is a problem, if you have eg. domain module, depending on which domain you save your admin form, the config saved will be different, if the domain conditional config override gets into the form. That will be confusing when you stage / update site config from development, since your live site will show changes while it just flip-flops between different domain based values. The config diff / staging system as well as overrides were built with the idea that the saved configuration is the canonical value and overrides are not used as "even more canonical".

Gábor Hojtsy’s picture

Anybody still interested in resolving this? It has been raised to critical, a solution has been proposed and then no feedback for over two weeks. If that is as much as we care, then I don't see how this would be critical?

tstoeckler’s picture

I'm not sure how entirely, but I think #1932336: Use override free configuration context for SystemConfigFormBase is related to this.

Gábor Hojtsy’s picture

Yeah, that would also need to be modified to the new context introduced here. I don't think it makes sense to keep updating the patch if nobody cares though.

Gábor Hojtsy’s picture

Priority: Critical » Normal

Sounds to me like if people care so much as to not review the proposed patch for a month, then this should not be anywhere near critical.

Anonymous’s picture

Issue tags: +Configuration system

adding tag.

Gábor Hojtsy’s picture

Hah, heh. Any opinions? :D

Gábor Hojtsy’s picture

Priority: Normal » Critical

Talking to people in Portland, they thought this is still very important, they just did not have time to think about this. So this can just as well be back to critical.

Anonymous’s picture

i've tried to think of something useful to say about this issue and patch a couple of times, but never quite managed it.

implementation details aside, i'm ok with values set in settings.php being special, so i think i'm ok with the patch in #24. the context system is already insanely hard to reason about, so i don't think another layer of special casing makes much difference, and seems to solve a real use case or ten.

honestly, i'd still rather we rip out the context system, and bake in special handling for settings.php and locale. that would get us more features than D7 (first class locale support), without making it insane.

mtift’s picture

Issue tags: +Needs reroll

I can no longer duplicate the behavior outlined in #18. However, the idea of making global overrides stick makes sense to me.

It looks like this patch needs a re-roll now that CoreBundle.php has been replaced by CoreServiceProvider.php in #1988508: Stop pretending we support Symfony bundles when we really just have service providers.

mtift’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

Ec1ipsis’s picture

Assigned: Unassigned » Ec1ipsis
Issue tags: -Needs reroll
Ec1ipsis’s picture

Assigned: Ec1ipsis » Unassigned
FileSize
4.25 KB

Per my review with @YesCT, this patch is likely in need of a complete rewrite. Maybe @Cottser can help?

There are three files with issues: core/modules/system/system.admin.inc, core/modules/user/user.admin.inc, and core/lib/Drupal/Core/CoreBundle.php. User.admin.inc in particular had ~1,700 lines deleted from it, and as was mentioned above CoreBundle.php was completely removed.

We attempted to find the issue that deleted CoreBundle.php with the command: git log -S'class CoreBundle extends Bundle'. The intention was to find the commit where that particular line was removed, which would tell us when the file was deleted. The result was:

commit b1c684b8f82e103764422acd95cab332dd7904ff
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Fri Jun 28 17:11:57 2013 +0100

    Issue #1988508 by katbailey: Stop pretending we support Symfony bundles when we really just have service providers.

commit e665805ccee0c28c21d613c87ff0d2c23f9f4b7c
Author: Katherine Bailey <katherine@katbailey.net>
Date:   Wed Jul 11 16:47:37 2012 -0700

    Various coding standards fixes and other minor changes in response to Crell's latest patch review

Unfortunately, this didn't provide the information we were after.

The attached patch is the result of applying #comment-7189198 and removing all offending code. File size of new patch has been reduced roughly 60% due to changes being removed during the process.

moshe weitzman’s picture

Anyone able and willing to take this on?

moshe weitzman’s picture

Anyone able and willing to take this on?

Gábor Hojtsy’s picture

Status: Needs review » Postponed

Moshe: I think we should discuss first and foremost what we want. Not many people cared to comment on my patch :/ Also #2098119: Replace config context system with baked-in locale support and single-event based overrides is about to change the config context/override system drastically so not sure there is a point in working on this one *now* until that is committed.

DamienMcKenna’s picture

Status: Postponed » Needs review
FileSize
0 bytes

A minor reroll.

DamienMcKenna’s picture

Letmetrythatagain.

DamienMcKenna’s picture

Status: Needs review » Postponed

Putting it back on the backlog per #38.

Anonymous’s picture

bump.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Postponed » Needs work
mgifford’s picture

This no longer applies....

$ git apply drupal-n1934152-39-conf_ftw.patch
error: patch failed: core/lib/Drupal/Core/Config/Config.php:275
error: core/lib/Drupal/Core/Config/Config.php: patch does not apply
error: core/lib/Drupal/Core/Config/Context/ConfigContext.php: No such file or directory
error: core/lib/Drupal/Core/Config/Context/ContextInterface.php: No such file or directory
error: core/lib/Drupal/Core/Config/Context/FreeConfigContext.php: No such file or directory
error: core/lib/Drupal/Core/EventSubscriber/ConfigGlobalOverrideSubscriber.php: No such file or directory

Berdir’s picture

Is this still an issue given that the context stuff is gone and this is always applied last in ConfigFactory?

alexpott’s picture

@Berdir The question that remains is: should ConfigFactory::setOverrideState(FALSE) stop overrides in settings.php from applying? People have raised this as a security issue.

Gábor Hojtsy’s picture

Right. I have not managed to get the security team to weigh on this though :/

webchick’s picture

chx’s picture

Now that context is gone the IS definitely needs upgrade. As there doesn't seem to have anyone else, I will try to follow up with security considerations (someone must, after all) once that's done.

Gábor Hojtsy’s picture

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

Title: Figure out override priorities and whether we want global overrides to stick (settings.php overrides don't work on all pages) » Figure out if we want global config overrides to stick (settings.php overrides don't work on all pages)

Updated the issue summary! The situation now in short is that global overrides always trump every other override, see ConfigFactory::get(), but no override is applied if in override-free mode, such as in admin forms, see ConfigFactory::setOverrideState(). So this later situation is in question in this issue.

  public function setOverrideState($state) {
    $this->useOverrides = $state;
    return $this;
  }

  public function get($name) {
    if ($config = $this->loadMultiple(array($name))) {
      return $config[$name];
    }
    else {
      $cache_key = $this->getCacheKey($name);
      // If the config object has been deleted it will already exist in the
      // cache but self::loadMultiple does not return such objects.
      // @todo Explore making ConfigFactory a listener to the config.delete
      //   event to reset the static cache when this occurs.
      if (!isset($this->cache[$cache_key])) {
        // If the configuration object does not exist in the configuration
        // storage or static cache create a new object and add it to the static
        // cache.
        $this->cache[$cache_key] = new Config($name, $this->storage, $this->eventDispatcher, $this->typedConfigManager);

        if ($this->useOverrides) {
          // Get and apply any overrides.
          $overrides = $this->loadOverrides(array($name));
          if (isset($overrides[$name])) {
            $this->cache[$cache_key]->setModuleOverride($overrides[$name]);
          }
          // Apply any settings.php overrides.
          if (isset($GLOBALS['config'][$name])) {
            $this->cache[$cache_key]->setSettingsOverride($GLOBALS['config'][$name]);
          }
        }
      }
      return $this->cache[$cache_key];
    }
  }
Gábor Hojtsy’s picture

chx’s picture

Status: Needs work » Active

Yes, settings.php overrides must win. I am not sure how is this even a question but having access to settings.php is usually rarer than having access to the UI so it should win.

We discussed the UI half with timplunkett and the conclusion I arrived to is as follows:

  1. The overrides should not get into CMI because a development override shouldn't get into the export to bleed into production.
  2. However, this leads to situations where you change a value in the UI and it doesn't take.
  3. The desired resolution for this, in my opinion, is to show the overridden value in place of the edit element and in the description add "This value is overridden, the original value is X click here to edit it. The change will be saved into the database and can be exported but it won't be visible as long as it is overridden." and then the user clearly knows why his edit won't take.
Gábor Hojtsy’s picture

Yeah that as an idea is great (to show if a value is overridden) *however* configuration does not have a consistent formbuilder API at all for anything outside the form class itself to inject generic messages and/or disable elements. Configuration does not even use standardized widgets so we can tell a widget to disable itself (eg. parts of the views UI or a block configuration or whatnot). So long as each form needs to deal with any part of it needing to be disabled and print that message, it becomes a huge pain.

Finally, even if all forms would deal with that and display such message, it may or may not be true. There is no indication as to whether the global override is static or its dynamic, ie. it may or may not apply at certain cases and the db stored value may or may not apply then with appropriate conditions. Eg. if config is overridden for anonymous users, the admin will never see the override on the UI and the changes will be possible to be saved, but not taken for anonymous users. Same if the config is overriden for authenticated only, the config cannot be changed in your scheme even if it would change how anonymous behaves.

One of the things pointed out in the issue summary is that global settings overrides are used for all kinds of fun things, eg. site sections / context dependent settings which comes with various conditions.

In short I don't think we can do anything about this on the UI since there is no control over how the UIs are created in the first place. AFAIS the only question is whether we apply those overrides for the UIs too, which seems to be people saying NO, since it would save back to actual config and with the assumption that the global settings overrides are environment specific (another interesting assumption about what people use their global overrides for :D), we don't want them to end up in that config.

chx’s picture

Sure, skip the UI parts, I just didn't want to confuse people by showing one value in the UI and then editing it not taking but then again I presume settings overrides will be rare and understood.

Gábor Hojtsy’s picture

@chx: well, the state of play now is that overrides are not applied to config when taken for forms following #2251915: Overridden config entity bleeds through to admin forms and #2239065: Overridden config bleeding through to configuration forms. From one point of view that is good because the overrides will not appear in the forms and therefore not saved back to config storage. That is what @webchick opened #2239065: Overridden config bleeding through to configuration forms as a critical for.

OTOH now the entity loaded in upcasting in admin contexts and all config loaded on forms is override-free and therefore if you wanted to apply overrides to eg. configuration of how the form is built or how the form acts when submitted that is impossible to do now because the overrides don't apply. Eg. if the admin form sends an email when changed and the email address is overridden in the local context so the live monitoring is not spammed, this will not work because admin forms don't apply overrides you know.

This issue was opened for security considerations. I don't have a good example, but surely there are people who are more creative than I am :D

alexpott’s picture

eg. configuration of how the form is built or how the form acts when submitted that is impossible to do now because the overrides don't apply. Eg. if the admin form sends an email when changed and the email address is overridden in the local context so the live monitoring is not spammed, this will not work because admin forms don't apply overrides you know.

Hmmm... we should only be using override-free configuration for what is displayed & configured using the form - not for how form acts.

Gábor Hojtsy’s picture

Hmmm... we should only be using override-free configuration for what is displayed & configured using the form - not for how form acts.

Yeah that sounds good as a principle. However:

1. In practice since we have all that code doing $this->config(), it is very easy to use $this->config() for whatever builds the form or reacts to the submission as well.
2. OTOH any API calls to get data for forms (ie. not using $this->config() to get data but some API) may get us overridden config, since our override state altering is entirely encapsulated in ConfigFormBase::config().

So forms may still get overridden config where they did not want (2) and they would naturally use $this->config() for auxiliary config access even for things where overrides should be applied (1). I think the distinction is where overrides should be applied is a big enough can of worms that the multiple use cases of global overrides just complicates more.

So looks like it would be down to developers understanding the whole override mess for them to make informed choices, no? We can make parts of the API easier to identify where they belong. Eg. we can rename ConfigFormBase::config() to rawConfig() or something along those lines? Not sure how to make it apparent that admin upcast entities are also raw but all other API access will not yield raw config and that for behavioural code in the form one should not use raw config.

alexpott’s picture

+1 to making it more explicit through renaming the method - great idea.

FormBase implements config() too - so I think the ConfigFormBase should actually throw an exception if that is called. And then we can have two methods, not sure of their names... ConfigForEditing() and ConfigForUsing() so that the caller knows the explicit choices they are making.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
56.74 KB

Here is a draft for what that may look like with rawConfig() and liveConfig() as method names. I just did a quick phpstorm refactor so there may be false positives or missing pieces. Just posting to keep the discussion going. The substantial change is in ConfigFormBase.php

Gábor Hojtsy’s picture

Any feedback on this approach to make the override/no override situation clearer in config admin forms?

catch’s picture

Not 100% on the method names, but having two explicitly named methods seems good.

Should 'raw' be 'stored' maybe?

Gábor Hojtsy’s picture

Config also has

  /**
   * Gets the raw data without overrides.
   *
   * @return array
   *   The raw data.
   */
  public function getRawData() {
    return $this->data;
  }

it also has getOriginal() which is the (not yet modified from) stored version of the config with or without overrides :) So we should somehow look for terminology harmony here and if we want to introduce something else here, then apply it to elsewhere where it makes sense.

YesCT’s picture

when will we use liveConfig()?
it is not used in the patch.
and I'm not clear from the docs.

yeah, consistant names through Config StorableConfigBase and ConfigBase would be helpful. If we can agree on an approach here, we could do renaming across the classes in a separate issue.

Gábor Hojtsy’s picture

liveConfig() would be used on any config that is read to construct the form and/or to react to the form being submitted outside of saving the config itself. That is if you are editing config A, you may use config B to construct the form or react to saving it. I'm not sure replacing config() methods on base classes with liveConfig() for consistency is a good idea in and of itself though.

YesCT’s picture

I was thinking more along the lines of consistent use use of: raw (stored) original live data
I suppose I could clarify by actually making such an issue, and if we decide not to, wont fix it.

Gábor Hojtsy’s picture

@catch: what do you think in light of the mentioned other existing methods?

star-szr’s picture

FileSize
54.04 KB

Rerolled for PSR-4. One small context conflict in \Drupal\Core\Installer\Form\SiteConfigureForm.

Status: Needs review » Needs work

The last submitted patch, 68: 1934152-68-psr4.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll

sadly...

chx’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
54.01 KB

One of those patches where using git to reroll is significantly easier: the patch applied on top of d0d3e533615df7539a54cac995b50e75f18d1597 then I merged on top of HEAD and there was a single, easy to fix conflict.

Status: Needs review » Needs work

The last submitted patch, 72: 1934152_72.patch, failed testing.

Status: Needs work » Needs review

Gábor Hojtsy queued 72: 1934152_72.patch for re-testing.

Gábor Hojtsy’s picture

Thanks @chx for rerolling. This is still down to needing feedback.

Gábor Hojtsy’s picture

Issue tags: +sprint
YesCT’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

maybe formstate interface conflicts... july was a long time ago.

catch’s picture

Issue tags: +D8 upgrade path

Tagging with D8 upgrade path since we're aiming to fix all known security issues before supporting that.

martin107’s picture

Assigned: Unassigned » martin107
Status: Needs work » Needs review
FileSize
53.64 KB

The age of the patch, plus the number of changes I made, make me think I will not get it all completely correct first go

#77 is mostly right ... lots of form state changes....

I mades note .... MethodForm.php and NegotiationConfigureForm.php are files where the code had changed so much that I could just go with the changes from HEAD.

Lets see what testbot says.

I will lurk on this issue until its green:)

Status: Needs review » Needs work

The last submitted patch, 79: 1934152_79.patch, failed testing.

martin107’s picture

Assigned: martin107 » Unassigned

It looks like duplicates of what was defined in FormBase

public function config()
public function rawConfig()
public function liveConfig()

will now need to be defined as trait, which can also be used in \Drupal\Core\Entity\EntityForm.

that way lines that involve $this->rawConfig() will work in things that extend EntityForm and things that extend ContentEntityForm.

For example look at the test failure for

\Drupal\contact\ContactFormEditForm. ( which extends EntityForm )
\Drupal\comment\CommentForm ( which extends ContentEntityForm )

This is a critical issue and needs input from system architects before I proceed :(

So after putting on my flame resistant suit t can I start the naming debate by suggesting

ConfigHelperTrait

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
53.48 KB

Your patch contained unrelated stuff, like that MenuSettingsForm. I redid the patch from scratch with the following:

- same changes in ConfigFormBase and FormBase
- phpstorm refactor to rename config() to rawConfig()
- I removed the config() from ConfigFormBase since it was not in use anymore

The goal of this issue to let config edit forms access raw config by default. So long as you are not **editing** your config, the primary use case is you want to access it with all overrides applied, in fact that distinction was the goal of this issue. So I am not sure why would content entity forms need this at all? They don't currently have a config() method and if they would have one, they would expect to get values with overrides (they are not editing that config).

Gábor Hojtsy’s picture

Note that 82 also renames the config() method to rawConfig() in FormBase, which is silly, that is a side effect of the overzealous phpstorm refactor tool :) We should not do that of course. Here is another more intelligently ran refactor that keeps config() on FormBase intact. No interdiff because I recreated the refactor with phpstorm again. The actual changes are only in ConfigFormBase the others are refactor (search and replace).

Gábor Hojtsy’s picture

Duh, given the parent is not rawConfig() anymore, we should call the right method. Also minor docs fix.

chx’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll +Needs tests
Gábor Hojtsy’s picture

Status: Needs work » Needs review

I don't think its a smart idea to work on more code for this unless we even agree we want something like this. Sounds like an easy waste of time for someone. The tests will not make the approach any easier to review or provide feedback on. We still need feedback on this. I don't contest that it needs tests once we agree on even doing this and doing it this way. Therefore moving back to needs review for feedback.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
    @@ -61,12 +61,23 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +    throw new \Exception('Calling config() directly in configuration forms is forbidden. Call rawConfig() or liveConfig() instead.');
    +  }
    

    I kinda like semantic exceptions ... have you thought about using BadMethodCallException instead?

  2. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
    @@ -61,12 +61,23 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +  protected function rawConfig($name) {
    
    @@ -75,4 +86,22 @@ protected function config($name) {
    +  protected function liveConfig($name) {
    +    return parent::config($name);
    +  }
    +
    

    The only question is why we not just have liveConfig() and keep config() as it is. I would assume that the common usecase for settings forms is exactly the functionality of rawConfig?
    Comment #1934152-58: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects and #1934152-59: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects seem to be the key points here. We want people to actively make the decision. Did we considered to have rawConfig() as
    well as overriddenConfig()(maybe this tells a bit more about what is going on), but just an idea ...

Gábor Hojtsy’s picture

@dawehner: Thanks for the feedback. I definitely agree a more semantic exception is in order. I will work on that as soon as there is some agreement we want this kind of change to happen :) You also identified the reasons for the two new methods very well. I think overriddenConfig() may be a bit confusing, as we don't really know if there are any overrides, there may be... on an English only site that does not use domain access, og, etc. there will not be anything that may even override config. So that was the reason for liveConfig(). But I don't have very strong feelings on that, overriddenConfig() is fine if others believe that is fine too :)

benjy’s picture

I really think having global overrides from settings.php is important. We use it for all kinds of things such as disabling cache, css/js aggregation locally, API keys on live etc.

Didn't Acquia just use this global override approach to switch out the DB abstraction for the recent security issue? Yes you might do that differently in D8 but I think that shows that a way to trump all other configuration from the settings file is very handy in many cases.

Gábor Hojtsy’s picture

@benjy: right, this patch does not make them applied less, it just makes it obvious when overrides are used vs. when they are not, so people can consciously use what they need to. It is important that when you edit config, you can edit the original values and that experience is not obscured by overrides (for domain, language, etc). However for anything else on the page, such as eg. labels of the forms for editing the config should apply overrides. Eg. if you are editing a view in French, the UI should be French (eg. labels of fields) but if the view is originally English, you should edit the English view not a (partial) translation.

Prior to this patch it is easy to just use config() and depending on whether your class was extending ConfigFormBase or not, it would behave VERY differently. The goal of this patch is to make that behaviour difference apparent.

tim.plunkett’s picture

Is this still in the "figure out if" stage? Perhaps this could be discussed and decided in Ghent?

xjm’s picture

@tim.plunkett, yep, it's going to be discussed today. See: https://groups.drupal.org/node/451513

Gábor Hojtsy’s picture

Title: Figure out if we want global config overrides to stick (settings.php overrides don't work on all pages) » FormBase::config() and ConfigFormBase::config() work entirely differently
Issue summary: View changes
Related issues: +#2136559: Config entity admin lists show configuration with overrides (eg. localized)
Gábor Hojtsy’s picture

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

Title: FormBase::config() and ConfigFormBase::config() work entirely differently » FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects
Gábor Hojtsy’s picture

Status: Needs review » Postponed

Postponing on #2392319: Config objects (but not config entities) should by default be immutable based on discussion in Ghent. That would provide a systemic solution that also applies if you use FormBase to save back config (eg. you don't know about ConfigFormBase) or for things like config entity list controllers that set weights which have the same problem

xjm’s picture

Issue tags: +Ghent DA sprint

(For the issue summary update and hard problems discussion.)

Gábor Hojtsy’s picture

Issue tags: -sprint
Gábor Hojtsy’s picture

Issue tags: -sprint
alexpott’s picture

#2392319: Config objects (but not config entities) should by default be immutable will probably solve this issue - if it does I will mark this issue as a dupe. OTOH, if this does not play out that way then we will need to triage this issue.

Wim Leers’s picture

Title: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects » [PP-1] FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects
Gábor Hojtsy’s picture

Status: Postponed » Closed (duplicate)

#2392319: Config objects (but not config entities) should by default be immutable resolved this in a different way. Config forms config() will work appropriately for config names if they are listed in the editable config method or not.

Gábor Hojtsy’s picture

Title: [PP-1] FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects » FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects
xjm’s picture

sime’s picture