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.
Merge request link
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3415938
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
Comment #3
longwaveComment #4
longwaveComment #5
longwaveThis also fails in ServiceProviderTestServiceProvider:
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:
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.
Comment #6
longwaveComment #9
longwaveChainedFastBackendFactory 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.
Comment #10
spokjeI 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.
Comment #11
longwaveYeah no CR is needed, no API is changing here really, we are just copying the method and property directly into the class.
Comment #16
catchCommitted/pushed to 11.x and cherry-picked to 10.3.x, thanks! Also can't see any need for a CR here.
Comment #17
andypostPlease close MR