Problem/Motivation

As per reviews by @webchick and discussion between @alexpott, @xjm and @effulgentsia, it may be a problem that configuration forms work totally opposite in terms of overridden values compared to how they worked in Drupal 7 and before. In Drupal 7 if you had a settings.php configuration override for example, that got used to build the configuration form and whatever you changed, the change did not stick. That was a WTF, but at least it was some sort of feedback on the value not being possible to change. In Drupal 8 however, overrides are not used to build forms and you can actually change settings and they would actually be saved and then displayed again on the form as if they were persisted. They are actually persisted but overrides may still apply on them, so the operation may be moot.

Proposed resolution

Following #2392319: Config objects (but not config entities) should by default be immutable each configuration form knows about the configuration keys edited and centralizes an accessor ($this->config()) to access them. Thanks to this, we can track if there were any configuration details loaded that may have overrides applied (by loading the editable version as well and comparing the data). That way, we can detect if there are overridden values at least under the same conditions as the admin form was displayed (eg. static global overrides). Using that information, we can display a message on top of the form if there is overridden configuration and editing is pointless.

Remaining tasks

Implement. Add tests. Review. Commit.

User interface changes

Configuration forms may display a message on the top if there are overridden values.

API changes

None.

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
1.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,941 pass(es). View
115.4 KB

Here is a quick patch. Not sure how to do this better. I used the following text:

This form may include elements which have overridden values in your current context. Editing those values will save changes but will only be used if overrides don't apply.

The reasons are the following:

This form (1) may include elements which have overridden values (2) in your current context. Editing those values will save changes but will only be used (3) if overrides don't apply.

1. We don't really know which elements of said config object are ACTUALLY used to build this form. So for example AccountSettingsForm uses system.site to store the notification address. If the site name is overridden in settings.php, therefore the account settings page will say there are overridden values, but those are not actually used to generate the form. To be able to intercept actual keys used, we would need to wrap the get() on these objects or add listeners to getters. Neither sounds like a good idea.

2. We don't really know why are those overrides there. They may be permanent settings.php overrides, or because you visited this config form on a domain or a language. We only have the vague idea that in the context of this page at the time of the form generation those overrides happened to apply.

3. Finally, we don't really know if there are other overrides that may still apply. Eg. the developer removes the site_name override, but it may ALSO be overridden by group or domain or language. So we cannot say that the original value will be used if the overrides that are CURRENTLY applied are not applied because we don't know if there are other overrides to prevail in that case.

We may chose to ignore some of these facts to make the message look clearer but then it may be lying under some cases.

The visual display currently is as follows:

Of course by design none of the form elements actually show that I had an override or what that override was, that is largely left to guesses. We could display the config names, but that is not very user friendly. We could use typed config manager to get the top level descriptions for said config names and display those, but (see (1)) that would look really odd on eg. the account settings page. It would say you are editing the site information on the account settings page. Yeah.

Gábor Hojtsy’s picture

Just realizing now that we should also incorporate in the text somehow that those overrides are not actually visible / used in the form.

effulgentsia’s picture

To be able to intercept actual keys used, we would need to wrap the get() on these objects or add listeners to getters. Neither sounds like a good idea.

Actually, I think it would be a great idea for ConfigFormBaseTrait::config() to add a get() listener on the object returned by getEditable(). Not sure if our config API supports adding per-object listeners like that, but would be cool.

Gábor Hojtsy’s picture

So get() does not even have an event. Save, delete and rename have events. Get is so common to fire an event at that point sounds like a major performance drain?

alexpott’s picture

Issue tags: +Configuration system

Tagging...

jhedstrom’s picture

FileSize
1.47 KB
2.48 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,260 pass(es). View

This is the same as #1 with a test added.

So get() does not even have an event. Save, delete and rename have events. Get is so common to fire an event at that point sounds like a major performance drain?

This does seem like it would certainly impact performance.

xjm’s picture

anavarre’s picture

Moshe pointed me to that issue today. Really confusing to never (quickly) know if your conf override is being picked up or not. You're always wondering if you've implemented the override successfully or not and then need to spend time to confirm/deny while you should have an easy way to determine this.

