Problem/Motivation

There are 2 main problems in here:

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

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

Comments

ndobromirov created an issue. See original summary.

ndobromirov’s picture

Status: Needs work » Needs review
StatusFileSize
new682 bytes

Here is a patch that will resolve both issues mentioned in the description.

ndobromirov’s picture

Issue summary: View changes
ndobromirov’s picture

Dropped all writes to the config from the Jsonlog class.
Tweaks on some inline documentations.

ndobromirov’s picture

StatusFileSize
new2 KB

Missed the file...

ndobromirov’s picture

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

jacobfriis’s picture

@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 ;-)

jacobfriis’s picture

Priority: Critical » Normal

Cannot be critical, because doesn't ruin anything (but performance).

lorenzs’s picture

Hi; I think this could be an improvement idd; I'll try to test this next week (bit of a busy schedule now).
Thx!

ndobromirov’s picture

I think install is the best direction to calculate defaults.

  • lorenzs committed 3d37d48 on 8.x-1.x
    Issue #3028292 by ndobromirov, lorenzs: No longer using getEditable in...
lorenzs’s picture

Version: 8.x-1.1 » 8.x-1.x-dev
Assigned: Unassigned » lorenzs

Patch was applied and tested on a local site; Together with another fix and cleanup of deprecated calls it is now available on dev branch.

lorenzs’s picture

Version: 8.x-1.x-dev » 8.x-1.2
Status: Needs review » Fixed

In release 8.x-1.2

Status: Fixed » Closed (fixed)

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