Problem/Motivation

In #3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core we are trying to reduce the use of ContainerAwareTrait as Symfony has deprecated it.

CacheFactory and ChainedFastBackendFactory are container aware because they need to retrieve cache backend services by ID.

Because these are low-level services they sometimes need to be accessed at container build time, which means we can't inject a service locator that only contains cache bins; instead it appears the simplest solution is just to continue injecting the entire container.

Steps to reproduce

Proposed resolution

Add setContainer() methods directly to the services.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3415938

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:

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave’s picture

Status: Needs review » Needs work
longwave’s picture

This also fails in ServiceProviderTestServiceProvider:

    // Make sure a cached service can be also called in a service provider.
    // https://www.drupal.org/project/drupal/issues/2363351
    /** @var \Drupal\Core\Extension\ModuleHandlerInterface $module_handler */
    $module_handler = $container->get('module_handler');

The module handler ultimately depends on the cache factory, but because of the way the service locator is injected we end up with this error:

1) Drupal\KernelTests\Core\DrupalKernel\DrupalKernelTest::testCompileDIC
Symfony\Component\DependencyInjection\Exception\RuntimeException: Constructing service "cache_factory" from a parent definition is not supported at build time.

As we need cacheable services accessible at container build time, not sure there is a way around this; we might have to inject the entire container instead.

longwave’s picture

Title: Convert CacheFactory to use a service locator » Remove ContainerAwareTrait from CacheFactory and ChainedFastBackendFactory
Issue summary: View changes
Status: Needs work » Needs review

ChainedFastBackendFactory follows the same pattern. Have not figured out how to work around the issue in #5, so the simplest solution is just to continue injecting the container into both factory services.

spokje’s picture

Status: Needs review » Reviewed & tested by the community

I also don't see a way around injecting the whole container in both services.

Tests are green, code changes make sense and are minimal, don't think this needs a CR?

RTBC.

longwave’s picture

Yeah no CR is needed, no API is changing here really, we are just copying the method and property directly into the class.

catch made their first commit to this issue’s fork.

  • catch committed 66345ac2 on 10.3.x
    Issue #3415938 by longwave, Spokje, taraskorpach: Remove...

  • catch committed 78ec28a9 on 11.x
    Issue #3415938 by longwave, Spokje, taraskorpach: Remove...

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks! Also can't see any need for a CR here.

andypost’s picture

Please close MR

Status: Fixed » Closed (fixed)

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