Problem/Motivation

In general, it is discouraged to read config in a service constructor as it might be initialized when that specific config isn't actually needed.

This is especially true for BanIpManager, because it' in the super-critical-path (a middleware), that means it's instantiated even on a page cache it.

The seconds configuration is only needed if an IP is actually being banned.

Steps to reproduce

Proposed resolution

Move the config read into the banIp() method.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork auto_unban-3501358

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:

About issue forks

Comments

berdir created an issue. See original summary.

berdir opened merge request !4

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

Also fixed the default value (get() does not have a second argument), although this indicates that this isn't actually a concern and could be removed as well as it never worked.

And I fixed a deprecation on the key() method, this was already deprecated in 8.0 but Drupal 11 actually removes the old path now.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Pushed a fix, was missing the assignment of configFactory completely now.

  • douggreen committed e012561a on 1.0.x authored by berdir 
    Resolve #3501358 "Do not read config in BanIpManager::__construct()"
douggreen’s picture

douggreen commented
Status: Needs review » Fixed

Thanks!

Status: Fixed » Closed (fixed)

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