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.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3399072-support-config-overrides-1.patch | 3.39 KB | lars.stiebenz |
Issue fork charts-3399072
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
lars.stiebenz commentedComment #4
nikathoneLet see if the tests will pass. Also is there a way you can provide a sample code of how the override is done?
Comment #5
lars.stiebenz commentedSimple Code for testing (placed in setting.php or settings.local.php):
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.
Comment #6
nikathoneIf 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.
Comment #9
andileco commentedLooks great. Thank you!