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.

StreamWrapperManager is container aware because it needs to retrieve stream wrapper services by ID.

Instead of injecting the entire container we can inject a service locator that only knows about stream wrapper services.

Steps to reproduce

Proposed resolution

Inject a service locator from RegisterStreamWrappersPass.

Convert StreamWrapperManager to accept a service locator instead of being fully container aware.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3414627

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’s picture

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

longwave opened merge request !6160

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Thank you, looks it needs CR but up to commiter

dpi’s picture

dpi
Location Perth, Australia
commented

Put together a similar locator in #3408433: Use a service locator to group hooks services together to ease future privatization and lazy loading recently so can say LGTM.

Only one optional hyper- nit. Leaving as RTBC.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Fixed the nit, added a CR: https://www.drupal.org/node/3414687

  • catch committed 3998f8d0 on 11.x 
    Issue #3414627 by longwave, andypost, dpi: Convert StreamWrapperManager...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Nifty, didn't realise this had been possible since Symfony 3.3.

Committed/pushed to 11.x, thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented

Please close MR

catch closed merge request !6160

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Parent issue: #3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core » #3397519: [META] Reduce use of ContainerAware classes where possible
Related issues: +#3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core

Status: Fixed » Closed (fixed)

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

cmlara’s picture

cmlara
he/him
commented

Note for those who come across this issue in the future:

This did cause a break in D10.3 when streamWrappers were registered as part of a second compiler pass

Service "stream_wrapper.myscheme" not found: the container inside "Drupal\Core\StreamWrapper\StreamWrapperManager" is a smaller service locator that only knows about the "stream_wrapper.assets", "stream_wrapper.private", "stream_wrapper.public" and "stream_wrapper.temporary" services.

In my case (s3fs) it appears it may be easy enough to fix by moving to ServiceProviderInterface::register() however it is a refactoring on minor upgrade.

itsbakiya’s picture

itsbakiya commented

Facing same issue we #13.

Any solution to fix this

cmlara’s picture

cmlara
he/him
commented

#3461207: D10.3 StreamWrapperManager change to Service Locators breaks with second compiler pass was where s3fs fixed this, basically you have to make sure you register your services before the first compiler pass completes and the closures are generated hence the move into register().