Problem/Motivation
Currently StreamWrapperManager has addStreamWrapper, register() and unregister() documented as
*
* Internal use only.
*without the @internal flag.
These functions are necessary for Core to bootstrap however they are generally not necessary for most modules likely justifying them generally being considered internal.
However since the StreamWrapperManager is a Drupal Service it is possible to decorate with a Service Decorator. Any module wanting to decorate the StreamWrapperManager must implement these methods or core will be unable to function.
Possible reasons to Decorate the StreamWrapperManager service that I have seen are:
- Registering a StreamWrapper for use as a supported stream for use in Managed Files but not overriding the protocol in PHP (http/https in Remote Stream Wrapper as an example)
- Adding StreamWrappers when schemes are not known during the Compiler Pass as they require data from Config Entities (being implemented in S3FS)
With these functions not on the interface they appear (by policy) to be subject to change in minor point releases without any deprecation window making decorating the service appear 'risky'.
Steps to reproduce
See core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
Proposed resolution
Add addStreamWrapper, register(), unregister() to a new interface.
Document they are generally not to be called and are on interface solely for Drupal Core and Service Decorators.
Remaining tasks
User interface changes
None
API changes
New interface created for existing methods.
Data model changes
None Expected.
Release notes snippet
TBD
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | streamwrappermanager-3256822-4.patch | 3.01 KB | cmlara |
Issue fork drupal-3256822
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 #2
cilefen commentedHow about a new interface containing only these methods?
Comment #3
cmlaraI’m good with it being a new interface.
I can probably whip together a patch later today to do that unless someone else beats me to it.
Comment #4
cmlaraAttached patch adds CoreStreamWrapperManagerInterface to hold these three functions.
Left the "Internal Use Only" text in the method documentation to make it clear that these are primarily for use by core along with a note in the class docblock.
Comment #7
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
This could use a test case to show the issue.
Comment #11
bramdriesenReally not sure we can write a test for an interface. All tests are passing and code style is fixed.
Comment #12
smustgrave commented2 small comments on the MR. Will keep an eye out
If you are another contributor eager to jump in, please allow the original poster @bramdriesen at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
Comment #13
bramdriesenFixed the two remarks, English is not my native language 😇
Don't really think we need a CR as it was also internal before?
Comment #14
smustgrave commentedThreads have been resolved.
Read the documentation and reads fine to me and will agree with the CR not being needed so closed that thread too.
Think this one is ready.
Comment #15
catchI think we should be able to add these methods to the existing interface under the 1-1 rule.
See https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...