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

Use #config_target property to determine config keys which are editable in a form and have overrides.

Remaining tasks

Review. Commit.

User interface changes

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

API changes

None.

CommentFileSizeAuthor
#255 Screenshot 2024-03-08 151447.png12.02 KBAaronMcHale
#251 CleanShot 2024-03-01 at 14.22.36@2x.png81.5 KBalexpott
#250 CleanShot 2024-03-01 at 14.17.30@2x.png82.14 KBalexpott
#246 CleanShot 2024-02-27 at 10.02.40@2x.png248.8 KBalexpott
#245 CleanShot 2024-02-27 at 09.48.46@2x.png179.75 KBalexpott
#244 site name.png180.15 KBalexpott
#244 notification emails.png192.78 KBalexpott
#223 Screenshot 2024-02-14 at 4.29.53 PM.png149.05 KBsmustgrave
#218 Screenshot 2024-02-06 at 3.09.04 PM.png147.71 KBsmustgrave
#210 Screenshot 2023-12-15 at 2.36.59 PM.png75.17 KByash.rode
#200 Update-Manager-settings-My-Drupal-site.png231.83 KBnarendraR
#192 Screenshot 2023-11-07 at 7.01.59 PM.png165.26 KByash.rode
#186 double.jpg142.44 KBrkoller
#180 Screenshot 2023-09-04 at 5.32.30 PM.png383.59 KBWim Leers
#160 drupal-config_override-2408549-160.patch10.54 KBLiam Morland
#153 2408549-153.patch10.52 KBRatan Priya
#153 interdiff_152-153.txt965 bytesRatan Priya
#152 2408549-152.patch10.49 KBShubham Chandra
#150 2408549-150.patch12.45 KBnarendra.rajwar27
#143 interdiff_141-143.txt1.3 KBravi.shankar
#143 2408549-143.patch12.59 KBravi.shankar
#141 2408549-139.patch12.5 KBmiksan
#138 2408549-138-c.png215.58 KBsime
#138 2408549-138-b.png143.48 KBsime
#137 interdiff-133-137.txt2.61 KBHardik_Patel_12
#137 2408549-137.patch14.75 KBHardik_Patel_12
#134 interdiff-132-133.txt1.86 KBHardik_Patel_12
#134 2408549-133.patch12.84 KBHardik_Patel_12
#132 2408549-132.patch12.81 KBandypost
#132 interdiff.txt1.64 KBandypost
#131 2408549-131.patch12.79 KBandypost
#125 20181008-Configuration_Split_Warning.png137.08 KBkarolus
#106 2408549-106.patch12.71 KBjofitz
#106 interdiff-104-106.txt901 bytesjofitz
#104 there_is_no_indication-2408549-104.patch12.78 KBsnehi
#104 interdiff.txt906 bytessnehi
#99 there_is_no_indication-2408549-99.patch12.76 KBborisson_
#99 interdiff-2408549.txt1.93 KBborisson_
#97 there_is_no_indication-2408549-97.patch12.69 KByogeshmpawar
#92 form-override-message.png29 KBChi
#91 2408549-91.patch12.56 KBjofitz
#87 2408549-2-87.patch12.52 KBalexpott
#87 85-87-interdiff.txt764 bytesalexpott
#85 2408549-2-85.patch11.77 KBalexpott
#78 core-config_override_indication-2408549-78.patch8.04 KBjenlampton
#74 core-config_override_indication-2408549-74.patch7.15 KBjenlampton
#69 there_is_no_indication-2408549-69.patch7.08 KBeiriksm
#67 drupal-n2408549-67.interdiff.txt2.03 KBDamienMcKenna
#67 drupal-n2408549-67.patch5.6 KBDamienMcKenna
#60 interdiff-2408549-58-60.txt4.87 KBeiriksm
#60 there_is_no_indication-2408549-60.patch5.63 KBeiriksm
#58 drupal-n2408549-58.patch5.55 KBDamienMcKenna
#56 Screen Shot 2017-02-13 at 3.25.36 PM.png94.97 KBjoshua.boltz
#51 config_override_message-1.patch5.55 KBElijah Lynn
#38 2408549-38.patch2.33 KBalexpott
#38 36-38--interdiff.txt2.37 KBalexpott
#36 2408549-36.patch2.28 KBalexpott
#36 29-36-interdiff.txt2.37 KBalexpott
#1 2408549-message.patch1.01 KBGábor Hojtsy
#1 FormOverride.png115.4 KBGábor Hojtsy
#6 interdiff.txt1.47 KBjhedstrom
#6 config-override-warning-2408549-06.patch2.48 KBjhedstrom
#29 2408549-interdiff.txt1.69 KBswentel
#29 config-override-warning-2408549-29.patch2.33 KBswentel

Issue fork drupal-2408549

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
1.01 KB
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

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
hanoii’s picture

I have something to add. I am particularly overriding a secret from a payment processor, and I don't want that to leak to the admin, I wonder if that should be optional or something like this should be properly thought about it?

slydevil’s picture

The patch from #60 works well. I patched an 8.3 site with it no problem.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jzavrl’s picture

The patch works for me as well.

The patch does need a bit more work in terms of coding standards. Usage of single and double quotes where not necessary and usage of the inherited t() method instead of the function.

I also agree with @eiriksm about the template and permissions part. Tests would also be nice here. I'll see if I have some time to work on this a little.

colan’s picture

Status: Needs review » Needs work

Setting status based on #65.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
5.6 KB
2.03 KB

Updated per #65.

mirom’s picture

I tried to use this patch for indicating override of Interface translations directory (\Drupal::configFactory()->getEditable('locale.settings')->get('translation.path')). Problem is that buildForm is executed before form alter, which is adding this input in locale_form_system_file_system_settings_alter(). Do you have any ideas how to fix this?

eiriksm’s picture

Updated the patch with:

- Use a template.
- Add a permission.
- Use this permission.

Still needs tests though

Status: Needs review » Needs work

The last submitted patch, 69: there_is_no_indication-2408549-69.patch, failed testing. View results

frob’s picture

It looks like there has been a bunch of focus on the form side. It seems like a better solution would live on the entity side. That way it can be reused when this becomes a problem for getting/setting config via a REST endpoint.

geek-merlin’s picture

#71: Good point. THis would mean the logic should go there.
Yet i don't see how the API should look like. Some pseudo entityref fields?
Yet maybe we should finish this and make it a followup.

jenlampton’s picture

Assigned: Unassigned » jenlampton

Some feedback on the language and messaging...

This form has values that are overridden either by a module or by settings files. You can still change them and they will be saved in the config storage, but the changes will maybe not appear within the site.

* Is it necessary to add "either by a module or by settings files" here? Does this add any value if we are not telling the user exactly where the override is coming from, or are we just adding clutter to the UI?

