Problem/Motivation

In the settingsForm which is a child of ConfigFormBase, \Drupal\siteimprove\Form\SettingsForm::buildForm is not access config correctly for the settings form as described in Working with Configuration Forms

It is currently calling the configFactory to get the config. Which is fine except is it allow overriden values to be saved in config when depending on how these are set these could be loaded from secrets depending on the system.

Steps to reproduce

in yours settings.php add the following lines.

$config['siteimprove.settings']['api_username'] = 'xxx';
$config['siteimprove.settings']['api_key'] = 'xxx';

Proposed resolution

on the settings form on line 108 which is

    $config = $this->configFactory->get('siteimprove.settings');

this needed to be changed to

    $config = $this->config('siteimprove.settings');

Now the value which is stored in config will display and not the overriden values.

For the API checked we will need to get the values from the real config factory so that they can be run.

CommentFileSizeAuthor
#4 after.png44.82 KBkeshavv
#4 before.png46.7 KBkeshavv
#4 settings.png40.72 KBkeshavv
#2 3381807-2.patch516 bytesmukhtarm
Command icon Show commands

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

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

Comments

gordon created an issue. See original summary.

mukhtarm’s picture

StatusFileSize
new516 bytes

please review the patch

bhupendra_raykhere’s picture

Status: Active » Needs review
keshavv’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new40.72 KB
new46.7 KB
new44.82 KB

Confirmed that the given solution works.

Updated settings.php file
Settings

Before patch
Before

After patch
After

gordon’s picture

Status: Reviewed & tested by the community » Needs work

Hi

Thanks for this but this change then breaks the republish check.

So in my clients case we override the user and api key and set these fields to ‘’ in config. So when loading this page it will do the checks with user and api key set to null were it should be using the overridden value.

There needs to be a change further down where the check will be done not using $config but using a a different version of the config using configFactory() and then the creds will be loaded correctly if they are overridden.

Thanks for the fast work.
Gordon.

gordon’s picture

Status: Needs work » Needs review

Hi,

I have changed this into an issue fork and added the necessary changes so that when it does the API check it will use the real config which includes any overrides.

gordon’s picture

I have removed the original patch.

bartvig’s picture

We have a conflict with https://www.drupal.org/project/siteimprove/issues/3308624

In that issue, the code was specifically changed from $config = $this->config('siteimprove.settings'); to $config = $this->configFactory->get('siteimprove.settings'); to allow config overrides.

bartvig’s picture

Oh, I think I understand the issue now. You don't want the overridden value to be displayed in the settings form, but the overridden should still be used in API calls.

  • bartvig committed 614c2ea4 on 2.0.x authored by gordon
    Issue #3381807: Settings form is not accessing config correctly.
    
tlyngej’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.