Problem/Motivation
There are 2 main problems in here:
-
Initializing the logger component is heavy, as it's doing a raw DB query on every request.
On component constructor it it's always getting an editable version of the configuration. This means it's always doing the RAW db query to fetch it, essentially bypassing Drupal's cache for config entities.
- As a follow-up, caused from that. Drupal 8 config overwrite system can not be utilized to change the settings passed on the component, as it's using the values from DB that are passed in, preventing the possibility to have different configs on different environments through settings.php overwrites.
Proposed resolution
Switch to the D8 recommended $config_factory->get('...')
Remaining tasks
Patch... Commit...
User interface changes
None.
API changes
None - bug-fix.
Data model changes
None.
Release notes snippet
- Resolved a bug preventing config overwrites to work.
- Improved performance.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | jsonlog-3028292-4.patch | 2 KB | ndobromirov |
Comments
Comment #2
ndobromirov commentedHere is a patch that will resolve both issues mentioned in the description.
Comment #3
ndobromirov commentedComment #4
ndobromirov commentedDropped all writes to the config from the Jsonlog class.
Tweaks on some inline documentations.
Comment #5
ndobromirov commentedMissed the file...
Comment #6
ndobromirov commentedIf defaults should be computed and stored in the configuration as they currently are on the first request. This should happen either on logs config form save OR somewhere in hook_requirements. This is to prevent this happening in the critical path, as well as dropping the need for an editable config to be injected.
Any feedback will be appreciated. I think it's safe to have this things implemented in a follow-up either way to not block this from being committed.
Comment #7
jacobfriis commented@ndobromirov
Great effort! :-)
I think hook_requirements is pointing in the right direction.
The module cannot depend on somebody submitting a configuration form, or likewise.
It must work as-is, when installed/enabled.
How about hook_install?
It's more straight to the point, and hook_requirements is so Drupal idiosyncratic (makes no sense unless one was a core developer for D5-something ;-)
Comment #8
jacobfriis commentedCannot be critical, because doesn't ruin anything (but performance).
Comment #9
lorenzs commentedHi; I think this could be an improvement idd; I'll try to test this next week (bit of a busy schedule now).
Thx!
Comment #10
ndobromirov commentedI think install is the best direction to calculate defaults.
Comment #12
lorenzs commentedPatch was applied and tested on a local site; Together with another fix and cleanup of deprecated calls it is now available on dev branch.
Comment #13
lorenzs commentedIn release 8.x-1.2