Fortunately, Drush can help today:

$ drush @site.env cget system.file
allow_insecure_uploads: false
default_scheme: public
path:
  temporary: /tmp
temporary_maximum_age: 21600

$ drush @site.env cget system.file --include-overridden
allow_insecure_uploads: false
default_scheme: public
path:
  temporary: /mnt/tmp/
temporary_maximum_age: 21600
Gábor Hojtsy’s picture

@anavarre: which overrides would that drush command consider? language? domain? time? is-it-pirate-day?

anavarre’s picture

Gábor Hojtsy’s picture

@anavarre: there may be any number of overrides (collections) in active storage some of which may apply under specific conditions.

anavarre’s picture

Isn't Drush going to list all overrides for a specific config no matter what? Or are there specific situations in which you think it doesn't make sense or it's simply not doable?

Gábor Hojtsy’s picture

@anavarre: the example you provided, it did not list all overrides, it merely presented a variant of the config with some (undisclosed) overrides applied. There may be conflicting overrides with different values for path.temporary in more than one override.

anavarre’s picture

There may be conflicting overrides with different values for path.temporary in more than one override.

That's actually a fair concern though I *think* what Drush takes into account is the last override for a given value. E.g. if you have an override in settings.local.php and one at the very last line of settings.php it will take into account the later. I'm not sure it should behave any differently. Should it? If yes I can file an issue against the Drush project.

Gábor Hojtsy’s picture

@anavarre: right, what I was getting at is that you two drush commands returning the same config does not mean there are no overrides under other circumstances (time of day, role of user, organic group being viewed, if there is pirate day, etc). Also they returning different values does not indicate that only those values that are different at the time may be different under other circumstances.

anavarre’s picture

Filed https://github.com/drush-ops/drush/issues/1710 accordingly

EDIT: not much we can do it seems. Issue closed.

swentel’s picture

Ok, this has been bugging me for a while now as well, having a warning is actually pretty great imo.

Also, are new strings for 8.1 ?

+++ b/core/modules/config/src/Tests/ConfigFormOverrideTest.php
@@ -23,6 +23,11 @@ class ConfigFormOverrideTest extends WebTestBase {
+    // Ensure warning is not present if no overrides exist.
+    $this->drupalGet('admin/config/system/site-information');
+    $this->assertNoRaw("This form may include elements which have overridden values in your current context. Editing those values will save changes but will only be used if overrides don't apply.");
+
+
     $overridden_name = 'Site name global conf override';

extreme nitpick - one line to many

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Going to be bold here and setting to RTBC, want to know what committers think of it :)

dasjo’s picture

Also see #2659602: ConfigOverrides are not shown to the user in config forms which does the same thing and also informs about which config key/value pairs are overridden.

Ideally, the form elements would use identifiers that allow to map which config is being used to mark those form elements individually but I guess that all can go into a follow-up improvement task :)

Gábor Hojtsy’s picture

I am still not sure that we need/should have this. It may give the false feeling that people see the warning when overrides may be applied to something while it only gives the warning if an override IS applied on the admin page in question. Language, domain, etc. overrides will still happily apply, and your editing the values may still be in vain even if the warning never displays. So its far too detached from real site use cases AFAIS. Nonetheless it does display warnings in some very limited cases. If we need/want that and are not concerned for misleading users with it, then it may be helpful.

Gábor Hojtsy’s picture

@dasjo: there is no guarantee whatsoever that form key ids use the same name as config IDs, consider multiscreen forms for example. Also there is no gurantee whatsoever that a form actually uses all the keys from a config file, there are plenty examples to that in core.

alexpott’s picture

I;m in agreement with @Gábor Hojtsy here - we can only do this once the forms are built automatically from schema and config - kinda like the old system_settings_form() just better.

Berdir’s picture

The problem is that language/domain/... overrides are very different from $config/settings.php overrides, which always apply.

Yes, for language overrides, it doesn't really make sense to show that warning. And it would also vary depending on the language you're looking at (if you edit with current language german when original is english, it will show the warning but not if you edit with english).

