- Merge request !3286Issue #2408549: There is no indication on configuration forms if there are overridden values → (Closed) created by Liam Morland
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
This merge request contains patches #152 and #153 plus changes to address the errors raised by the automated testing of #153.
- Status changed to Needs work
almost 2 years ago 12:44pm 25 January 2023 - 🇫🇮Finland lauriii Finland
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.
- Status changed to Needs review
over 1 year ago 1:32pm 16 March 2023 - Status changed to Needs work
over 1 year ago 4:18pm 16 March 2023 - Status changed to Needs review
over 1 year ago 4:52pm 16 March 2023 - 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
Uploading patch so that automated tests will be run.
- Status changed to Needs work
over 1 year ago 6:20pm 16 March 2023 - 🇺🇸United States smustgrave
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?
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
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.
- 🇺🇸United States DamienMcKenna NH, USA
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.
- last update
over 1 year ago Build Successful - First commit to issue fork.
- 🇺🇸United States tim.plunkett Philadelphia
tim.plunkett → made their first commit to this issue’s fork.
- last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago 109 pass - last update
over 1 year ago Build Successful - last update
over 1 year ago 109 pass - last update
over 1 year ago 109 pass - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,811 pass - Status changed to Needs review
over 1 year ago 7:09am 14 July 2023 - 🇮🇳India narendraR Jaipur, India
Updated MR with required changes and made tests pass.
- Status changed to Needs work
over 1 year ago 2:48pm 14 July 2023 - 🇺🇸United States smustgrave
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.
- last update
over 1 year ago 29,812 pass, 2 fail - last update
over 1 year ago 29,815 pass - Status changed to Needs review
over 1 year ago 9:39am 17 July 2023 - 🇮🇳India narendraR Jaipur, India
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',
- Status changed to Needs work
over 1 year ago 2:39pm 17 July 2023 - 🇺🇸United States smustgrave
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 falseSo 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.
- Status changed to Needs review
over 1 year ago 5:35am 18 July 2023 - Status changed to RTBC
over 1 year ago 2:10pm 18 July 2023 - 🇺🇸United States smustgrave
Cleaned up the CR. Think putting a link to a comment on another ticket doesn't provide much for people.
- last update
over 1 year ago Fetch Error - last update
over 1 year ago 29,826 pass - last update
over 1 year ago 29,878 pass - Status changed to Needs work
over 1 year ago 10:15am 24 July 2023 22:13 42:43 Running- last update
over 1 year ago 29,877 pass - last update
over 1 year ago 29,879 pass - Status changed to Needs review
over 1 year ago 8:58am 25 July 2023 - 🇭🇺Hungary Gábor Hojtsy Hungary
Can the issue summary be updated with a summary of how the current proposal works and screenshots / steps to reproduce?
- 🇫🇮Finland lauriii Finland
FYI, ✨ Add optional validation constraint support to ConfigFormBase Fixed 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.
- last update
over 1 year ago 29,884 pass - Status changed to Needs work
over 1 year ago 2:29pm 27 July 2023 - 🇺🇸United States smustgrave
For the issue summary update. Also #176 should be addressed. Not sure if one should be postponed for the other.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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 ✨ Add optional validation constraint support to ConfigFormBase Fixed 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()
- last update
about 1 year ago 30,136 pass - Status changed to Needs review
about 1 year ago 3:55pm 4 September 2023 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Just pushed 2 commits that make this use ✨ Add optional validation constraint support to ConfigFormBase Fixed '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! 🤓
- last update
about 1 year ago 30,133 pass, 6 fail - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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 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. - last update
about 1 year ago Build Successful - last update
about 1 year ago Build Successful - Status changed to Needs work
about 1 year ago 8:03pm 4 September 2023 - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Build Successful - last update
about 1 year ago 30,074 pass, 44 fail - 🇺🇸United States worldlinemine
Usability review
We discussed this issue at 📌 Drupal Usability Meeting 2023-09-08 Fixed . 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.
- Switch the message type to info from its current type of warning
- The long sentence should be moved to the bottom of the proposed Info message.
- The list of overrides (and possibly the long sentence) should be placed in a collapsed detail element.
- Remove line of text describing the "config file".
- Add a link to
https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →
- 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.
- Test the link both inside and outside the details element.
- 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:- The value for "Site Name" has been overridden. 'Drush Site-Install' has been changed to 'Druverride'.
Changes to these values can still be saved, but the overridden values will take precedence.
- 🇺🇸United States benjifisher Boston area
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 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:
- https://www.drupal.org/project/config_override_warn →
- https://www.drupal.org/project/coi →
- https://www.drupal.org/project/config_override_message →
- https://www.drupal.org/project/config_notify →
- https://www.drupal.org/project/config_inspector → (related)
At least one of these was mentioned in a previous comment (#120).
- 🇮🇳India narendraR Jaipur, India
CR needs to be updated as per new functionality.
- last update
about 1 year ago 30,106 pass, 44 fail - last update
about 1 year ago 30,106 pass, 44 fail - 🇩🇪Germany rkoller Nürnberg, Germany
I've created the two followup issues: ✨ Indicate that a field value is overridden inline Active and ✨ Add a reports page for overridden values Active . 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. - 🇩🇪Germany rkoller Nürnberg, Germany
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.
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.
- 🇮🇳India narendraR Jaipur, India
Created issue as per #180 https://www.drupal.org/project/drupal/issues/3389566 📌 Add config validation for the path data type of config schema Active
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@narendraR & others: could you please review 📌 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 Fixed ? 🙏 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 😊
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Great news — 📌 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 Fixed landed, which is why this is now … pretty much … trivial? 🥳
- 🇮🇳India yash.rode pune
yash.rode → made their first commit to this issue’s fork.
- 🇮🇳India yash.rode pune
Hi, I need some help with #2408549-85: 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. - Status changed to Needs review
about 1 year ago 1:55pm 7 November 2023 - Status changed to Needs work
about 1 year ago 6:27am 8 November 2023 - 🇮🇳India narendraR Jaipur, India
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.
- 🇨🇭Switzerland berdir Switzerland
> 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 🐛 Multiple warning messages when having untranslatable fields Fixed for an example for a workaround, in short, build a render array with the info instead.
- 🇮🇳India yash.rode pune
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 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? - 🇨🇭Switzerland berdir Switzerland
> 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 "."
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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
- 🇮🇳India narendraR Jaipur, India
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.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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. - 🇮🇳India yash.rode pune
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? - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
what can be the reason?
Without digging into the code, these immediately come to mind:
-
\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! - 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
.
-
- 🇮🇳India yash.rode pune
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.
- 🇫🇮Finland lauriii Finland
Looks like the failing test may be a regression in the config validation system. Opened separate issue with failing test: 🐛 \Drupal\Core\Form\ConfigTarget is not fully serializable Active .
- Status changed to Needs review
11 months ago 9:39am 14 December 2023 - Status changed to Needs work
11 months ago 2:46pm 14 December 2023 - 🇺🇸United States smustgrave
#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 🐛 \Drupal\Core\Form\ConfigTarget is not fully serializable Active from #205?
- Status changed to Needs review
11 months ago 9:11am 15 December 2023 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇮🇳India yash.rode pune
Addressed #201, if that looks good https://www.drupal.org/node/3375145 → can be deleted.
- 🇺🇸United States smustgrave
See there are some open threads, want to confirm this is good for review?
- Status changed to Needs work
11 months ago 7:46am 23 December 2023 - 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
- 🇮🇳India yash.rode pune
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 detecting config override https://git.drupalcode.org/project/drupal/-/merge_requests/3286#note_243097?
- 🇮🇳India kunal.sachdev
kunal.sachdev → made their first commit to this issue’s fork.
- Status changed to Needs review
10 months ago 11:20am 31 January 2024 - 🇺🇸United States smustgrave
Tested 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.
- Status changed to Needs work
9 months ago 9:57am 7 February 2024 - 🇮🇳India kunal.sachdev
#218 🐛 Display status message on configuration forms when there are overridden values Fixed : No, this not what is 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.
- Status changed to Needs review
9 months ago 11:11am 12 February 2024 - 🇮🇳India kunal.sachdev
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.
- First commit to issue fork.
- 🇺🇸United States smustgrave
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.
- 🇮🇳India kunal.sachdev
#223 🐛 Display status message on configuration forms when there are overridden values Fixed : This is happening because there are two forms
ClearCacheForm
andPerformanceForm
on Performace Page and it shows the status messages of the 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. - 🇮🇳India kunal.sachdev
Tests are passing and all feedback is addressed, but there are 26 unresolved threads and most of them can be resolved.
- Status changed to RTBC
9 months ago 5:16pm 22 February 2024 - 🇺🇸United States smustgrave
From #224 thanks for answering that!
Appears all feedback has been addressed
Unfortunately only original person or committers can close those threads :(
- Status changed to Needs work
9 months ago 8:04am 23 February 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- Status changed to RTBC
9 months ago 2:18pm 23 February 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- Status changed to Needs review
9 months ago 1:31pm 26 February 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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?
- Status changed to Needs work
9 months ago 2:07pm 26 February 2024 - 🇨🇭Switzerland bircher 🇨🇿
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 #234We 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.
- 🇨🇭Switzerland bircher 🇨🇿
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. - 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
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 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.
- 🇩🇪Germany FeyP
warning: "Warning: Give to trusted roles only; this permission has security
implications. It may lead to sensitive configuration values that are
specified insettings.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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
alexpott → changed the visibility of the branch 2408549-one-more-permission to hidden.
- Status changed to Needs review
9 months ago 7:36pm 26 February 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
I've pushed a commit that makes several improvements.
- It uses config target information to get all the information it needs to determine what is overridden. It no longer needs to flatten anything.
- 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.
- It handles overrides individual elements of sequences more elegantly because it only cares about the config target map.
- 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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
@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.
- 🇭🇺Hungary Gábor Hojtsy Hungary
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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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. - 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
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.
- 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
Perhaps the usability meeting can provide some suggestions/recommendations on the wording :)
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
- 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
Usability review
We reviewed this issue at 📌 Drupal Usability Meeting 2024-03-08 Active . 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:
- 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.
- 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.
- 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.
- 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.
- 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.
- Status changed to Needs work
8 months ago 3:34pm 8 March 2024 - 🇮🇳India omkar.podey
omkar.podey → made their first commit to this issue’s fork.
- 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
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
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:
- Ctrl+click
- Middle click
- Right click, click to open in a new tab
- Status changed to Needs review
7 months ago 9:27am 26 April 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
I think this is ready the usability feedback has been addressed. It's a vast improvement on HEAD and we can iterate further improvements once this in.
- 🇬🇧United Kingdom longwave UK
Added some nitpicky feedback, but feel free to disregard as I hadn't read #255 first. I still think the "Status message" title doesn't really add any value though.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
Replice to all of @longwave's comments on the MR and fixed issue summary screenshot...
- Status changed to Needs work
7 months ago 4:11pm 26 April 2024 - 🇭🇺Hungary Gábor Hojtsy Hungary
Sorry for being that guy. But my main problem remains with the text that it does not help. The logic I think is "the values may be overridden elsewhere (we don't know where) based on some criteria (which we also don't know)" so users may or may not encounter that override anywhere else. I can see this may still help solve some confusion when/if you are aware of those overrides or conditions. But I also see a lot of potential for new confusion too. Because the override is not present in the form itself. So the statement that the values are overridden is not true for the form itself and may only happen elsewhere.
Eg. you see your site name is "Bla". You go edit it and its "Foo" there. You demand an explanation. But you go edit a setting and it says its overridden, but you may have never seen the overriden value (in some organic group or whatnot) and you are puzzled as to what does that even mean.
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
The current situation is worse. The config page says it is configured in a certain way, but secretly the config is actually different. Even a cryptic message is an improvement because the user could search the Internet for the message and find an explanation.
It seems to me that the most important thing is for a user to be able to see what the actual config is, even if they are not able to change it in the UI.
- Status changed to Needs review
7 months ago 7:49pm 26 April 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
Thanks @longwave and @AaronMcHale - removed "Link to" as instructed. Very happy to be told what to do here :)
@Gábor Hojtsy - I disagree. I think knowing that a specific field is overridden and your changes will not apply is incredibly valuable and a massive improvement on the current state of play where you have no clues as to what is going on. For me the approach on this issue represents an improvement and something we can continue to improve over time.
The current behaviour of allowing you to change a value and then that change not applying with no indication of what is going on is way way more confusing than this message and we're linking to a page on drupal.org that we can continue to improve with information about what is going on.
- 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
Agree with @Liam Morland and @alexpott that this is still an improvement on the current situation. I think we can continue to improve it in follow-up issues. For example, maybe there's a way for us to tell if the override is coming from settings.php (likely the 90% case), so that could be a good follow-up candidate.
At least now we are telling people that an override exists, the current state is arguably more confusing because there's no indication on the form. I think in most cases the site itself would have applied the overrides, so there's a good chance that the person looking at the form was the person who applied the override, or at least they are aware that some settings have been overridden. They might have completely forgotten that in the past they did override a value, maybe because they did it only for a specific environment in settings.php by wrapping the override in an if statement that checks the value of, say, the ENV_NAME variable, and it's not the environment they are working on.
Another good thing about this change is that it gives you confidence that your overrides are actually taking effect. The site name is an easy one to check, but maybe you have overridden configuration like the mail server settings for the SMTP module for only a specific environment, and the only way for you to know if the override worked is to send mail and hope it goes to the right place. At least now, no matter what configuration you override you have a way of knowing that the override value is in use.
- 🇺🇸United States markdorison
Agreed with @alexpott, @Liam Morland, and @AaronMcHale. It would be great to provide even further detail, but any additional context is a net positive here.
- 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
Usability review was done in #255, I think I just forgot to remove the tag, sorry for any confusion.
- Status changed to RTBC
5 months ago 6:02pm 14 June 2024 - 🇺🇸United States smustgrave
Retested this one again on the performance page and the message definitely helps for me.
Verified the link is correct
Verified the anchor links for the configuration correctly go to the right fields and clicking them moves focus to the configuration.Think this would be a good improvement
- 🇦🇺Australia sime Melbourne
I've preferred settings.php overrides for nearly 10 years, and they have felt like a second class citizen to contrib solutions for all that time. I support the sentiment of #267, #268, etc. Massive step forward.
I tested the same things as #272. Since I last looked at issue I see there's a twig template for the message which was a nice surprise! So I also tested that I could override that, because for now I would do that as a standard practice to explain to site builders where the values are being overridden.
- 🇦🇺Australia acbramley
This looks awesome! A massive improvement over the current state.
Looks like the CR needs updating as the screenshot no longer matches what's been implemented.
- 🇦🇺Australia acbramley
CR has been rewritten, I've used the same screenshot from the IS.
- Status changed to Needs work
4 months ago 6:24am 8 July 2024 - 🇳🇿New Zealand quietone
I looked at the MR and this should add parameter and return type hints, https://www.drupal.org/docs/develop/standards/php/php-coding-standards#s... → as well as adding
declare(strict_types = 1);
. Setting to NW for this piece.I also read the CR and it reads well to me, although the branch and version are incorrect because 10.2 is in security mode now.
- Status changed to RTBC
4 months ago 7:25am 8 July 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
I've added typehints and
declare(strict_types = 1);
but I think we should not set MR back from rtbc for such things, ff PHPCS / PHPStan cannot enforce the standard.Also the majority of typehints I added were for hooks or callbacks which always feels a bit meh because we have no way of enforcing that (yet).
- Status changed to Needs work
4 months ago 9:45am 10 July 2024 - 🇬🇧United Kingdom catch
One complaint - #theme links supports the exact HTML structure of the new template/theme hook so we can use that with a suggestion instead of adding yet another one-off theme hook. See #1804614: [meta] Consolidate similar twig templates and properly use theme suggestions in core → for the general issue trying to fix this for existing one-off theme hooks across core.
- Status changed to RTBC
4 months ago 12:37pm 10 July 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
I've implemented @catch's suggestion. The only change is the the text at the top of the message is now wrapped in a div which feels correct to me anyway. Given all this is doing is removing a theme template and re-using one we have in core... setting back to RTBC.
Obligatory screenshot attached to prove that nothing has changed.
- 🇬🇧United Kingdom catch
diffstat on that last couple of commits looks great, about 50 lines net removal, we don't have to add another template override to stable etc.
Nice to get this one over the line finally. Committed/pushed to 11.x, 11.0.x and 10.4.x, since this has UI/string changes I don't think it should go back to 10.3.x. Thanks everyone!
Did my best with commit credit but apologies if anyone was overlooked.
- Status changed to Fixed
4 months ago 1:00pm 10 July 2024 - 🇺🇸United States apotek
Gloria in excelsis mundi !!! Thank you to everyone for this epic improvement.
- 🇺🇸United States bkosborne New Jersey, USA
I just saw this change record. Awesome work!!
Automatically closed - issue fixed for 2 weeks with no activity.