Problem/Motivation

As discussed in #3514491: [meta] Replace lazy service proxy generation with service closures our lazy classes implementation isn't ideal.

There seems to be no good reason that ConfigInstaller or ModuleInstaller are marked lazy; as far as I can see they aren't injected into any other service anyway. ModuleInstaller is largely used by tests and various forms in system.module, where performance isn't critical. ConfigInstaller is loaded by ModuleInstaller and ThemeInstaller and immediately used. Neither of them have a chain of dependent services that seem like they wouldn't be instantiated anyway.

Steps to reproduce

Proposed resolution

Remove the lazy flag and the proxy class for both services.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3570570

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

longwave created an issue. See original summary.

longwave opened merge request !14563

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Remove lazy declaration and proxy class for ModuleInstaller » Remove lazy declaration and proxy class for ConfigInstaller and ModuleInstaller
Issue summary: View changes

Let's expand scope to the ConfigInstaller as well.

nicxvan’s picture

nicxvan commented

Is there a way to tests those or some documentation I can read?

nicxvan’s picture

nicxvan commented

This does what it says, I found this: https://symfony.com/doc/current/service_container/lazy_services.html

It's green.

I want to pull this down and confirm where is injected, but I think the issue it's correct.

Once I do that I think this is ready.

nicxvan’s picture

nicxvan commented

Only found one call in here: https://git.drupalcode.org/project/drupal/-/blob/main/core/modules/langu...

I'm not sure if this is a concern.

Also blockhooks but that is in themes installed.

Everything else is in install or loading so I think it is fine to remove.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I think if there are performance issues identified after this then we solve them at the point where they are injected by using service closures (as we've done in other issues).

The old lazy proxy code seems fundamentally broken for most if not all cases and I'd like to try and remove it entirely.

nicxvan’s picture

nicxvan commented

In general I agree, but since it's just potentially one service shouldn't we just check where that is injected and see if we need the service closure? Language gets loaded in a lot of places I'm not sure if this tags along.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

config.installer is only injected into theme_installer, which itself is only injected into the ThemeController and the experimental theme form. Otherwise it is only instantiated via \Drupal::service(). All other uses of config.installer are via \Drupal::service().

module_installer is only injected into ConfigImporterFactory (which itself is only used in the config import admin forms) and various module-related forms. All other uses are via \Drupal::service().

To me none of these are on the critical path; calls via \Drupal::service() generally mean that the service is going to be immediately used. We only need to concern ourselves with a lazy proxy technique when something is injected into another service, but then rarely used. Even then, in both these cases, while the services have a bunch of dependencies, there's nothing that stands out as particularly expensive to instantiate - many dependencies (typed config, event dispatcher, kernel, database connection) will likely already be instantiated anyway.

ModuleInstaller previously was more expensive to instantiate as all the uninstall validators were constructed and injected, but since #3432595: Use a tagged service iterator for uninstall validators instead of individual lazy proxies that has been neutralised as well.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Yeah, as I mentioned I was concerned about where config install was called in installLanguageOverrides, but it looks like that is only called in the configurable language form too.

I think you're right, this is good to go.

catch closed merge request !14563

catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and cherry-picked to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed 433c1f97 on 11.x 
    task: #3570570 Remove lazy declaration and proxy class for...

  • catch committed 94da3be3 on main 
    task: #3570570 Remove lazy declaration and proxy class for...

Status: Fixed » Closed (fixed)

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