Problem/Motivation

Drupal has mechanisms to override configurations: https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-...
However, the charts module is bypassing all those mechanisms by always loading the configuration with the "getEditable()"-method.

Steps to reproduce

Try any of the override mechanisms from the documentation.

Proposed resolution

Use the "config"-service for loading the configuration in all cases where read-only is sufficient, i.e. during rendering and within requirement checks. For example:

-  $config = \Drupal::service('config.factory')->getEditable('charts.settings');
+  $config = \Drupal::config('charts.settings');

User interface changes

None.

API changes

Support for Drupal's configuration override API.
No changes to the existing API of the module.

Data model changes

None.

Issue fork charts-3399072

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

lars.stiebenz created an issue. See original summary.

lars.stiebenz’s picture

StatusFileSize
new3.39 KB

nikathone’s picture

Version: 5.0.8 » 5.0.x-dev
Status: Active » Needs review

Let see if the tests will pass. Also is there a way you can provide a sample code of how the override is done?

lars.stiebenz’s picture

Version: 5.0.x-dev » 5.0.8
Status: Needs review » Active

Simple Code for testing (placed in setting.php or settings.local.php):

$config['charts.settings']['advanced']['requirements']['cdn'] = FALSE;

More advanced options are in the documentation.

Preconditions:
- CDN should be allowed in the advanced configurations.
- Used library should be any but Google (that one lacks the config-check).
- The library itself should not be installed for the test.

Testing:
Place the code and go to /admin/reports/status.

Results:
Without patch: CDN-warning appears.
With patch: CDN-warning does not appear.

Background:
The overrides should work for any of the configurations. The CDN-Option is relatively simple to test since it shows almost directly in the status report.

nikathone’s picture

Version: 5.0.8 » 5.0.x-dev
Status: Active » Needs review

If the test pass which I think it will, this should be committed. Also @lars.stiebenz please don't change the version because that's where the commit will be applied to. Also let see if we can get another person than me and you to RTBC. Thanks for the patch.

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

andileco’s picture

Status: Needs review » Fixed

Looks great. Thank you!

Status: Fixed » Closed (fixed)

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