Problem/Motivation

Inspired by #3453216: Clean up outdated mentions of prepareLegacyRequest I wondered why we explicitly have to call StreamWrapperManager::register() in multiple places - is it enough to register the stream wrappers in the constructor?

Steps to reproduce

Proposed resolution

Call $this->register() from the constructor
Remove all other calls
See what breaks

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3583911

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 !15398

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

Slightly surprised this is green, but it seems like it's working; maybe in a followup we can do the same for ModuleHandler::loadAll()?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Also maybe we move register() into the constructor directly, does it need to be a public method at all?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3414627: Convert StreamWrapperManager to use a service locator

Just realised the reason this probably wasn't done before is that prior to #3414627: Convert StreamWrapperManager to use a service locator the stream wrappers were not available in the constructor, they were injected shortly afterwards.

smustgrave’s picture

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

Seems like a nice cleanup. Not suer if a small CR is needed incase contrib needs to update? May be overkill.

catch’s picture

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

This looks good but I think we should mark ::register() as @internal with a note it will become protected in 13.0.0 or similar, if we want to do that in a follow-up, that's OK but let's open the follow-up if so.

Or reading @longwave's comment a second time, if we inline the logic, we can deprecate it now for removal with no replacement, in which case we could just do that there.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I do also wonder about the comment in ModuleInstaller about drush, perhaps this needs manual testing with drush because I guess this is quite hard to write an automated test for.

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

cmlara’s picture

cmlara
he/him
commented
Related issues: +#3256822: StreamWrapperManager Service has public methods not on interface.

#3256822: StreamWrapperManager Service has public methods not on interface. may become unnecessary if this is completed.