We might need a better way to detect overrides and limit this to settings.php overrides?

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Clearly still being discussed.

swentel’s picture

So what if we add a setting to settings.local.php, someting like

$settings['show_overrides_warning'] = TRUE;

and wrap the message text to only show it when this is TRUE (and if there's an override of course), plus a big docblock for the setting that this is experimental.

Simply having an indication is valuable (to me at least), especially during development or when setting up an environment for a testbot. Ususally, the things I'm overriding are dead simple configs, but can have impact: API keys, service urls, choice of mail transport, etc which should be disabled on dev/testbot..

Berdir’s picture

Yes, but even when you have that set, you'll get pointless messages if you use config translations and are accessing that page in a different than the original language of that config entity.

Maybe we can just explicitly check for $GLOBALS['config'][$name], which is what ConfigFactory is doing, and only display this message for that?

swentel’s picture

Marked #2661050: Config overrides doesn't get reflected in the forms based on ConfigFormBase as a duplicate.

Will try to check with $GLOBALS['config'][$name] tomorrow.

swentel’s picture

so something like this then ?

alexpott’s picture

+++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
@@ -58,6 +58,11 @@ protected function config($name) {
+        drupal_set_message(t("This form may include elements which have overridden values in your current context. Editing those values will save changes but will only be used if overrides don't apply."), 'warning');

+++ b/core/modules/config/src/Tests/ConfigFormOverrideTest.php
@@ -23,6 +23,10 @@ class ConfigFormOverrideTest extends WebTestBase {
+    $this->assertNoRaw("This form may include elements which have overridden values in your current context. Editing those values will save changes but will only be used if overrides don't apply.");

@@ -39,6 +43,9 @@ public function testFormsWithOverrides() {
+    $this->assertRaw("This form may include elements which have overridden values in your current context. Editing those values will save changes but will only be used if overrides don't apply.");

I think we now need to say that the overrides are from settings.php.

mgifford’s picture

So something like "This form may include elements which have overridden values in your current context. Editing those values will save changes but will only be used if overrides from the settings.php file don't apply."

Makes me wonder when they wouldn't apply?

Gábor Hojtsy’s picture

@mgifford: Here is a settings.php snippet for you:

if (date('m.d') == '09.16') {
  $conf['system.site']['site.name'] = 'Pirate day!';
}

Of course less contrived examples are possible :)

dasjo’s picture

@Gábor Hojtsy ad #22 you are right, thinking of a way to map form elements back to the config elements being used - could be a data attribute for example.

mgifford’s picture

@Gábor Hojtsy A hearty thanks fer that excellent example!

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
@@ -58,6 +58,11 @@ protected function config($name) {
+        drupal_set_message(t("This form may include elements which have overridden values in your current context. Editing those values will save changes but will only be used if overrides don't apply."), 'warning');

Not sure what "in your current context" really means. I think it is worth just saying that settings.php may contain overrides rather than introducing new language to explain.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review
FileSize
2.37 KB
2.28 KB

Maybe as this is UI text referring to settings.php would be a bad idea. So perhaps we should use less words to say the same thing. I pondered about saying "global overrides" but we still have the problem of what are they?

It'd be great to get some usability feedback on this.

Gábor Hojtsy’s picture

Well, they are settings.php overrides, I am not sure making up some "friendly term" will help. While it may be friendly, it will be unclear.

alexpott’s picture

How about this then

Gábor Hojtsy’s picture

Sounds better to me. Will leave it to others to decide if this is useful for them or not. The text is still sufficiently unclear if there is any element even displayed on the page which may be overridden and then which one, because its hard/impossible to tell that, so I am still not convinced of the usefulness in general. Referring to others for judging that :)

alexpott’s picture

@Gábor Hojtsy well someone at Chapter Three who experienced the current behaviour was very very confused and asked why is there no warning.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
@@ -55,6 +55,11 @@ protected function config($name) {
+        drupal_set_message(t('This form may include elements whose values are overridden using settings.php. Editing those values will save changes but will only be used if the overrides do not apply.'), 'warning');

Should we tell people which elements are overridden? We could even extract a human readable name optionally when we leverage config schema.

Gábor Hojtsy’s picture

@dawehner: will those elements appear on the page? Eg. on the views UI or the admin emails configuration page there are lots of things not displayed but even though in the config file.

dawehner’s picture

@Gábor Hojtsy
Good question. I agree, it could be really a long list. I was more thinking about the probably common usecase of just overriding just a few of them.

minnur’s picture

I spent some time trying to understand why Drupal wasn't picking up the variable. A warning message is a very important improvement! Would it also make sense to display more details about overridden variables on the status page: admin/reports/status and have a link to the page in the warning message ?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

yoroy’s picture

Not sure I understand the details of this discussion but slightly shorter rewrite of the second sentence:

This form may include elements whose values are overridden by settings.php
You can change these values but they will only be used if the overrides do not apply.

"May include" is uncomfortable because it's unhelpful help; only vaguely pointing in a general direction instead of of spelling out the specifics. If we can show which ones are overridden then we probably should. Still, there's anectdotal evidence in this thread that says even this general message at least gives a concrete clue as to where to look for understanding why things (don't) happen.

The suggestion by @minnur of linking to the list in the status report sounds potentially sensible. Or even on it's own reports page? Not sure…

Gábor Hojtsy’s picture

@yoroy: yeah the technical challenge is that a config file is used for the form does not mean that all its entries are used (views UI is most trivial but other screens like user settings do the same). There is also no necessary correlation between keys in the config file and keys in the config form (also again eg. in views UI), so we cannot tell if the values that *are* overridden in config being edited appear in the form at all or not.

zviryatko’s picture

I've added verification, is the configuration is overridden in settings.php, to smtp module, see #2731283. It will be a huge work to add the same to all core configurations, but maybe there is a more fastest way.

pazhyn’s picture

I do like the solution made in SMTP module, mentioned in #48

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Elijah Lynn’s picture

Posting Schnitzel's patch from #2659602: ConfigOverrides are not shown to the user in config forms that was closed as a duplicate. It has a similar message but also tells which overrides are present and their values. Works as is for 8.2.3 but does need a re-roll, and the wording could be improved too.

https://www.drupal.org/files/issues/config_override_message-1.patch

I would like to see this functionality have an optional dependency on the inline form errors module. If IFE module is enabled then it will highlight the fields inline, progressive enhancement.

Elijah Lynn’s picture

Also, I am half tempted to mark this as critical (but I know it isn't). From a DX (developer experience) I can't say enough how frustrating this was to not have the D7 behavior or better just be there. I spent an hour trying to figure this out before I found this issue. This has probably happened to thousands of developers already. This is a high priority.

Status: Needs review » Needs work

The last submitted patch, 51: config_override_message-1.patch, failed testing.

ohthehugemanatee’s picture

Yep, definitely a huge PITA for DX. If IEF wants to elaborate on the notice, that's up to IEF. Even acknowledging that there are overridden values would be a big improvement over the status quo though.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joshua.boltz’s picture

The patch in #51 is working great.
If in the site's settings.php file, you have a line such as:
$config['system.site']['name'] = 'Drupal 8 DEVELOPMENT';

That value is not displayed back in the form, so this patch adds a nice UX improvement in the admin UI to inform the user that the value shown in the form field is overridden elsewhere and is not the one actually being used.

This is mostly an enhancement for admin users in the UI, as developers can use a drush command on the server to check the overridden values by passing the --include-overridden flag to config-get:
drush config-get my_module.settings form_field --include-overridden

Posting screenshot so others can see an example of the end result of this patch and what it provides.

Elijah Lynn’s picture

Embedding #56 for easier viewing.

UI Message from #51 patch

DamienMcKenna’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
5.55 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 58: drupal-n2408549-58.patch, failed testing.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
5.63 KB
4.87 KB

OK, this should fix the tests.

Did another couple of small fixes in typos and coding standards (interdiff attached).

Another 2 questions:
- I don't really like that we are creating markup like this. How about creating a template instead?
- How about creating a new permission, and only displaying this message if the user has access to see overrides?

eiriksm’s picture

Issue tags: +DevDaysSeville