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.
Comment | File | Size | Author |
---|---|---|---|
#251 | CleanShot 2024-03-01 at 14.22.36@2x.png | 81.5 KB | alexpott |
#250 | CleanShot 2024-03-01 at 14.17.30@2x.png | 82.14 KB | alexpott |
#246 | CleanShot 2024-02-27 at 10.02.40@2x.png | 248.8 KB | alexpott |
#245 | CleanShot 2024-02-27 at 09.48.46@2x.png | 179.75 KB | alexpott |
#244 | site name.png | 180.15 KB | alexpott |
Issue fork drupal-2408549
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:
Comments
Comment #1
Gábor HojtsyHere is a quick patch. Not sure how to do this better. I used the following text:
The reasons are the following:
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.
Comment #2
Gábor HojtsyJust realizing now that we should also incorporate in the text somehow that those overrides are not actually visible / used in the form.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedActually, 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.
Comment #4
Gábor HojtsySo 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?
Comment #5
alexpottTagging...
Comment #6
jhedstromThis is the same as #1 with a test added.
This does seem like it would certainly impact performance.
Comment #7
xjmComment #9
anavarreMoshe 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:
Comment #10
Gábor Hojtsy@anavarre: which overrides would that drush command consider? language? domain? time? is-it-pirate-day?
Comment #11
anavarre#10 it only focuses on active storage
https://github.com/drush-ops/drush/blob/master/commands/core/config.drus...
Comment #12
Gábor Hojtsy@anavarre: there may be any number of overrides (collections) in active storage some of which may apply under specific conditions.
Comment #13
anavarreIsn'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?
Comment #14
Gábor Hojtsy@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.
Comment #15
anavarreThat'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 ofsettings.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.Comment #16
Gábor Hojtsy@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.
Comment #17
anavarreFiled https://github.com/drush-ops/drush/issues/1710 accordingly
EDIT: not much we can do it seems. Issue closed.
Comment #18
swentel CreditAttribution: swentel commentedOk, 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 ?
extreme nitpick - one line to many
Comment #19
swentel CreditAttribution: swentel commentedGoing to be bold here and setting to RTBC, want to know what committers think of it :)
Comment #20
dasjoAlso 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 :)
Comment #21
Gábor HojtsyI 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.
Comment #22
Gábor Hojtsy@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.
Comment #23
alexpottI;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.
Comment #24
BerdirThe 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?
Comment #25
Gábor HojtsyClearly still being discussed.
Comment #26
swentel CreditAttribution: swentel commentedSo what if we add a setting to settings.local.php, someting like
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..
Comment #27
BerdirYes, 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?
Comment #28
swentel CreditAttribution: swentel commentedMarked #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.
Comment #29
swentel CreditAttribution: swentel commentedso something like this then ?
Comment #30
alexpottI think we now need to say that the overrides are from settings.php.
Comment #31
mgiffordSo 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?
Comment #32
Gábor Hojtsy@mgifford: Here is a settings.php snippet for you:
Of course less contrived examples are possible :)
Comment #33
dasjo@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.
Comment #34
mgifford@Gábor Hojtsy A hearty thanks fer that excellent example!
Comment #35
alexpottNot 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.
Comment #36
alexpottMaybe 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.
Comment #37
Gábor HojtsyWell, 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.
Comment #38
alexpottHow about this then
Comment #39
Gábor HojtsySounds 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 :)
Comment #40
alexpott@Gábor Hojtsy well someone at Chapter Three who experienced the current behaviour was very very confused and asked why is there no warning.
Comment #41
dawehnerShould we tell people which elements are overridden? We could even extract a human readable name optionally when we leverage config schema.
Comment #42
Gábor Hojtsy@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.
Comment #43
dawehner@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.
Comment #44
minnur CreditAttribution: minnur at Chapter Three commentedI 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 ?
Comment #46
yoroy CreditAttribution: yoroy at Roy Scholten commentedNot sure I understand the details of this discussion but slightly shorter rewrite of the second sentence:
"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…
Comment #47
Gábor Hojtsy@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.
Comment #48
zviryatko CreditAttribution: zviryatko at AnyforSoft commentedI'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.
Comment #49
pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft, Drupal Ukraine Community for AnyforSoft commentedI do like the solution made in SMTP module, mentioned in #48
Comment #51
Elijah LynnPosting 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.
Comment #52
Elijah LynnAlso, 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.
Comment #54
ohthehugemanatee CreditAttribution: ohthehugemanatee at Amazee Labs commentedYep, 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.
Comment #56
joshua.boltz CreditAttribution: joshua.boltz commentedThe 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.
Comment #57
Elijah LynnEmbedding #56 for easier viewing.
Comment #58
DamienMcKennaRerolled.
Comment #60
eiriksmOK, 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?
Comment #61
eiriksmComment #62
hanoiiI 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?
Comment #63
slydevil CreditAttribution: slydevil commentedThe patch from #60 works well. I patched an 8.3 site with it no problem.
Comment #65
jzavrl CreditAttribution: jzavrl at NDP commentedThe 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.
Comment #66
colanSetting status based on #65.
Comment #67
DamienMcKennaUpdated per #65.
Comment #68
mirom CreditAttribution: mirom at MADE it digital s.r.o. commentedI 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?
Comment #69
eiriksmUpdated the patch with:
- Use a template.
- Add a permission.
- Use this permission.
Still needs tests though
Comment #71
frobIt 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.
Comment #72
geek-merlin#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.
Comment #73
jenlamptonSome feedback on the language and messaging...
* 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.
I agree we should get this in first, and then add something similar that could be used via REST.
Comment #74
jenlamptonWell, 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.
Comment #75
jenlamptonComment #77
Gábor Hojtsy@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.
Comment #78
jenlampton@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...
Comment #79
Gábor Hojtsy@jenlampton:
Comment #80
geek-merlinUnlike 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).
Comment #81
frobIs there a Drupal 8 equivalent to system_settings_form?
Seems like that might help with a 90% usecase here.
Comment #82
alexpott@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.Comment #83
colanSounds 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)?
Comment #84
alexpott@colan we'd need a separate issue for that.
Comment #85
alexpottHere'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.
Comment #87
alexpottFixing the test... it now has a dependency on user because of the user role check.
Comment #88
agentrickard@axel.rutz -- Not to derail here, but something like what you mention was filed here -- #2677124: Allow discovery of configuration UI.
Comment #90
Chi CreditAttribution: Chi commentedConfigFormOverrideTest has been converted to browser based test.
#2870439: Convert web tests to browser tests for config module
Comment #91
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #92
Chi CreditAttribution: Chi commentedThis won't print properly boolean values.
I propose something like this.
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)?
Comment #94
Chi CreditAttribution: Chi commentedNeeds reroll after #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it.
Comment #95
Chi CreditAttribution: Chi commentedComment #96
yogeshmpawarComment #97
yogeshmpawarAdded updated patch because previous patch failed to apply on 8.5.x branch.
Comment #98
yogeshmpawarComment #99
borisson_Hidden all previous patches, made the changes request in #92. I've also finished an unfinished comment.
Comment #101
bircherThis 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
Comment #102
colanThat one should get in soon.
Comment #103
anavarre#2923004: Add method to check if any overrides are applied to \Drupal\Core\Config\Config just landed.
Comment #104
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedDone as mentioned in #101
Please review.
Comment #106
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect the test failure by calling
hasOverrides()
on the$config
variable.Comment #108
Wim LeersComment #110
dpiHi everyone, Sam152 made me aware that a pair of modules I recently released are relevant to what this issue hopes to solve:
Provides mapping between form fields to which config they represent.
Provides various UI modifications if a field representing config is overridden.
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: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.Comment #111
andypostSetting mapping for each element is not very handy
Looks config translation mapping is better way to build forms for config & provide indication
Comment #112
minnur CreditAttribution: minnur at Chapter Three commentedJust as an idea, maybe it makes also sense to make elements overridden in settings.php disabled in the UI ?
Comment #113
jofitz CreditAttribution: jofitz at ComputerMinds commented@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.
Comment #114
frobWhile 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.
Comment #115
diamondseaLate to the party, but I'll add a few possibilities based on reading the issue so far:
Comment #116
minnur CreditAttribution: minnur at Chapter Three commented@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).
Comment #117
anavarreTentatively adding the CMI 2.0 candidate tag. It would be nice to tackle this issue as part of the initiative.
Comment #118
frob@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.
Comment #119
sunset_bill CreditAttribution: sunset_bill commentedI'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.
Comment #120
Dave ReidFor 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.
Comment #122
karolus CreditAttribution: karolus as a volunteer commentedKnow 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:
Comment #123
colan#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.
Comment #124
karolus CreditAttribution: karolus as a volunteer commented@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.
Comment #125
karolus CreditAttribution: karolus as a volunteer commentedWell, 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.
Comment #126
ndf CreditAttribution: ndf at Dx Experts for Clockwork commentedThanks @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
Comment #129
liquidcms CreditAttribution: liquidcms commentedThanks a ton @Dave. Disappointing this isn't in core.
Comment #130
romainj CreditAttribution: romainj as a volunteer commentedIn 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.
Comment #131
andypostReroll for 9.0.x - keeping needs work because 2
\Drupal::
calls are weird in trait, probably this method should live inConfigFormBase
Comment #132
andypostFix new home for stable9 theme
Comment #134
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedRe-roll. Applies to 9.1.x-dev , and solving test cases also.
Comment #136
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedFailure test case is
not related to above patch.
Comment #137
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedRemoving 2 \Drupal:: calls form ConfigFormBaseTrait and solving test cases.
Comment #138
simeFrom 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...
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.
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?
Comment #139
colanWe 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:
Comment #140
simeIt 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
Comment #141
miksan CreditAttribution: miksan at Adapt commentedModified patch no 106 to match core 8.9.6...
Comment #143
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed failed test of patch #141.
Comment #148
alisonArrived 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!
Comment #149
solideogloria CreditAttribution: solideogloria commentedI 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.
Comment #150
narendra.rajwar27Adding patch for Drupal 9.5.x
Comment #152
Shubham Chandra CreditAttribution: Shubham Chandra as a volunteer and at Dotsquares Ltd. commentedAdding patch for drupal 10.1.x
Comment #153
Ratan Priya CreditAttribution: Ratan Priya at OpenSense Labs for DrupalFit commentedFixed custom commands for patch #152.
Comment #154
Liam MorlandIf 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.
Comment #156
Liam MorlandThis merge request contains patches #152 and #153 plus changes to address the errors raised by the automated testing of #153.
Comment #157
lauriiiTried 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.
Comment #158
Liam MorlandRebased.
Comment #159
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems there are some test failures in the MR.
Comment #160
Liam MorlandUploading patch so that automated tests will be run. This is to avoid #3194156: Patches and Merge Requests lead to different test results.
Comment #161
smustgrave CreditAttribution: smustgrave at Mobomo commentedStill 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?
Comment #162
Liam MorlandI 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.
Comment #163
DamienMcKennaFWIW 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.
Comment #167
narendraRUpdated MR with required changes and made tests pass.
Comment #168
smustgrave CreditAttribution: smustgrave at Mobomo commentedHiding 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.
Comment #169
narendraRRe #168, I have added a CR.
This can be achieved by adding
$config['system.performance']['css']['preprocess'] = FALSE;
in settings.php and adding below code.Comment #170
smustgrave CreditAttribution: smustgrave at Mobomo commentedI did follow #169 and able to see the message, which is nice
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.
Comment #171
narendraRComment #172
smustgrave CreditAttribution: smustgrave at Mobomo commentedCleaned up the CR. Think putting a link to a comment on another ticket doesn't provide much for people.
Comment #173
larowlanleft a review on the MR
Comment #174
narendraRComment #175
Gábor HojtsyCan the issue summary be updated with a summary of how the current proposal works and screenshots / steps to reproduce?
Comment #176
lauriiiFYI, #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.
Comment #177
smustgrave CreditAttribution: smustgrave at Mobomo commentedFor the issue summary update. Also #176 should be addressed. Not sure if one should be postponed for the other.
Comment #178
Wim LeersAgreed with @lauriii in #176.
I don't see how
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:\Drupal\Core\Entity\Display\EntityFormDisplayInterface::flagWidgetsErrorsFromViolations()
\Drupal\Core\Entity\Entity\EntityFormDisplay::movePropertyPathViolationsRelativeToField()
Comment #179
Wim LeersJust 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! 🤓
Comment #180
Wim LeersAlso tested this with the only config form that #3364506 updated, and it works fine:
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 maketype: 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 leaveSiteInformationForm
in a kind of Frankenstein state.Comment #181
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems there may be a schema error?
Comment #182
worldlinemine CreditAttribution: worldlinemine as a volunteer commentedUsability 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.
Suggested format for the message would resemble the following example -
Comment #183
benjifisherI 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 insettings.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).
Comment #184
narendraRCR needs to be updated as per new functionality.
Comment #185
rkollerI'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.Comment #186
rkollerAnd 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.
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.
Comment #187
narendraRCreated issue as per #180 https://www.drupal.org/project/drupal/issues/3389566
Comment #188
Wim Leers@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 😊
Comment #189
Wim LeersGreat news — #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 landed, which is why this is now … pretty much … trivial? 🥳
Comment #191
yash.rode CreditAttribution: yash.rode at Acquia commentedHi, 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.Comment #192
yash.rode CreditAttribution: yash.rode at Acquia commentedIs this what we are expecting?
Comment #193
yash.rode CreditAttribution: yash.rode at Acquia commentedIf the answer for #192 is true, then this is ready for review.
Comment #194
narendraRI added below configurations in my settings.php
On
/admin/config/people/accounts
I am gettingThe 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 gettingThe 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.
Comment #195
Berdir> 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.
Comment #196
Wim Leers🏓 Posted a detailed review!
Comment #197
yash.rode CreditAttribution: yash.rode at Acquia commentedTrying to address @narendraR's feedback,
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 callhasOverrides()
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 insettingsOverrides
array, how can we fix this?Comment #198
Berdir> 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 "."Comment #199
Wim LeersMerged 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:Comment #200
narendraRThanks for the review, Wim 🙏.
All points addressed, except below point, which needs some help.
Re
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
Re:
Is there any example in core from where I can get reference from?
Re:
We are showing what changed with which value.
Comment #201
Wim LeersYes:
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.Comment #202
yash.rode CreditAttribution: yash.rode at Acquia commentedI 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?Comment #203
Wim LeersWithout digging into the code, these immediately come to mind:
\Drupal\Core\Config\ConfigFactory::getEditable()
vs::get()
:👆 When calling
::getEditable()
, overrides are explicitly NOT loaded!\Drupal\Core\Entity\EntityForm
), not a simple config form (\Drupal\Core\Form\ConfigFormBase
). Only the latter has#config_target
.Comment #204
yash.rode CreditAttribution: yash.rode at Acquia commentedIs there any example of
ConfigFormBase
where sequence is usedor is the below statement correct?
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.
Comment #205
lauriiiLooks 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.
Comment #206
yash.rode CreditAttribution: yash.rode at Acquia commentedAdded test coverage and fixed test.
Comment #207
smustgrave CreditAttribution: smustgrave at Mobomo commented#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?
Comment #208
yash.rode CreditAttribution: yash.rode at Acquia commented#207 that issue is already in, so no need to postpone.
Comment #209
narendraRI don't think we require CR #3375145 now.
Comment #210
yash.rode CreditAttribution: yash.rode at Acquia commentedComment #211
alexpottCan 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.
Comment #212
yash.rode CreditAttribution: yash.rode at Acquia commentedAddressed #201, if that looks good https://www.drupal.org/node/3375145 can be deleted.
Comment #213
smustgrave CreditAttribution: smustgrave at Mobomo commentedSee there are some open threads, want to confirm this is good for review?
Comment #214
borisson_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.
Comment #215
yash.rode CreditAttribution: yash.rode at Acquia commentedHi @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?
Comment #217
kunal.sachdev CreditAttribution: kunal.sachdev at Acquia commentedComment #218
smustgrave CreditAttribution: smustgrave at Mobomo commentedTested on a local 11.x
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.
Comment #219
Wim Leers#218: does that happen on all forms or just the performance settings form? 🤔
@kunal.sachdev You marked this
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.Comment #220
kunal.sachdev CreditAttribution: kunal.sachdev at Acquia commented#218: No, this is not expected. It is working like this because there are two forms
ClearCacheForm
andPerformanceForm
and it shows the status message on the top of thePerformanceForm
below theClearCacheForm
.Comment #221
kunal.sachdev CreditAttribution: kunal.sachdev at Acquia commentedWorked 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.
Comment #223
smustgrave CreditAttribution: smustgrave at Mobomo commentedRe-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.
Comment #224
kunal.sachdev CreditAttribution: kunal.sachdev at Acquia commented#223: This is happening because there are two forms
ClearCacheForm
andPerformanceForm
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.Comment #225
kunal.sachdev CreditAttribution: kunal.sachdev at Acquia commentedTests are passing and all feedback is addressed, but there are 26 unresolved threads and most of them can be resolved.
Comment #226
smustgrave CreditAttribution: smustgrave at Mobomo commentedFrom #224 thanks for answering that!
Appears all feedback has been addressed
Unfortunately only original person or committers can close those threads :(
Comment #227
Martijn de WitComment #228
alexpottDiscussed 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.
Comment #229
alexpottI 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.
Comment #231
alexpottOkay 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.
Comment #232
lauriii+1 for the fix. Good job figuring that out. 👍
Comment #233
alexpottI think we need to have a good think about #62
I guess we're okay because this is behind a new permission that has restrict access set to TRUE.
Comment #234
alexpottSo 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.
Comment #235
Wim LeersAs 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:👆 That led me to discover this documentation on
\Drupal\user\PermissionHandlerInterface::getPermissions()
: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?
Comment #236
bircherGiven 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:
Comment #237
bircherAnd 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.
Comment #238
alexpottDiscussed this with @lauriii.
Comment #239
solideogloria CreditAttribution: solideogloria commentedI 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.
Comment #240
FeyP CreditAttribution: FeyP as a volunteer and at werk21 commentedWording 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.
Comment #241
Wim LeersSounds great — then there is no risk 👍
Comment #243
alexpottI've pushed a commit that makes several improvements.
Comment #244
alexpottUploading 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
Form
Update settings
Settings.php
Form
Comment #245
alexpottI'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]
Comment #246
alexpottWrong image uploaded.... in #245 - uploading new one and will fix above comment...
Comment #247
alexpottDiscussed #245 with @lauriii. We agreed to push the discussion to a follow-up because:
Comment #248
alexpott@Gábor Hojtsy has pointed out that the text
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.
Comment #249
Gábor HojtsyI 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.
Comment #250
alexpottI'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]
Comment #251
alexpottFixing the screenshot in the above comment.
Comment #252
Liam MorlandDoes 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.
Comment #253
AaronMcHalePerhaps the usability meeting can provide some suggestions/recommendations on the wording :)
Comment #254
alexpott@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.
Comment #255
AaronMcHaleUsability 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.
This recommendation is based on a few key points:
Comment #256
AaronMcHaleComment #258
AaronMcHaleWe 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
Comment #259
solideogloria CreditAttribution: solideogloria commentedAgreed. Users can do any number of things themselves to open it in a new tab of their own volition: