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

CommentFileSizeAuthor
#4 streamwrappermanager-3256822-4.patch3.01 KBcmlara

Issue fork drupal-3256822

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

cmlara created an issue. See original summary.

cilefen’s picture

How about a new interface containing only these methods?

cmlara’s picture

I’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.

cmlara’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new3.01 KB

Attached 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.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

This 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.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

bramdriesen’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Really not sure we can write a test for an interface. All tests are passing and code style is fixed.

smustgrave’s picture

Status: Needs review » Needs work

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

bramdriesen’s picture

Status: Needs work » Needs review

Fixed the two remarks, English is not my native language 😇

Don't really think we need a CR as it was also internal before?

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Threads 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.

catch’s picture

Status: Reviewed & tested by the community » Needs work

I 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...

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.