Problem/Motivation

In a persistent worker, such as FrankenPHP in worker mode, Settings::getInstance() retains the first instance across requests. Replacing container services does not clear this static. Because the instance retains its original current-user proxy and other dependencies, Settings::get() can read another user's settings. The dependency retention may also keep an old container in memory.

Steps to reproduce

  1. In one PHP process, configure two users with different admin theme settings and enable their overrides.
  2. Resolve Settings through the static accessor using the first user's services.
  3. Replace the container with one configured for the second user.
  4. Call the accessor again. It returns the original instance and reads the first user's settings.

Proposed resolution

Resolve Settings through the class resolver on each call instead of caching it in a function-local static.

AI Disclosure

AI assistance (Astra and Opus 5.5) used to draft the IS and prepare the MR.

Issue fork drupal-3626158

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

ptmkenny created an issue. See original summary.

ptmkenny opened merge request !17257

ptmkenny’s picture

ptmkenny commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented

Understand this is needed for FrankenPHP but won't this impact those who aren't running that.

ptmkenny’s picture

ptmkenny commented

Is there a standard core performance test or something I can run to measure the impact? It's pretty easy to switch back and forth between frankenphp and nginx on ddev, but I'm not sure what the best way would be to measure the impact of this change.

smustgrave’s picture

smustgrave commented

Let me try and get ya answer!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Where is this method called from? Wondering if we can just optimise/deprecate this method away entirely or do it in a better way.

I don't have any performance concerns about this change given it is only a small part of the admin theme, just it seems a bit odd in the first place that we even need this.

ptmkenny opened merge request !17486

ptmkenny’s picture

ptmkenny commented

In core, this method is only called within default_admin. I opened a new MR (deprecate_settings_getinstance) that deprecates the Settings::getInstance() and instead injects the class resolver into the hook classes and calls it directly when it cannot be injected. It's not much code and all the tests pass, so that's one potential approach.

If this looks OK, I can write a change record.

smustgrave’s picture

smustgrave commented

@longwave thoughts?