Display status message on configuration forms when there are overridden values

Created on 16 January 2015, over 9 years ago
Updated 11 July 2024, 1 day ago

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.

๐Ÿ› Bug report
Status

Fixed

Version

10.4 โœจ

Component
Configurationย  โ†’

Last updated about 17 hours ago

Created by

๐Ÿ‡ญ๐Ÿ‡บHungary Gรกbor Hojtsy Hungary

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • ๐Ÿ‡จ๐Ÿ‡ฆ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 over 1 year ago
  • ๐Ÿ‡ซ๐Ÿ‡ฎ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
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Liam Morland Ontario, CA ๐Ÿ‡จ๐Ÿ‡ฆ

    Rebased.

  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Seems there are some test failures in the MR.

  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Liam Morland Ontario, CA ๐Ÿ‡จ๐Ÿ‡ฆ

    Uploading patch so that automated tests will be run.

  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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 about 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 about 1 year ago
    Build Successful
  • last update about 1 year ago
    Build Successful
  • last update about 1 year ago
    Build Successful
  • last update about 1 year ago
    Build Successful
  • last update about 1 year ago
    Build Successful
  • last update about 1 year ago
    Build Successful
  • last update about 1 year ago
    Build Successful
  • last update 12 months ago
    Build Successful
  • last update 12 months ago
    Build Successful
  • last update 12 months ago
    Build Successful
  • last update 12 months ago
    109 pass
  • last update 12 months ago
    Build Successful
  • last update 12 months ago
    109 pass
  • last update 12 months ago
    109 pass
  • last update 12 months ago
    Custom Commands Failed
  • last update 12 months ago
    29,811 pass
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

    Updated MR with required changes and made tests pass.

  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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 12 months ago
    29,812 pass, 2 fail
  • last update 12 months ago
    29,815 pass
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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 false

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

    Rest looks good though.

  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to RTBC 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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 12 months ago
    Fetch Error
  • last update 12 months ago
    29,826 pass
  • last update 12 months ago
    29,878 pass
  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    left a review on the MR

  • 7:32
    28:02
    Running
  • last update 12 months ago
    29,877 pass
  • last update 12 months ago
    29,879 pass
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • ๐Ÿ‡ญ๐Ÿ‡บ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 12 months ago
    29,884 pass
  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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:

    1. \Drupal\Core\Entity\Display\EntityFormDisplayInterface::flagWidgetsErrorsFromViolations()
    2. \Drupal\Core\Entity\Entity\EntityFormDisplay::movePropertyPathViolationsRelativeToField()
  • last update 10 months ago
    30,136 pass
  • Status changed to Needs review 10 months ago
  • ๐Ÿ‡ง๐Ÿ‡ช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 10 months 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 make type: path validatable, then this issue does not need to bother with that. I'd be fine with doing that in this issue too of course. Or to defer that entirely to a follow-up, because it's technically not in scope. The bonus commit can even be reverted in its entirety, but that'd leave SiteInformationForm in a kind of Frankenstein state.

  • last update 10 months ago
    Build Successful
  • last update 10 months ago
    Build Successful
  • Status changed to Needs work 10 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Seems there may be a schema error?

  • last update 10 months ago
    Custom Commands Failed
  • last update 10 months ago
    Build Successful
  • last update 10 months 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.

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

    Suggested format for the message would resemble the following example -

    This form contains values that have been overridden:

    • 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 in settings.php, and sometimes in modules, so we recommend a link to the documentation that explains this (#82.5).

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

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

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

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

    CR needs to be updated as per new functionality.

  • last update 10 months ago
    30,106 pass, 44 fail
  • last update 10 months 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.

  • Pipeline finished with Success
    8 months ago
    #45349
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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.

  • Pipeline finished with Failed
    8 months ago
    #45496
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune


    Is this the expected behaviour?

  • Pipeline finished with Success
    8 months ago
    #45497
  • Status changed to Needs review 8 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune

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

  • Status changed to Needs work 8 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    ๐Ÿ“ Posted a detailed review!

  • ๐Ÿ‡ฎ๐Ÿ‡ณ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 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?

  • ๐Ÿ‡จ๐Ÿ‡ญ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 "."

  • Pipeline finished with Success
    8 months ago
    #49770
  • Pipeline finished with Failed
    8 months ago
    #49856
  • ๐Ÿ‡ง๐Ÿ‡ช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
    
  • Pipeline finished with Failed
    8 months ago
    #49933
  • Pipeline finished with Failed
    8 months ago
    #50652
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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.

  • Pipeline finished with Failed
    8 months ago
    #50702
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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:

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

      ๐Ÿ‘† When calling ::getEditable(), overrides are explicitly NOT loaded!

    2. The form you're referring to is a config entity form (\Drupal\Core\Entity\EntityForm), not a simple config form (\Drupal\Core\Form\ConfigFormBase). Only the latter has #config_target.
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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.

  • Pipeline finished with Canceled
    7 months ago
    #62045
  • Pipeline finished with Failed
    7 months ago
    #62046
  • Pipeline finished with Failed
    7 months ago
    #62060
  • Pipeline finished with Canceled
    7 months ago
    #49852
  • ๐Ÿ‡ซ๐Ÿ‡ฎ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 .

  • Pipeline finished with Failed
    7 months ago
    #63486
  • Pipeline finished with Failed
    7 months ago
    #63609
  • Pipeline finished with Success
    7 months ago
    #63611
  • Status changed to Needs review 7 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune

    Added test coverage and fixed test.

  • Status changed to Needs work 7 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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?

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune

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

  • Pipeline finished with Success
    7 months ago
    #64150
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

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

  • Status changed to Needs review 7 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune
  • ๐Ÿ‡ฌ๐Ÿ‡ง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.

  • Pipeline finished with Success
    7 months ago
    #64228
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

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

  • Status changed to Needs work 7 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium borisson_ Mechelen, ๐Ÿ‡ง๐Ÿ‡ช

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

    1. There is missing test coverage, (see #199 - #203.
    2. There is also an optimization in ConfigFormBase.
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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.

  • Pipeline finished with Failed
    6 months ago
    #82239
  • Pipeline finished with Failed
    6 months ago
    #82245
  • Pipeline finished with Failed
    6 months ago
    #82246
  • Pipeline finished with Running
    6 months ago
    #82399
  • Pipeline finished with Success
    5 months ago
    Total: 824s
    #84803
  • Pipeline finished with Success
    5 months ago
    Total: 591s
    #85303
  • Status changed to Needs review 5 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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 5 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #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.

  • Pipeline finished with Failed
    5 months ago
    Total: 519s
    #90253
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

    #218 ๐Ÿ› There is no indication on configuration forms if there are overridden values Needs work : 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.

  • Pipeline finished with Failed
    5 months ago
    Total: 608s
    #90597
  • Pipeline finished with Failed
    5 months ago
    Total: 179s
    #90616
  • Pipeline finished with Failed
    5 months ago
    Total: 176s
    #92765
  • Status changed to Needs review 5 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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.
  • Pipeline finished with Failed
    5 months ago
    Total: 1016s
    #93645
  • ๐Ÿ‡บ๐Ÿ‡ธ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 ๐Ÿ› There is no indication on configuration forms if there are overridden values Needs work : This is happening because there are two forms ClearCacheForm and PerformanceForm 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.

  • Pipeline finished with Success
    5 months ago
    Total: 483s
    #95533
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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 5 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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 5 months ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands Martijn de Wit ๐Ÿ‡ณ๐Ÿ‡ฑ The Netherlands
  • ๐Ÿ‡ฌ๐Ÿ‡ง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 5 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

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

  • Pipeline finished with Failed
    5 months ago
    Total: 469s
    #102486
  • Merge request !6758Add a permission โ†’ (Open) created by alexpott
  • Pipeline finished with Failed
    5 months ago
    Total: 484s
    #102651
  • ๐Ÿ‡ฌ๐Ÿ‡ง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.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    +1 for the fix. Good job figuring that out. ๐Ÿ‘

  • ๐Ÿ‡ฌ๐Ÿ‡ง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 5 months ago
  • ๐Ÿ‡ง๐Ÿ‡ช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 5 months ago
  • ๐Ÿ‡จ๐Ÿ‡ญ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 #234

    We discussed this on slack with @alexpott:

    alexpott: I think we should remove the value the disclosure and not have a permission :slightly_smiling_face:
    alexpott: And then add a permission for value disclosure in a followup
    alexpott: Because then what you are opting in for is way more obvious.
    
  • ๐Ÿ‡จ๐Ÿ‡ญ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 in settings.php to become visible in the user interface, such
    as API keys, tokens and other secrets."

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

    Also fwiw +1 for #238.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Sounds great โ€” then there is no risk ๐Ÿ‘

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

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

  • Pipeline finished with Running
    5 months ago
    #104379
  • Status changed to Needs review 5 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    I've pushed a commit that makes several improvements.

    1. It uses config target information to get all the information it needs to determine what is overridden. It no longer needs to flatten anything.
    2. It uses the label from schema (as opposed to the config key). This has a much higher chance of matching the text in the form and therefore being more obvious what's going on.
    3. It handles overrides individual elements of sequences more elegantly because it only cares about the config target map.
    4. It only displays information about what is overridden in the form - if the config file has something overridden that's not mapped to the form it does not display.
  • Pipeline finished with Success
    5 months ago
    Total: 512s
    #104398
  • ๐Ÿ‡ฌ๐Ÿ‡ง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.

  • Pipeline finished with Success
    5 months ago
    #104845
  • ๐Ÿ‡ฌ๐Ÿ‡ง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 ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

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

  • ๐Ÿ‡ฌ๐Ÿ‡ง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.
  • Pipeline finished with Success
    5 months ago
    Total: 504s
    #105105
  • ๐Ÿ‡ฌ๐Ÿ‡ง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.

  • Pipeline finished with Canceled
    4 months ago
    Total: 265s
    #108163
  • ๐Ÿ‡ฌ๐Ÿ‡ง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.

  • Pipeline finished with Canceled
    4 months ago
    Total: 388s
    #108170
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    Fixing the screenshot in the above comment.

  • ๐Ÿ‡จ๐Ÿ‡ฆ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.

  • Pipeline finished with Success
    4 months ago
    Total: 510s
    #108180
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom AaronMcHale Edinburgh, Scotland

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

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom 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.

  • ๐Ÿ‡ฌ๐Ÿ‡ง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:

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

    omkar.podey โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Success
    4 months ago
    Total: 569s
    #118020
  • ๐Ÿ‡ฌ๐Ÿ‡ง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:

    1. Ctrl+click
    2. Middle click
    3. Right click, click to open in a new tab
  • Pipeline finished with Canceled
    3 months ago
    #157315
  • Status changed to Needs review 3 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡ง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.

  • Pipeline finished with Success
    3 months ago
    #157320
  • ๐Ÿ‡ฌ๐Ÿ‡ง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...

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom AaronMcHale Edinburgh, Scotland

    Add comments

  • ๐Ÿ‡ญ๐Ÿ‡บ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 3 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡ง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 States smustgrave

    Does this still need usability?

  • ๐Ÿ‡ฌ๐Ÿ‡ง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 29 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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 Canberra

    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 5 days ago
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone New Zealand

    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 5 days ago
  • ๐Ÿ‡ฌ๐Ÿ‡ง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 3 days ago
  • ๐Ÿ‡ฌ๐Ÿ‡ง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 3 days ago
  • ๐Ÿ‡ฌ๐Ÿ‡ง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.

  • Pipeline finished with Success
    3 days ago
    #220876
    • catch โ†’ committed efac917d on 10.4.x
      Issue #2408549 by alexpott, narendraR, yash.rode, kunal.sachdev, lauriii...
    • catch โ†’ committed 64f70652 on 11.0.x
      Issue #2408549 by alexpott, narendraR, yash.rode, kunal.sachdev, lauriii...
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States shaal Boca Raton, FL
  • ๐Ÿ‡ฌ๐Ÿ‡ง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 3 days ago
    • catch โ†’ committed 1602eb31 on 11.x
      Issue #2408549 by alexpott, narendraR, yash.rode, kunal.sachdev, lauriii...
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States apotek

    Gloria in excelsis mundi !!! Thank you to everyone for this epic improvement.

Production build 0.69.0 2024