* What is 'the config storage' and is that important? Can we say 'You can still change them and they will be saved', or maybe simply 'Your changes will be saved'.

* What does it mean that "the changes will maybe not appear within the site". Why maybe?

* Will my changes still affect the site, even if they don't appear?

* What does the bold system.site mean, and why is there a list below it?

I'm going to take a stab at cleaning up the language and adding some description text to each form element that has been overridden.

maybe we should finish this and make it a followup.

I agree we should get this in first, and then add something similar that could be used via REST.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
7.15 KB

Well, I now understand why adding a class or description text to existing form elements is so hard: the name of the form elements has no correlation to the keys in the config file, making them extremely hard to match up.

The attached patch attempts to make the messaging more direct, and adds some human language into the list of overridden values for clarity. I think this is an improvement over the current patch, but agree it would be better with support for Inline Form Errors.

jenlampton’s picture

Assigned: jenlampton » Unassigned

Status: Needs review » Needs work

The last submitted patch, 74: core-config_override_indication-2408549-74.patch, failed testing. View results

Gábor Hojtsy’s picture

@jenlampton: yeah it is not just that form element names are not necessarily reflective of config keys, it is also that not all config keys are on the forms, so there may be overrides for the config that are not on the page and maybe none of the config shown on the page has overrides, but we would still say there are overrides.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
8.04 KB

@Gábor Hojtsy I wonder if it's also possible for one form to load from more than one config file, compounding the problem even further. There could be also be more than one config key that matches a form element key. A trixy problem for sure.

Here's a new patch that should pass tests...

Gábor Hojtsy’s picture

@jenlampton:

  • The account settings page/form is a good example of multiple config files involved but all of them partially.
  • Views UI is a good example of one config file involved but the set of keys affected/used on any given form is very limited.
geek-merlin’s picture

Unlike more modern frameworks, drupal does not link form elements to the data model (here: config entity). On a quick glance, i did not find an issue for that, but it would definitely ne related (not for a "better-done-than-perfect" solution, but for one that idicates individual form elements).

frob’s picture

Is there a Drupal 8 equivalent to system_settings_form?

Seems like that might help with a 90% usecase here.

alexpott’s picture

@frob no there is no equivalent.

I agree with #80 and others that we should be linking the form elements to the data model (either config entities or simple config) as that would make it much simpler to determine what in the form is overridden and how. In an ideal world I guess we should be generating form elements from config schema but we don't have all the validation in place (see #2300677: JSON:API POST/PATCH support for fully validatable config entities) and other metadata like title and description in the config schema. Maybe what we could do is add a special property to the form element that says what config the value is derived from. Something like '#config_data_source' => 'system.site:name' and then implement a #after_build in ConfigFormBase/ConfigFormBaseTrait that finds elements implementing this and displays the overridden message. Yes this would mean that we have to add this all the config forms in core but we'll end up being able to relate the override to a specific form element which will really help when viewing something super complex like the views ui with multiple overrides.

colan’s picture

Sounds like a good idea for now to get this rolling. Can we create a separate issue for generating form elements from config schema for later (or is #2300677: JSON:API POST/PATCH support for fully validatable config entities it)?

alexpott’s picture

@colan we'd need a separate issue for that.

alexpott’s picture

Here's a start on using a #config_data_store and implementing it for the site information form. In adding tests for this to \Drupal\config\Tests\ConfigFormOverrideTest I discovered that the current test is not quite testing what it thinks it is because the final post actually fails to save. Ensuring the users used for the test have the 'link to any page' permission fixes this.

There are several tidy ups that need to be done. For example I think the markup and wording of the message need work - I'm not sure of the best way to display the overrides. Also I think linking somehow to the actual form element would be super useful.

As this is quite a different approach than #78 I've not provided an interdiff. Also I'm not actively working on this issue so if someone else wants to take the patch on that'd be great.

Status: Needs review » Needs work

The last submitted patch, 85: 2408549-2-85.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
764 bytes
12.52 KB

Fixing the test... it now has a dependency on user because of the user role check.

agentrickard’s picture

@axel.rutz -- Not to derail here, but something like what you mention was filed here -- #2677124: Allow discovery of configuration UI.

Status: Needs review » Needs work

The last submitted patch, 87: 2408549-2-87.patch, failed testing. View results

Chi’s picture

Issue tags: +Needs reroll

ConfigFormOverrideTest has been converted to browser based test.
#2870439: Convert web tests to browser tests for config module

jofitz’s picture

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

Re-rolled.

Chi’s picture

+++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
@@ -47,14 +69,68 @@ protected function config($name) {
+          'original' => $original_value,
+          'override' => $value

This won't print properly boolean values.
I propose something like this.

'original' => var_export($original_value, TRUE),
'override' => var_export($value, TRUE)

This screenshot was made after adding manually #config_data_store keys to PerformanceForm because SiteInformationForm has no any checkboxes.

Also can we turn it back to warning (yellow)?

Status: Needs review » Needs work

The last submitted patch, 91: 2408549-91.patch, failed testing. View results

Chi’s picture

Issue tags: +Needs reroll
yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
12.69 KB

Added updated patch because previous patch failed to apply on 8.5.x branch.

yogeshmpawar’s picture

Issue tags: -Needs reroll
borisson_’s picture

FileSize
1.93 KB
12.76 KB

Hidden all previous patches, made the changes request in #92. I've also finished an unfinished comment.

Status: Needs review » Needs work

The last submitted patch, 99: there_is_no_indication-2408549-99.patch, failed testing. View results

bircher’s picture

+++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
@@ -47,14 +69,68 @@ protected function config($name) {
+      if ($value !== $original_value) {

This is not a good way to know if a config value is overwritten. What if the value in settings.php is the same as the one in the config. then both $value and $original_value will be the same but the config is still overwritten.

We should probably add a method to the config object that can indicate if it is overwritten instead.
see #2923004: Add method to check if any overrides are applied to \Drupal\Core\Config\Config

colan’s picture

Status: Needs work » Postponed

That one should get in soon.

anavarre’s picture

snehi’s picture

Status: Needs work » Needs review
FileSize
906 bytes
12.78 KB

Done as mentioned in #101
Please review.

Status: Needs review » Needs work

The last submitted patch, 104: there_is_no_indication-2408549-104.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
901 bytes
12.71 KB

Correct the test failure by calling hasOverrides() on the $config variable.

Status: Needs review » Needs work

The last submitted patch, 106: 2408549-106.patch, failed testing. View results

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpi’s picture

Hi everyone, Sam152 made me aware that a pair of modules I recently released are relevant to what this issue hopes to solve:

I think at least this issue should provide core/contrib with hints with what fields map to what config, if UI is deferred.

Notably, I'm using the #config namespace in form elements. For example:

$form['site_information']['site_name'] = [
  '#type' => 'textfield',
  '#title' => t('Site name'),
  '#default_value' => $site_config->get('name'),
  '#required' => TRUE,
  '#config' => [
    'key' => 'system.site:name',
    // And also 'secret'.
    'secret' => FALSE,
  ],
];

And I'm using 'secret' key to represent secrets (passwords, auth tokens), in case these should be hidden from site administrators. Normally overridden config values arn't exposed to users, so I consider this necessary to maintain existing behaviour.

andypost’s picture

Setting mapping for each element is not very handy
Looks config translation mapping is better way to build forms for config & provide indication

minnur’s picture

Just as an idea, maybe it makes also sense to make elements overridden in settings.php disabled in the UI ?

jofitz’s picture

@minnur That's not a bad idea, but it is out of scope for this issue. I recommend you create a new issue that refers back to this one.

frob’s picture

While that isn't a bad idea, I don't think it should be done.

If a user has multiple settings.php for different environments and a setting needs to be changed on production that is overridden in the dev settings.php then the setting cannot be changed in the ui and exported and then deployed to production.

By disabling the ui it will stop the workflow.

diamondsea’s picture

Late to the party, but I'll add a few possibilities based on reading the issue so far:

  1. Messaging
    1. Add a "This field may contain overridden settings. Details" and make "Details" a link shows a hidden DIV with the names of the overridden field
    2. Better (IMHO) would be to put a style on the specific field that their setting is/would be overridden. It's less cluttersome at the top of the page and would be impactful only when users are interacting with the specific field of interest.
    3. If the source of the overriding can be identified, displaying that would be very helpful. Maybe it could be returned as part of a listener response.
  2. Form API tweaks
    1. Add #overridden to Form API that can be applied to a field, which could addClass('overridden') for styling the field in the UI and maybe ReadOnly when displayed)
    2. Add #overridden_secret to Form API that will prevent the overridden value from being displayed to the user
    3. Add a #config field where devs can add the config variable and Form API can do a configFactory->get() and see if the value matches (and set #overridden) (this streamline the isOverridden() function call technique SMTP is using and move the logic into core.
  3. Finding Overrides
    1. Add a event in configfactory->get() that only fires if a value is overridden. This would let you have a listener but not have a significant performance impact on the get() method
minnur’s picture

@frob you won't be able to override anything in the UI if the value is overridden in settings.php (this is how it is right now).

anavarre’s picture

Issue tags: +CMI 2.0 candidate

Tentatively adding the CMI 2.0 candidate tag. It would be nice to tackle this issue as part of the initiative.

frob’s picture

@minnur, that isn't the scenario that I am trying to describe.

What I am saying it should be possible to change a setting in the UI even if it is overridden, and have it update the settings in configuration and in the db, even if those settings are not reflected.

As an example:

Dev Environment has caching disabled (enforced by settings.php override).
Production has caching enabled as set in the configuration.

Changes to this setting should be visible in Dev, and they should be settable in Dev even if they are overridden. They need to be settable in Dev because we might want to stage these changes for Prod. This would be impossible if the UI disables changing a setting that is overridden by settings.php.

This is about staging settings, which is nearly impossible (without code) in D7 but a feature of D8.

sunset_bill’s picture

I've applied the patch on an 8.5 site and I see this working on the site settings form. I'm now trying to get it working on a custom config form and not having any luck. I'm extending ConfigFormBase, but I don't see getConfigOverrides() being called for my custom form, even though it is loading config values from my settings.php if they're available.

Dave Reid’s picture

For anyone wanting to apply this to client sites, I wrote the Configuration Override Warning contributed module because I wasn't happy on client sites applying a core patch or contrib modules that needed to add/alter every config form, especially other contrib modules' config forms.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

karolus’s picture

Know I'm quite late to this discussion, but I've started to build projects where the challenges discussed here could be an issue. While there are some contrib projects like Environment Indicator and Dave Reid's Configuration Override Warning module, I'm looking at keeping my codebase light, where possible.

In a scenario where I'm using local.settings.php, and development.services.yml to override settings in the dev environments, how difficult would it be to create a message to display on the /admin/config page that local settings are being used to override the defaults?

I'm pretty strict in my own process, and normally have plenty of offline documentation for projects I'm working on, but there may be situations, such as dealing with collaborative projects that some settings aren't documented, and could be overlooked, especially on tight deadlines.

I'm thinking along the lines of even a hard-coded alert/message that could be adding to the default settings.php or local.settings.php that could be uncommented/added that could alert site contributors to check for overridden values, or local settings that are being applied. It wouldn't have to specific about what is being changed, but only a heads-up that there are modifications in effect.

This quick mockup is along the lines of what I'm thinking:
Only local images are allowed.

colan’s picture

#122: I don't believe that's specific enough. From a UX perspective, I'd be asking: Which settings are overridden? And there are plenty of site builders who configure things, that don't know about (or don't have access to) the PHP settings files. So telling them to look in there probably wouldn't be helpful.

karolus’s picture

@colan
True, but at this point, it would be better than nothing. For this case, it would be showing that local settings are being applied. Even for site builders who don't have access to settings.php files, it would point them to an area to reference. Even if only to reach out to site admins to do it for them.

Right now, if a non-dev site builder were working on a site with configuration overrides that they didn't know about, it would be a serious UX fail, since they would have no way of knowing there are settings applied they are trying to work against.

karolus’s picture

Well, this may not be the most elegant, code-wise, but as a stopgap, for now, I've added an alert for my own sanity. To do this, I copied the HTML & CSS from a standard system error message, and created a custom block that is set to only display on the /admin/config page of my site.

Since I'm using configuration splits to manage different settings between environments, I added text and a link to this area. For all intents and purposes, it looks like a system-generated message, and is a good, persistent alert to me when I'm going to make changes.

Configuration Split Warning

ndf’s picture

Thanks @Dave Reid for the module Configuration Override Warn

It does the job.
It behaves as described in #118: it notifies about overrides, but it won't change any form so you can still change and save configuration via the UI.

Maybe if that module adds all the features that are proposed above in this issue, "Configuration Override Warn" can be added to core as experimental.

Some of those features are already added to the issue queue of "Configuration Override Warn" (as separate issues) like #60 #3000645: Add a permission so content editors aren't bombarded with confusing messages

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

liquidcms’s picture

Thanks a ton @Dave. Disappointing this isn't in core.

romainj’s picture

In addition to the Configuration Override Warn module you can try the Configuration Overview module that display the status of each configuration on a single page.

andypost’s picture

Reroll for 9.0.x - keeping needs work because 2 \Drupal:: calls are weird in trait, probably this method should live in ConfigFormBase

andypost’s picture

Fix new home for stable9 theme

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Hardik_Patel_12’s picture

Status: Needs work » Needs review
FileSize
12.84 KB
1.86 KB

Re-roll. Applies to 9.1.x-dev , and solving test cases also.

Status: Needs review » Needs work

The last submitted patch, 134: 2408549-133.patch, failed testing. View results

Hardik_Patel_12’s picture

Failure test case is

"Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest::testWidgetUploadAdvancedUi
"image-1.png" not found
Failed asserting that a boolean is not empty."

not related to above patch.

Hardik_Patel_12’s picture

Status: Needs work » Needs review
FileSize
14.75 KB
2.61 KB

Removing 2 \Drupal:: calls form ConfigFormBaseTrait and solving test cases.

sime’s picture

FileSize
143.48 KB
215.58 KB

From a DX perspective this seems fairly practical. I could be lazy and set '#config_data_store' => 'system.date' at the top level of my form and it just gave me a blanket warning if any of the config in that namespace was overridden. But from a user POV I found this difficult to use.

1. UX on Warnings

It feels like these warnings should be inline, and that form element widgets should be responsible somehow for formatting the message. It's not obvious that "0" and "Sunday" are the same, but the select widget would know this...

Showing that warning messages are not inline

2. Raw values using var_export?

If we need to display raw values, such as nested values, shouldn't they be yml with diffs, per the config module's administration pages? This example shows what happens when setting `#config_data_store` at the root of a form tree and override with an array.
This is especially true if the form declares the whole "system.date" top level namespace and i just change one value under that, it shows the whole array as overridden.

Showing that warnings messages are hard to read even for developers

3. Is the config form interface the right place?

Can't we just say "Hey there are overrides here, why not go to /admin/config/development/configuration/overrides and check out what's going on" and then use the diff system and stuff?

colan’s picture

Can't we just say "Hey there are overrides here, why not go to /admin/config/development/configuration/overrides and check out what's going on" and then use the diff system and stuff?

We can, and that would be better than nothing, but if would be far less lazy to specify exactly what's overridden on the form. Personally, I'd be fine with the redirection for now (to get something in), and a follow-up with the details in context. Ultimately, we don't want users asking:

Why can't they just tell me which of the fields on this form are overridden?

sime’s picture

It should still be possible to show which config fields are overridden and I would like to see a little warning message against the applicable elements (not at the top of the page). But for the gory detail link to a dedicated page. The problem I see here is not about identifying the elements so much as how to describe the changes in a snazzy way

miksan’s picture

Modified patch no 106 to match core 8.9.6...

Status: Needs review » Needs work

The last submitted patch, 141: 2408549-139.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ravi.shankar’s picture

Fixed failed test of patch #141.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alison’s picture

Arrived here from https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-...

I know it's been a while BUT, is this functionality still desired? (If yes...)

Am I misunderstanding the current plan, or will it only provide messages on forms that are specifically identified to have these messages? (And then other modules can add the same kind of code to their config forms.)

Apologies if this idea has already been floated: What if it there were a generic "FYI there are config overrides" message on CMI pages (/admin/config/development/configuration)? When config_split or config_ignore is on a site, it's obvious, but when there are config overrides in settings.php, you'd never know (that's why we're all here, of course). I'm only one person, but I would definitely want a message on the Synchronize page (or comparable drush commands), because that's where I go to examine the state of my config.

Again, apologies if it's already been discussed/decided-against. Thanks for all the work on this issue!

solideogloria’s picture

I would like this functionality. There are certain settings that shouldn't be in the database for security reasons, so they end up in settings.php.

An alternative is to use the Key module, yet this means that based on the provider you select, your overrides will need to be in a private file somewhere, config (this isn't any better), or environment variable (this just seems like a pain), or another system. If you use the private file key provider, you're just using a separate file for each override now, which is harder to manage than just putting everything in settings.php.

So personally, I think using settings.php for overrides makes the most sense. Yet, it really needs to be handled well in the UI, so that the config form can still be saved to update other fields without messing up anything or putting the sensitive value in the database.

narendra.rajwar27’s picture

Status: Needs work » Needs review
FileSize
12.45 KB

Adding patch for Drupal 9.5.x

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Shubham Chandra’s picture

Adding patch for drupal 10.1.x

Ratan Priya’s picture

Fixed custom commands for patch #152.

Liam Morland’s picture

If a setting cannot be changed in the UI because it is fixed externally, the UI component should be read only so that it is clear that it cannot be changed.

Liam Morland’s picture

This merge request contains patches #152 and #153 plus changes to address the errors raised by the automated testing of #153.

lauriii’s picture

Status: Needs review » Needs work

Tried to test this manually on the \Drupal\system\Form\SiteInformationForm but looks like that the MR isn't in a working state at the moment. The after build is only trying to check for the config overrides on form level, but it should actually be executed on individual form items.

There's also a bunch of Nightwatch tests failing so moving to Needs Work.

Liam Morland’s picture

Status: Needs work » Needs review

Rebased.

smustgrave’s picture

Status: Needs review » Needs work

Seems there are some test failures in the MR.

Liam Morland’s picture

Uploading patch so that automated tests will be run. This is to avoid #3194156: Patches and Merge Requests lead to different test results.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Still seems to have some errors. https://dispatcher.drupalci.org/job/drupal_patches/173008/console isn't super helpful

Wonder if $this->installEntitySchema('user'); is needed for ExceptionHandlingTest?

Liam Morland’s picture

I don't understand how the ElementClickInterceptedError could be caused by the patch.

It also doesn't make sense that it would return "Build Successful" instead of a red test failure.

DamienMcKenna’s picture

FWIW there is a known problem where sometimes drupalci doesn't collect all of the output from the pipeline and just returns "build successful", because of the focus on gitlabci there hasn't been much time spent looking into it.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

narendraR made their first commit to this issue’s fork.

tim.plunkett made their first commit to this issue’s fork.

narendraR’s picture

Status: Needs work » Needs review

Updated MR with required changes and made tests pass.

smustgrave’s picture

Hiding the patches

If we are adding a new permission think we will need a CR

I applied the MR and went to /admin/config/development/performance where aggregation is override but I don't see it.

narendraR’s picture

Status: Needs work » Needs review

Re #168, I have added a CR.

I applied the MR and went to /admin/config/development/performance where aggregation is override but I don't see it.

This can be achieved by adding $config['system.performance']['css']['preprocess'] = FALSE; in settings.php and adding below code.

diff --git a/core/modules/system/src/Form/PerformanceForm.php b/core/modules/system/src/Form/PerformanceForm.php
index cf26b6dbf1..b7dbd04638 100644
--- a/core/modules/system/src/Form/PerformanceForm.php
+++ b/core/modules/system/src/Form/PerformanceForm.php
@@ -146,6 +146,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
       '#title' => $this->t('Aggregate CSS files'),
       '#default_value' => $config->get('css.preprocess'),
       '#disabled' => $disabled,
+      '#config_data_store' => 'system.performance:css.preprocess',
     ];
     $form['bandwidth_optimization']['preprocess_js'] = [
       '#type' => 'checkbox',
smustgrave’s picture

Status: Needs review » Needs work

I did follow #169 and able to see the message, which is nice

This form contains values that have been overridden. Changes to these values can still be saved, but the overridden values will take precedence. Overrides are as follows:
In the config file system.performance
- The value for css.preprocess has been overridden. true has been changed to false

So wonder if we need a 2nd CR for developers to update their forms with this new value. "config_data_store". Or if a follow up should be made to make it automatic?

Rest looks good though.

narendraR’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Cleaned up the CR. Think putting a link to a comment on another ticket doesn't provide much for people.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

left a review on the MR

narendraR’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Can the issue summary be updated with a summary of how the current proposal works and screenshots / steps to reproduce?

lauriii’s picture

FYI, #3364506: Add optional validation constraint support to ConfigFormBase is implementing a generic base class for simple config forms. I think we should try to align these two issues because they both have different solutions for mapping config with form elements.

smustgrave’s picture

Status: Needs review » Needs work

For the issue summary update. Also #176 should be addressed. Not sure if one should be postponed for the other.

Wim Leers’s picture

Agreed with @lauriii in #176.

I don't see how

'#config_data_store' => 'system.site:page.front',

scales, or how we will ensure that every single key-value pair in config will get a correct annotation in the form definition. There's no incentive for that to happen? 😇 (Except wanting to support this feature of course!)

However, if this were to wait for #3364506: Add optional validation constraint support to ConfigFormBase to land, then doing this functionality would become trivial to add using the infrastructure that that issue is adding. The incentive there is also far stronger: no more validation logic in forms, tightly coupled to the shape of forms!

Finally, the pattern that #3364506 issue uses (mapConfigPropertyPathToFormElementName()) is architecturally similar to:

  1. \Drupal\Core\Entity\Display\EntityFormDisplayInterface::flagWidgetsErrorsFromViolations()
  2. \Drupal\Core\Entity\Entity\EntityFormDisplay::movePropertyPathViolationsRelativeToField()
Wim Leers’s picture

Status: Needs work » Needs review

Just pushed 2 commits that make this use #3364506: Add optional validation constraint support to ConfigFormBase's infrastructure. \Drupal\Tests\config\Functional\ConfigFormOverrideTest::testFormsWithOverrides() still passes! 👍

I'll let others finish this one — the UX still needs refining, but the blocking infrastructure is now in place, without the need for extra infrastructure being added by this issue! 🤓

Wim Leers’s picture

Also tested this with the only config form that #3364506 updated, and it works fine:

$config['update.settings']['notification']['emails'][] = 'wim@example.com';

in my sites/default/settings.php results in this:

P.S.: Just pushed a bonus commit that converts most of SiteInformationForm::validateForm() to validation constraints. IMHO we should open a blocking issue to first make type: path validatable, then this issue does not need to bother with that. I'd be fine with doing that in this issue too of course. Or to defer that entirely to a follow-up, because it's technically not in scope. The bonus commit can even be reverted in its entirety, but that'd leave SiteInformationForm in a kind of Frankenstein state.

smustgrave’s picture

Status: Needs review » Needs work

Seems there may be a schema error?

worldlinemine’s picture

Usability review

We discussed this issue at #3385954: Drupal Usability Meeting 2023-09-08. That issue will have a link to a recording of the meeting.

For the record, the attendees at today's usability meeting were @AaronMcHale, @Emma Horrell, @benjifisher, @rkoller, @simohell, and @worldlinemine.

  1. Switch the message type to info from its current type of warning
  2. The long sentence should be moved to the bottom of the proposed Info message.
  3. The list of overrides (and possibly the long sentence) should be placed in a collapsed detail element.
  4. Remove line of text describing the "config file".
  5. Add a link to https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-...
    1. There might be a better link source or new documentation as opposed to the API documentation but for now adding the suggested link would be an improvement.
    2. Test the link both inside and outside the details element.
  6. Use the translated label, not the value (for example - “Site name” instead of “name”).

Suggested format for the message would resemble the following example -

This form contains values that have been overridden:
<ul>
  <li>The value for "Site Name" has been overridden. 'Drush Site-Install' has been changed to 'Druverride'.</li>
</ul>
Changes to these values can still be saved, but the overridden values will take precedence.
benjifisher’s picture

Issue tags: +Needs followup

I have a few more comments based on the 2023-09-08 usability meeting:

#82.2: The long sentences at the start of the message are hard to parse.

#82.3: We have seen examples of sites with lots of overrides, and scrolling past the message can be a problem.

#82.4: Technically, something like "update.settings" (screenshot in #180) is a config object, not a file. Not every site exports configuration to files. The files have the extension .yml. If the site administrator wants to examine, change, or remove the override, then the config file is not the right place to look, and that is the main reason we recommend removing this line from the message. Overrides are commonly made in settings.php, and sometimes in modules, so we recommend a link to the documentation that explains this (#82.5).

In addition to a message at the top of the page, it might be a good idea to indicate the individual form elements that have overridden values. Maybe there is already an issue for this. I am adding the "Needs followup" issue tag.

There are several contrib modules that address the same problem, at least these:

At least one of these was mentioned in a previous comment (#120).

narendraR’s picture

CR needs to be updated as per new functionality.

rkoller’s picture

Issue tags: -Needs followup

I've created the two followup issues: #3389388: Indicate that a field value is overridden inline and #3389394: Add a reports page for overridden values. The only question in that regard is if i've assigned them to the appropriate components. About that i am unsure. I am also unsure if it is the correct approach to simply link this issue in the followup issues as related issues? Please update if necessary. And i've removed the Needs followup tag.

rkoller’s picture

FileSize
142.44 KB

And while writing up the two followup issues i've noticed one detail about the latest changes. If you change the value in the form that is overridden (I've changed the name from Drupal to Drupals) and then save the configuration i see the status message twice.

status message showing first the old value stored in config and then the same again for the newly saved one

The first is the old name the second is the new name i've changed to. If i reload the page only the new is shown.

narendraR’s picture

Wim Leers’s picture

@narendraR & others: could you please review #3382510: Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms? 🙏 That essentially does what you were originally doing here until I expressed concerns about it in #178: it introduces a new #config_target property, similar to the #config_data_store that was used since @alexpott introduced it in #85 6 years ago.

It has the potential to make this issue a trivial follow-up, without the need for new infra, only the "override indicator" would still be needed 😊

Wim Leers’s picture

yash.rode made their first commit to this issue’s fork.

yash.rode’s picture

Hi, I need some help with #2408549-186: There is no indication on configuration forms if there are overridden values. As we have added the checkConfigOverrides() callback on '#after_build' and the form loads multiple times. The status message is that's why getting printed twice. How can we avoid this? Thanks in advance.

yash.rode’s picture

error
Is this what we are expecting?

yash.rode’s picture

Status: Needs work » Needs review

If the answer for #192 is true, then this is ready for review.

narendraR’s picture

Status: Needs review » Needs work

I added below configurations in my settings.php

$config['system.site']['name'] = 'My Drupal site';
$config['system.site']['email'] = 'abc@yopmail.com';
$config['system.site']['slogan'] = 'My Drupal site slogan';
$config['user.settings']['anonymous'] = 'Visitor';
$config['system.site']['page.front'] = 'node/1';

On /admin/config/people/accounts I am getting
The value for name has been overridden. 'Drush Site-Install' has been changed to 'My Drupal site'
The value for slogan has been overridden. 'dfdgdf' has been changed to 'My Drupal site'
The value for anonymous has been overridden. 'Anonymous' has been changed to 'Visitor'

On /admin/config/system/site-information I am getting
The value for name has been overridden. 'Drush Site-Install' has been changed to 'My Drupal site'
The value for slogan has been overridden. 'dfdgdf' has been changed to 'My Drupal site'

Not sure why email and page.front not showing on Basic site settings page and also name and slogan is showing on account settings page.

Berdir’s picture

> The status message is that's why getting printed twice. How can we avoid this? Thanks in advance.

Yes, known issue with status messages added when building forms. See #3002571: Multiple warning messages when having untranslatable fields for an example for a workaround, in short, build a render array with the info instead.

Wim Leers’s picture

🏓 Posted a detailed review!

yash.rode’s picture

Trying to address @narendraR's feedback,

Not sure why email and page.front not showing on Basic site settings page

mail is working as expected, you just missed $config['system.site']['email'], it's mail and not email.

And page.front is not working as, when we call hasOverrides() with page.front it returns false. We are doing $parts = explode('.', $key); in the hasOverrides function so the $key gets break in to ['page', 'front'] and therefore we can't find that in settingsOverrides array, how can we fix this?

Berdir’s picture

> And page.front is not working as, when we call hasOverrides() with page.front it returns false. We are doing $parts = explode('.', $key); in the hasOverrides function so the $key gets break in to ['page', 'front'] and therefore we can't find that in settingsOverrides array, how can we fix this?

This does not need to be fixed, the override is wrong. This needs to be overridden as $config['system.site']['page']['front'] = 'node/1'; and *not* with a "."

Wim Leers’s picture

Merged in origin/11.x, because the applied suggestions assumed a newer version of core than this MR was starting from, causing test failures like this:

Drupal\Tests\workspaces\Kernel\WorkspaceIntegrationTest::testFormCacheForRegularForms
TypeError: Drupal\Core\Form\ConfigTarget::__construct(): Argument #3 ($fromConfig) must be of type ?string, Closure given, called in /builds/issue/drupal-2408549/core/modules/system/src/Form/SiteInformationForm.php on line 118
narendraR’s picture

Thanks for the review, Wim 🙏.

All points addressed, except below point, which needs some help.

Re

Will this work correctly for config that has schema like this?

I am not sure, but when I did $config['update.settings']['notification']['emails'] = ['a@abc.com', 'admin@example.com']; in settings.php, I am getting what is shown below:

Is there a way through which I can test

something:
  type: sequence
  label: Favorite fruits
  sequence:
    type: string
    label: Favorite fruit

Re:

1. I'd like to see an explicit test for this, so that we know what user experience to expect.

Is there any example in core from where I can get reference from?

Re:

I think it would be better to i) detect that the target is a sequence, ii) if the original and current values both have only numerical indices, only show the actually changed values using array_diff()

We are showing what changed with which value.

Wim Leers’s picture

Is there any example in core from where I can get reference from?

Yes: block.block.*:visibility, which is visible on any block form — for example /admin/structure/block/manage/olivero_powered on a fresh install of the Standard install profile.

yash.rode’s picture

I tried accessing /admin/structure/block/manage/olivero_powered and adding setting related to that to settings.php, but not getting the status message(Not totally sure about what I have added to settings.php-- $config['block.block.olivero_powered']['visibility']['request_path']['pages'][] = ['/randompage'];). Tried putting a breakpoint in, core/lib/Drupal/Core/Form/ConfigFormBase.php it is not going to the breakpoint, what can be the reason?

Wim Leers’s picture

what can be the reason?

Without digging into the code, these immediately come to mind:

  1. \Drupal\Core\Config\ConfigFactory::getEditable() vs ::get():
      public function getEditable($name) {
        return $this->doGet($name, FALSE);
      }
      public function get($name) {
        return $this->doGet($name);
      }
    

    👆 When calling ::getEditable(), overrides are explicitly NOT loaded!

  2. The form you're referring to is a config entity form (\Drupal\Core\Entity\EntityForm), not a simple config form (\Drupal\Core\Form\ConfigFormBase). Only the latter has #config_target.
yash.rode’s picture

Is there any example of ConfigFormBase where sequence is used
or is the below statement correct?

I am not sure, but when I did $config['update.settings']['notification']['emails'] = ['a@abc.com', 'admin@example.com']; in settings.php, I am getting what is shown below:

If it is correct I will write test for this, otherwise can you please suggest any of the existing form which I can write test for.

lauriii’s picture

Looks like the failing test may be a regression in the config validation system. Opened separate issue with failing test: #3408120: \Drupal\Core\Form\ConfigTarget is not fully serializable.

yash.rode’s picture

Status: Needs work » Needs review

Added test coverage and fixed test.

smustgrave’s picture

Status: Needs review » Needs work

#175 requested an issue summary update which still needed. Haven't looked at the CR to see what updates are needed there.

But should this be postponed on #3408120: \Drupal\Core\Form\ConfigTarget is not fully serializable from #205?

yash.rode’s picture

#207 that issue is already in, so no need to postpone.

narendraR’s picture

I don't think we require CR #3375145 now.

yash.rode’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs change record updates
FileSize
75.17 KB
alexpott’s picture

Can someone update https://www.drupal.org/node/3374955 to have a bit more about what the permission actually does and maybe with the image from https://www.drupal.org/node/3375145 and then I'll delete https://www.drupal.org/node/3375145 as #209 is correct - it is no longer needed.

yash.rode’s picture

Addressed #201, if that looks good https://www.drupal.org/node/3375145 can be deleted.

smustgrave’s picture

See there are some open threads, want to confirm this is good for review?

borisson_’s picture

Status: Needs review » Needs work

I changed a couple of things in the change record, I don't think this is actually ready for review, the open thread still need to be resolved. Marking this as needs work.

  1. There is missing test coverage, (see #199 - #203.
  2. There is also an optimization in ConfigFormBase.
yash.rode’s picture

Hi @borisson_ can you please have a look#204? I have already added test coverage for that.

@alexpott, did you get a chance to look at the approach you suggested for detecting config override https://git.drupalcode.org/project/drupal/-/merge_requests/3286#note_243097?

kunal.sachdev made their first commit to this issue’s fork.

kunal.sachdev’s picture

Status: Needs work » Needs review
smustgrave’s picture

Tested on a local 11.x

test

Not sure if this is desired? If so rest looks good so would be a +1 from me. But want to make sure that's expected.

Wim Leers’s picture

Status: Needs review » Needs work

#218: does that happen on all forms or just the performance settings form? 🤔

@kunal.sachdev You marked this Needs review but there are still 26 unresolved threads! 😅 I went over a bunch of them and found several unaddressed, and even with no response at all.

kunal.sachdev’s picture

#218: No, this is not expected. It is working like this because there are two forms ClearCacheForm and PerformanceForm and it shows the status message on the top of the PerformanceForm below the ClearCacheForm.

kunal.sachdev’s picture

Status: Needs work » Needs review

Worked on addressing feedback, but there are 26 unresolved threads and most of them can be resolved. Also there is an unrelated CSpell failure in MR.

fjgarlin made their first commit to this issue’s fork.

smustgrave’s picture

Re-tested

Went to the performance page and cleared cache and now have 2 status message blocks. Not sure if that's desired or if it's a 1 off.

error

kunal.sachdev’s picture

#223: This is happening because there are two forms ClearCacheForm and PerformanceForm on Performace Page and it shows the status messages of both the forms on the top of the respective forms. I discussed this with @lauriii and we think it's not a problem, so we can keep it it as it is for now.

kunal.sachdev’s picture

Tests are passing and all feedback is addressed, but there are 26 unresolved threads and most of them can be resolved.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

From #224 thanks for answering that!

Appears all feedback has been addressed

Unfortunately only original person or committers can close those threads :(

Martijn de Wit’s picture

Status: Reviewed & tested by the community » Needs work
alexpott’s picture

Discussed this issue with @lauriii and we agreed UX with putting a fake message on the screen is not great but it is outweighed by the improvement of having the information available. When this issue lands I'll file a follow-up to discuss improvements. I think we can do something quickly to immediately improve the situation before thinking about longer term solutions.

Also the same reasoning can apply to improving detecting whether or not the form has a field for the overridden config key - so we don't display the override information when actually there is nothing overridden on the form. This can be improved in a follow-up.

My major concern at the moment is why does this change make the nightwatch change necessary? @lauriii said the permissions form was crashing in nightwatch... if this change is causing that I'm concerned. I have no idea why it would though because this is about ConfigFormBase and the permissions form is not one of those.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

I removed the two changes that are not related using the gitlab UI... if this is green then I think the previous RTBC in #226 can apply.

alexpott’s picture

Okay I've discovered that with headless nightwatch the toolbar and the stick headers get in the way of the click on the permissions form. Given we're not testing the permissions form I think it is acceptable to use javascript to click the checkboxes here.

lauriii’s picture

+1 for the fix. Good job figuring that out. 👍

alexpott’s picture

I think we need to have a good think about #62

I have something to add. I am particularly overriding a secret from a payment processor, and I don't want that to leak to the admin, I wonder if that should be optional or something like this should be properly thought about it?

I guess we're okay because this is behind a new permission that has restrict access set to TRUE.

alexpott’s picture

So looking at the Stripe configuration form - you have two secrets in the form both in clear text. On your production site if you have overridden these to be your production values and left your sandbox values as the values in config then users able to navigate to the form will now be able to see the production values if they have the new permission. Both the new permission and the 'administer commerce_payment_gateway' have restrict access: true set to true.

Before this change the user would not have been able to determine the secret key and now they can (if they have the new permission). I'm undecided as to whether this is a big problem or not.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

As long as the information is only revealed to users that have the ability to administer that form, I think it's probably fine — because they're already trusted to make changes in production.

Besides, the restrict access: true flag triggers this warning:

        // Fill in default values for the permission.
        $perm_item += [
          'description' => '',
          'restrict access' => FALSE,
          'warning' => !empty($perm_item['restrict access']) ? $this->t('Warning: Give to trusted roles only; this permission has security implications.') : '',
        ];
        $form['permissions'][$perm]['description'] = [
          '#type' => 'inline_template',
          '#template' => '<div class="permission"><span class="title table-filter-text-source">{{ title }}</span>{% if description or warning %}<div class="description">{% if warning %}<em class="permission-warning">{{ warning }}</em> {% endif %}{{ description }}</div>{% endif %}</div>',
          '#context' => [
            'title' => $perm_item['title'],
          ],
        ];

👆 That led me to discover this documentation on \Drupal\user\PermissionHandlerInterface::getPermissions():

   *   - warning: (optional) A translated warning message to display for this
   *     permission on the permission administration page. This warning
   *     overrides the automatic warning generated by 'restrict access' being
   *     set to TRUE. This should rarely be used, since it is important for all
   *     permissions to have a clear, consistent security warning that is the
   *     same across the site. Use the 'description' key instead to provide any
   *     information that is specific to the permission you are defining.

Nothing in core uses this, but the Webform module does. In this case, I think providing extra context to inform site owners is probably worth it. Something like:
warning: "Warning: Give to trusted roles only; this permission has security implications. It may lead to sensitive configuration values that are specified in <code>settings.php to become visible in the user interface, such as API keys, tokens and other secrets."

WDYT?

bircher’s picture

Status: Needs review » Needs work

Given that one of the main reasons for using configuration overrides (other than languages) is to set production only values such as API keys, I think we should be very careful disclosing the values.
Also the fact that values are overwritten is useful for people who should not have the permission to see the value, maybe even more so. So the permission as it is makes me a bit uneasy because I think it the risk may not be obvious to people giving this new permission to roles.
Ie in the current implementation you need this permission to use this cool new feature, so first reaction is to assign it and then API keys are printed to the frontend as mentioned in #234

We discussed this on slack with @alexpott:

alexpott: I think we should remove the value the disclosure and not have a permission :slightly_smiling_face:
alexpott: And then add a permission for value disclosure in a followup
alexpott: Because then what you are opting in for is way more obvious.
bircher’s picture

And to reiterrate on #235 which was cross posted: If you already have the permission to see the form you should know that the values you are editing are not the ones used (ie the original point of this issue). Then knowing what the value is is a nice feature, but it is very context specific of whether or not that is a good idea.
If we do this in a follow up we can discuss things like adding a config schema for sensitive values or something creative like that without derailing the great progress of this issue.

alexpott’s picture

Discussed this with @lauriii.

  • We agree that we should change this issue to not display the overridden values due to the above discussion.
  • We agree that we that if you are not displaying the values then we can remove the new permission. Permission to view the config form is enough.
  • We agree that a follow-up should be create to discuss the displaying of overridden values and the security implications and possible solutions.
solideogloria’s picture

I agree with the above. Most of the values that I override are things like Stripe keys, access tokens, base URLs for API requests, and port numbers-- all things that are pretty important/sensitive. I'd rather not have the ability to see them easily in the UI, even as an admin. I want the values to be restricted to those who have access to the server itself and can view settings.php.

FeyP’s picture

warning: "Warning: Give to trusted roles only; this permission has security
implications. It may lead to sensitive configuration values that are
specified in settings.php to become visible in the user interface, such
as API keys, tokens and other secrets."

Wording the message in this way may lead a user to the wrong conclusion, that leakage of such info is the only security implication, which is often not the case. Depending on the module, this might actually not even be the most severe implication, but rather one of the minor implications.

Also fwiw +1 for #238.

Wim Leers’s picture

Sounds great — then there is no risk 👍

alexpott changed the visibility of the branch 2408549-one-more-permission to hidden.

alexpott’s picture

Status: Needs work » Needs review

I've pushed a commit that makes several improvements.

  1. It uses config target information to get all the information it needs to determine what is overridden. It no longer needs to flatten anything.
  2. It uses the label from schema (as opposed to the config key). This has a much higher chance of matching the text in the form and therefore being more obvious what's going on.
  3. It handles overrides individual elements of sequences more elegantly because it only cares about the config target map.
  4. It only displays information about what is overridden in the form - if the config file has something overridden that's not mapped to the form it does not display.
alexpott’s picture

FileSize
192.78 KB
180.15 KB

Uploading some screenshots of the updated text and way it looks. FWIW the handling of sequences such as the notification emails is much better now as multiple values in the override do not correspond to multiple overrides - there is only one form field.

Site information form

Settings.php

$config['system.site']['name'] = 'A name';

Form

Update settings

Settings.php

$config['update.settings']['notification']['emails'] = ['test@example.com', 'test1@example.com'];

Form

alexpott’s picture

I've tested this with config_translation enabled and in my opinion this reveals problems that @Gábor Hojtsy was detailing earlier. Unfortunately config overrides from translations bleed in. See the screenshot below... I've overridden the site name in settings and translated the slogan via the translation form and then viewed the main config form with German as the UI language...

I think we should only target global overrides in settings.php.

{Edit: fixed image]

alexpott’s picture

Wrong image uploaded.... in #245 - uploading new one and will fix above comment...

alexpott’s picture

Discussed #245 with @lauriii. We agreed to push the discussion to a follow-up because:

  • The message is not wrong - slogan is overridden and if you change it - it will not show on this page. You'd need to move to English to make it show.
  • Potentially we can expand the ConfigFactory API to allow us to discover what overrides are applying and inform the user.
alexpott’s picture

@Gábor Hojtsy has pointed out that the text

This form contains values that have been overridden:

Is a little bit tricky because the values in the form are not overridden. They are the values from active config - which you can change from this form. I think we need some better text here. Not sure yet though what that is.

Gábor Hojtsy’s picture

I had this text suggestion in #1 originally :D

I believe the intent is to inform the user that you are editing ORIGINAL configuration (that have NOT been overriden), which may have unexpected values based on what you experience on the site. Eg. you are editing a site name you normally don't see because you always use the site in another language.

Also if we are to always consider all possible overrides for a config, this status message would show up on most of the config forms on multilingual sites, because they have language overrides for translations of config, no? That could quickly lead to banner blindness and a feeling of bloat.

alexpott’s picture

I've updated the current text will elements of #249. I've also removed the details element. Now that we're not listing so much information I think the complexities it introduces for Claro are not worth it. Here's a screenshot of the current implementation:

The links have a title based on the form element title - Link to '{{ info.form_element_title }}' form element. Note we no longer have to wrry about green outlines in Claro as there's no details and we don't have to make any theme changes.

[Edit: fixed screenshot]

alexpott’s picture

Fixing the screenshot in the above comment.

Liam Morland’s picture

Does it say what the overridden values are? It is important for people to be able to see what their current configuration is, not just know that the actual value is some unknown value different from what is shown.

AaronMcHale’s picture

Issue tags: +Needs usability review

Perhaps the usability meeting can provide some suggestions/recommendations on the wording :)

alexpott’s picture

@Liam Morland please read the comments re the security concerns with displaying the values. See #236 and #238. This will be considered in a follow-up issue.

AaronMcHale’s picture

Usability review

We reviewed this issue at #3424764: Drupal Usability Meeting 2024-03-08. That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, @shaal, @simohell and @worldlinemine.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

During the meeting we discussed the wording of the message, we acknowledged that this is a particularly tricky one to get right because of the context in which overrides are used, so we spent a lot of time trying to find wording that appropriately conveys that the overrides don't directly apply to what the user sees on the form, while also trying to keep it short and to the point. We also noted that a previous usability review was done only 6 months ago, but we feel confident that we were able to improve on the wording.

The recommendation we settled on is: These values are overridden. Changes on this form will be saved, but overrides will take precedence. See [configuration overrides documentation](https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-override-system) for more information.

That is the wording of the first paragraph of the message, followed the unordered list of form elements.

Screenshot of proposed text.

This recommendation is based on a few key points:

  1. Documentation link: it should be clear to the user where they are going and what they can expect when clicking the link, additionally for users of assistive technology, when tabbing through the list of links on the page, the link text should describe the link without requiring any other context.
  2. Similarly, we tried putting the link on its own line, but we worried that the link could be confused for being part of the list of form elements, so showing the link further away from the start of the line and wrapping it within text should help distinguish it from the list of links following.
  3. We felt we were able to reduce the number of words in the message to a degree where it is concise but still clear. We were mindful that the unordered list could be quite long, if the user is viewing a form where many values are overridden.
  4. We noted that the text "The following form elements have overridden values" is not strictly true, the form may actually be the only place where the user sees the non-overridden values.
  5. Similarly, we felt that the proposed message contains enough information to communicate the situation, without using words like "current context" which may not be clear to the user. We felt that providing the documentation link was sufficient enough for users who needed additional information.
AaronMcHale’s picture

Status: Needs review » Needs work

omkar.podey made their first commit to this issue’s fork.

AaronMcHale’s picture

We probably don't need the link to documentation to open in a new tab, I don't believe other links to Drupal.org open in a new tab in the admin UI.

WCAG 2.1 says "Opening new windows and tabs from a link only when necessary", to me this doesn't seem like a "necessary" case. Additionally, "In general, it is better not to open new windows and tabs since they can be disorienting for people".

Source: https://www.w3.org/WAI/WCAG21/Techniques/general/G200

solideogloria’s picture

We probably don't need the link to documentation to open in a new tab

Agreed. Users can do any number of things themselves to open it in a new tab of their own volition:

  1. Ctrl+click
  2. Middle click
  3. Right click, click to open in a new tab
  4. Left click the link, then middle click the browser's back button