Updated: Comment #57

Problem/Motivation

Stream wrappers are yet another info hook/callback pairing, and those are moving to annotated plugins.
In addition, it helps clean up and modernize file.inc.

Proposed resolution

Rewrite stream wrappers to be plugins (they are already PSR-0 classes)
Leave the procedural wrappers intact for now.

Remaining tasks

- Decide on discovery mechanism (Annotations+Cache or Hook). Just annotations seem to introduce performance regression, while usage of cache brings some new problems that would need more work to resolve. See #48 for more info on that.

User interface changes

N/A

API changes

If we decide to use annotated discovery:
- hook_stream_wrappers() is gone, and \Drupal\Core\StreamWrapper\StreamWrapperInterface classes must be annotated and live in a StreamWrapper directory

Files: 
CommentFileSizeAuthor
#150 stream-wrappers.150.patch71.06 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch stream-wrappers.150.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#150 interdiff.txt4.89 KBlarowlan

Comments

Assigned:Unassigned» tim.plunkett
Status:Active» Needs review
StatusFileSize
new21.75 KB
FAILED: [[SimpleTest]]: [MySQL] 56,719 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Here's a start to see if it even works.
If it does, I'll see about moving file_get_stream_wrappers() to be a method on the manager.

This might benefit from some of msonnabaum's decoupling in #2005716: Promote EntityType to a domain object, since we only want this for discovery, and the StreamWrapper\Plugin\StreamWrapper namespacing is bizarre.

Title:Convert hook_stream_wrappers() to plugin system?Convert hook_stream_wrappers() to plugin system
StatusFileSize
new2.45 KB
new24.19 KB
PASSED: [[SimpleTest]]: [MySQL] 56,954 pass(es).
[ View ]

StatusFileSize
new24.19 KB
FAILED: [[SimpleTest]]: [MySQL] 56,728 pass(es), 0 fail(s), and 4 exception(s).
[ View ]

Let's see if this breaks anything:

diff --git a/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
index 0410c2e..6d6526f 100644
--- a/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
+++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
@@ -40,7 +40,7 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
     parent::__construct('StreamWrapper', $namespaces, array(), 'Drupal\Core\Annotation\StreamWrapper');
     $this->alterInfo($module_handler, 'stream_wrappers');
-    //$this->setCacheBackend($cache_backend, $language_manager, 'stream_wrappers');
+    $this->setCacheBackend($cache_backend, $language_manager, 'stream_wrappers');
   }
}

StatusFileSize
new18.23 KB
new40.93 KB
FAILED: [[SimpleTest]]: [MySQL] 55,477 pass(es), 370 fail(s), and 2,460 exception(s).
[ View ]

This worked nicely.

StatusFileSize
new2.34 KB
new40.45 KB
FAILED: [[SimpleTest]]: [MySQL] 56,962 pass(es), 6 fail(s), and 1 exception(s).
[ View ]

Since stream wrappers need to be registered on every request, we don't want persistant caching, the static caching is equivalent to what HEAD does.

Issue tags:-Plugin-conversion

#9: stream-wrappers-2028109-9.patch queued for re-testing.

Issue tags:+Plugin-conversion
StatusFileSize
new41.29 KB
PASSED: [[SimpleTest]]: [MySQL] 56,536 pass(es).
[ View ]
new853 bytes

We need an empty StreamWrapperManager during installation.

StatusFileSize
new1.03 KB
new42.32 KB
FAILED: [[SimpleTest]]: [MySQL] 55,415 pass(es), 111 fail(s), and 94 exception(s).
[ View ]

Per chx, we can remove yet another special case from UpdateModuleHandler.

StatusFileSize
new652 bytes
new42.49 KB
FAILED: [[SimpleTest]]: [MySQL] 55,296 pass(es), 113 fail(s), and 94 exception(s).
[ View ]

Silly mistake with the fall-through on that switch/case

StatusFileSize
new1.19 KB
new1.37 KB
new42.66 KB
PASSED: [[SimpleTest]]: [MySQL] 56,906 pass(es).
[ View ]

Seriously, I hate switch/case sometimes. Going out on a limb and adding some line breaks for each return statement since there are no break statements.

Just cross-referencing #2032369-3: _update_8003_field_create_field() relies on field schemas never changing and plugins being available. Right now, plugin discovery during upgrade path works, but I expect that we have to lock that down as well as we could run into the exact same troubles there. We're just not experiencing that... yet.

Not a stopper for this, but something we'll have to look into sooner or later.

Dear hook_*_info(@tim.plunkett) killer here is a major task related to this issue #1308152: Add module://, theme:// and profile:// stream wrappers to access system files. Can we include that patch in this issue or just rewrite it when this is done?

Priority:Normal» Critical

Issue tags:-Approved API change

The person who told me to tag this was misinformed.

StatusFileSize
new42.78 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Rerolled!

StatusFileSize
new9.88 KB
new42.24 KB
FAILED: [[SimpleTest]]: [MySQL] 57,150 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

We can put plugins wherever we want now!

StatusFileSize
new2.08 KB
new40.49 KB
PASSED: [[SimpleTest]]: [MySQL] 57,498 pass(es).
[ View ]

Stupid mistakes.

#29: stream-wrappers-2028109-29.patch queued for re-testing.

Issue tags:+blocker

Issue summary:View changes

Updated issue summary.

StatusFileSize
new40.49 KB
PASSED: [[SimpleTest]]: [MySQL] 57,828 pass(es).
[ View ]

Reroll for patch fuzz, and I updated the issue summary.

+++ b/core/includes/file.inc
@@ -184,78 +155,33 @@
+ * @return string|bool
  *   Return string if a scheme has a registered handler, or FALSE.

Would you mind if we change all this bool return nonsense to a proper NULL throughout this patch?

If you do mind, I'll just shut up and rtbc :)

I do mind, this is really important to me to get in, and I don't see that as something that can't be a follow-up. It is, however, less invasive than #20 :)

Status:Needs review» Reviewed & tested by the community

Ok then, let's do this.

+++ b/core/core.services.yml
@@ -475,6 +475,9 @@ services:
+  plugin.manager.stream_wrapper:
+    class: Drupal\Core\StreamWrapper\StreamWrapperManager
+    arguments: ['@container.namespaces', '@module_handler']

Stream wrappers are registered on every request, yet this doesn't include persistent caching. #9 says this is equivalent to HEAD, but we know that AnnotatedClassDiscovery is much slower than calling an info hook. We could probably use some profiling here.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -512,6 +512,7 @@ protected function buildContainer() {
+      $namespaces['Drupal\\' . $parent_directory] = DRUPAL_ROOT . '/core/lib';
...
+++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
@@ -0,0 +1,169 @@
+    parent::__construct('StreamWrapper', $namespaces, array(), 'Drupal\Core\Annotation\StreamWrapper');

I'm not aware of any other plugin manager currently in HEAD that doesn't follow the convention of "Plugin/..." for where plugins are located. #2 explains why the exception is being made here, but I'm concerned about proliferating magic top-level directories within modules. I'm ok with "Plugin" and "Tests" being reserved for autodiscovery, since those are central concepts in Drupal, and for the same reason, I like the idea of promoting "Entity" to that level in #2005716: Promote EntityType to a domain object, but StreamWrapper seems too peripheral to warrant that promotion.

+++ b/core/modules/file/tests/file_test/lib/Drupal/file_test/StreamWrapper/DummyRemoteStreamWrapper.php
@@ -0,0 +1,31 @@
+class DummyRemoteStreamWrapper extends \Drupal\Core\StreamWrapper\PublicStream {

In the process of moving this class, we seemed to lose the use and instead inline the full class name of the base class. Why? I don't think we do this anywhere else in HEAD.

Because this issue is prioritized as critical, and has the "blocker" tag, I'm leaving it at RTBC for a core committer to decide if any of this blocks commit. I'm ok with handling any of these items in follow ups.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs profiling

The extends \Drupal\Core\StreamWrapper\PublicStream was just PHPStorm being weird when moving namespaces around.

I agree this needs some profiling, and likely a cache.

----
Nothing else in core does that because I *just* added that ability. I wrote it for this patch, actually, not for Entity (which also stands to gain by utilizing this).

Not putting Plugin/ in the namespace meant that the current streamwrapper classes didn't have to move at all.
Unfortunately, if we make it Plugin/Streamwrapper, then
Drupal\Core\StreamWrapper\PublicStream
would become
Drupal\Core\StreamWrapper\Plugin\StreamWrapper\PublicStream

With this patch as is, no one outside of the annotation class or the manager needs to know stream wrappers are plugins.

We have wrapper functions and annotated classes, when interacting with them the string "plugin" is no where in sight.
I don't see any reason to bludgeon the user over the head with "HI I'M A PLUGIN" for no real gain but more complex namespaces.

With this patch as is, no one outside of the annotation class or the manager needs to know stream wrappers are plugins.

Not exactly. Every module that implements a stream wrapper needs to know that simply putting a class into the module's lib/StreamWrapper directory (once we switch to PSR-4) makes it found and registered. Which is not how event subscribers, or services, or controllers, or any other kind of class works. Only tests and plugins (and maybe eventually, entity types) work that way. So we'd basically be explaining this to module devs as "stream wrappers are not plugins in the sense that they don't live in the Plugin directory, but they're like plugins in the sense of being autodiscovered annotated classes managed by a manager class that implements PluginManagerInterface". I don't really see what's so special about stream wrappers that makes us want to explain them that way, instead of just calling them regular plugins.

I don't really see what's so special about stream wrappers that makes us want to explain them that way

Well, maybe what's special about them is that they are a concept defined by PHP more so than by Drupal. If others agree with that reasoning, I can go along with it too.

I don't plan on using the Plugin/ directory in a single contrib module. If I could have moved ALL of them in that other issue, I would have.

This diff http://drupalcode.org/project/drupal.git/commitdiff/d88a2be#patch1 shows what shouldn't have happened. We should have left the managers the same and just moved everything out of the Plugin directories once and for all.

I don't plan on using the Plugin/ directory in a single contrib module.

I'm sorry for further derailing this issue, but this comment is a bit troubling. So, if a contrib module (A) defines a plugin type 'List' and another contrib module (B) puts a random class in its lib directory, let's say lib/List/RandomClass.php, that class won't be recongnized as a plugin, obviously, because it won't have the annotation. But what will a poor user understand from all this while trying to see examples of 'List' plugins and looking into that totally unrelated class from contrib module B?

If you name your plugin type List, I will file a bug report in your module. Whatever. This is super offtopic, and I can switch to moving everything into an extra two directories for "consistency" if that's what it takes.

Dude, chill, I know it's offtopic and I already said sorry for that, but so was your comment :) And I was genuinely interested in your answer..

I think you're focused on hating the wrong thing here, it should be redirected to the usage of PSR-0 in modules, not the Plugin directory. Plugins in D7 were kind of a shiny thing for a contrib to provide, and they had a nice flexibility for declaring their discovery by any implementing module, but they will be *super* common in D8 and without that flexibility, so I think this is really important.

To answer your comment about the bug report, the contrib author can be non-cooperative and even point you at core (or whatever other big contrib) and say "Look, core is doing it this way too with Action plugins, so leave me alone.". Just a random example, but I hope you get my point.

Anyway, I'm not saying we shouldn't do this for stream wrappers, I like @effulgentsia's reasoning in #40 and I can also go along with it.

Status:Needs review» Needs work

Moving to file system component per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

Component:base system» file system
Status:Needs work» Needs review
StatusFileSize
new40.47 KB
FAILED: [[SimpleTest]]: [MySQL] 56,667 pass(es), 399 fail(s), and 2,575 exception(s).
[ View ]
new3.06 KB

My apologies to @amateescu, my comment about being offtopic was in reference to *me* derailing the issue, not him.

Rerolling, and seeing what caching breaks.

Status:Needs review» Needs work

The last submitted patch, stream-wrappers-2028109-46.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new39.99 KB
PASSED: [[SimpleTest]]: [MySQL] 57,769 pass(es).
[ View ]

Unable to find the wrapper "public" - did you forget to enable it when you configured PHP?

Yeah that's what I thought. If you cache it, it doesn't re-register them each time.

That's going to be more work to figure out, let's just profile this and see if its even worth doing.

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review

I fixed devel_generate so it would work with images :)
Some kind soul, have at it!

StatusFileSize
new40 KB
PASSED: [[SimpleTest]]: [MySQL] 58,150 pass(es).
[ View ]

Rerolled for the config() to Drupal::config() conversion.

OK I did some xhprof profiling, overall results here: without patch: 520f15feb42f4 with patch: 520f15bcce8ff

  Run #520f15feb42f4 Run #520f15bcce8ff Diff Diff%
Number of Function Calls 114,851 114,300 -551 -0.5%
Incl. Wall Time (microsec) 535,815 565,746 29,931 5.6%
Incl. CPU (microsecs) 492,974 505,604 12,630 2.6%
Incl. MemUse (bytes) 24,782,896 23,973,144 -809,752 -3.3%
Incl. PeakMemUse (bytes) 24,918,504 24,067,840 -850,664 -3.4%

#51 is pretty bad. What if we keep the work that's been done here in terms of the plugin manager, but use HookDiscovery instead of AnnotatedClassDiscovery?

StatusFileSize
new22.47 KB
new22.68 KB
FAILED: [[SimpleTest]]: [MySQL] 58,118 pass(es), 0 fail(s), and 8,275 exception(s).
[ View ]

Okay.

Status:Needs review» Needs work

The last submitted patch, stream-wrappers-2028109-53.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new732 bytes
new22.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,003 pass(es).
[ View ]

Right, we don't get the nice defaults from the annotation anymore.

Here's a reroll of the old patch.

Issue summary:View changes

Updated

Updated issue summary.

StatusFileSize
new4.23 KB
new39.79 KB
FAILED: [[SimpleTest]]: [MySQL] 40,706 pass(es), 7,401 fail(s), and 10,001 exception(s).
[ View ]

Attached patch is based on #56. It partly re-applies interdiff from #46 to re-add cache support. It then changes the way how we register wrappers to PHP a bit. Instead of doing it in processDefinitions() I created new function that is called from getWrappers().

There is another issue if we register in processDefinitions(). Since alter gets called after that we possibly ignore any changes that were added in alter hooks and would influence wrapper's registration.

Interdiff is against #56.

Status:Needs review» Needs work

The last submitted patch, drupal8.file-system.2028109-58.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.63 KB
new40.95 KB
FAILED: [[SimpleTest]]: [MySQL] 41,416 pass(es), 7,472 fail(s), and 3,970 exception(s).
[ View ]

Some semi-smart cache clears added. Hope this will help.

Status:Needs review» Needs work

The last submitted patch, drupal8.file-system.2028109-60.patch, failed testing.

How is that supposed to work? getWrappers is not guaranteed to run on every page.

benjy was going to do more profiling today, please use #55 and #56?

@tim.plunkett: getWrappers() is called from file_get_stream_wrappers(), which is called from _drupal_bootstrap_full(). Shouldn't that be enough?

OK I've tested #55 and #56.

HEAD: 5217f06eec80f
#55: 5217f0ea61cc2
#56: 5217f152d5f71

HEAD and #55

Run #5217f06eec80f Run #5217f0ea61cc2 Diff Diff%
Number of Function Calls 129,544 129,724 180 0.1%
Incl. Wall Time (microsec) 576,804 576,306 -498 -0.1%
Incl. CPU (microsecs) 534,737 539,156 4,419 0.8%
Incl. MemUse (bytes) 24,711,496 25,278,040 566,544 2.3%
Incl. PeakMemUse (bytes) 24,784,672 25,374,920 590,248 2.4%

HEAD and #56

Run #5217f06eec80f Run #5217f152d5f71 Diff Diff%
Number of Function Calls 129,544 129,724 180 0.1%
Incl. Wall Time (microsec) 576,804 590,530 13,726 2.4%
Incl. CPU (microsecs) 534,737 546,930 12,193 2.3%
Incl. MemUse (bytes) 24,711,496 25,278,040 566,544 2.3%
Incl. PeakMemUse (bytes) 24,784,672 25,374,920 590,248 2.4%

Status:Needs work» Needs review
StatusFileSize
new42.82 KB
FAILED: [[SimpleTest]]: [MySQL] 58,368 pass(es), 3 fail(s), and 63 exception(s).
[ View ]
new6.67 KB
new6.19 KB

I was able to reproduce some of the fails in #60 locally, but not all of them.

I made some changes, and am including two interdiffs, one from my last annotated plugin patch in 56 and one from @slashrsm's patch in #60.

Status:Needs review» Needs work

The last submitted patch, stream_wrappers-2028109-65.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new650 bytes
new42.92 KB
FAILED: [[SimpleTest]]: [MySQL] 58,310 pass(es), 0 fail(s), and 50 exception(s).
[ View ]

The Locale stream wrapper isn't working. No idea.

Tested #67

HEAD: 521851729b64b
#67: 52185125e7a9d

Run #521851729b64b Run #52185125e7a9d Diff Diff%
Number of Function Calls 130,560 129,270 -1,290 -1.0%
Incl. Wall Time (microsec) 557,331 543,049 -14,282 -2.6%
Incl. CPU (microsecs) 518,218 507,715 -10,503 -2.0%
Incl. MemUse (bytes) 25,598,792 24,496,872 -1,101,920 -4.3%
Incl. PeakMemUse (bytes) 25,695,904 24,567,568 -1,128,336 -4.4%

OK, after a few comments regarding the previous results I ran the tests again, ensuring that I got the same results after 5 runs with HEAD and 5 runs with #67 applied.

HEAD: 5218585295f8f
#67: 52185960b16c4

Run #5218585295f8f Run #52185960b16c4 Diff Diff%
Number of Function Calls 128,596 130,705 2,109 1.6%
Incl. Wall Time (microsec) 558,652 572,344 13,692 2.5%
Incl. CPU (microsecs) 516,710 536,959 20,249 3.9%
Incl. MemUse (bytes) 24,492,616 24,658,472 165,856 0.7%
Incl. PeakMemUse (bytes) 24,561,464 24,730,032 168,568 0.7%

Status:Needs review» Needs work

The last submitted patch, stream_wrappers-2028109-67.patch, failed testing.

As requested, I tested #56 against HEAD again this time doing each test 5 times and excluding any outliers.

HEAD: 5219456189fcc
#56: 5219477745219

Run #5219456189fcc Run #5219477745219 Diff Diff%
Number of Function Calls 130,707 129,712 -995 -0.8%
Incl. Wall Time (microsec) 566,546 553,525 -13,021 -2.3%
Incl. CPU (microsecs) 524,670 517,883 -6,787 -1.3%
Incl. MemUse (bytes) 24,589,720 23,799,528 -790,192 -3.2%
Incl. PeakMemUse (bytes) 24,662,328 23,871,264 -791,064 -3.2%

Status:Needs work» Needs review
StatusFileSize
new40.05 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

So now we have two sets of numbers for #56, this latest one saying its faster...

Here's a reroll of #56 just to make sure it still passes.

We might need a second opinion here on the xhprof stuff.

Did some more profiling. 5 subsequent requests to homepage, caches are empty at 1st request.

Results:
https://docs.google.com/spreadsheet/ccc?key=0AuCzOZ1L52ODdDhSWU1kOGQ5RXh...

The diff looks reversed in the spreadsheet?

It looks like there are consistently 1195 function calls removed (or added?). If this is the case it should be easy using diff in the standard xhprof UI (or just having a look around) to see which function calls those are.

You are right. Diff was inverted. Fixed.

Status:Needs review» Needs work

The last submitted patch, stream-wrappers-2028109-72.patch, failed testing.

Status:Needs work» Needs review

#72: stream-wrappers-2028109-72.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, stream-wrappers-2028109-72.patch, failed testing.

Status:Needs work» Needs review

#72: stream-wrappers-2028109-72.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs profiling, +blocker, +Plugin-conversion, +Kill includes

The last submitted patch, stream-wrappers-2028109-72.patch, failed testing.

Issue tags:+Needs reroll

This issue needs a reroll.

Status:Needs work» Needs review
StatusFileSize
new40.08 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-roll
Still doesn't install
Tried to find the issue but it looks annotation related and they all look ok to me :(

Status:Needs review» Needs work

The last submitted patch, stream-wrappers-2028109.83.patch, failed testing.

will try and nut this out later today

Status:Needs work» Needs review
StatusFileSize
new40.08 KB
PASSED: [[SimpleTest]]: [MySQL] 59,235 pass(es).
[ View ]
new804 bytes

Lets see how this goes

+++ b/core/includes/file.inc
@@ -185,78 +156,33 @@
+  return Drupal::service('plugin.manager.stream_wrapper')->getWrappers($filter);
...
+  $wrapper = Drupal::service('plugin.manager.stream_wrapper')->getDefinition($scheme);
@@ -351,25 +277,17 @@ function file_stream_wrapper_uri_normalize($uri) {
+  return Drupal::service('plugin.manager.stream_wrapper')->getInstance(array('uri' => $uri));
@@ -383,25 +301,17 @@ function file_stream_wrapper_get_instance_by_uri($uri) {
+  return Drupal::service('plugin.manager.stream_wrapper')->getInstance(array('scheme' => $scheme));

should use \Drupal::

Issue tags:-Needs reroll

StatusFileSize
new42.44 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new7.65 KB

Reroll.

StatusFileSize
new1.69 KB
new42.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,769 pass(es).
[ View ]

Missed the review in #87

Status:Needs review» Needs work
  1. +++ b/core/includes/file.inc
    @@ -185,78 +156,33 @@
    function file_get_stream_wrappers($filter = STREAM_WRAPPERS_ALL) {
    ...
    function file_stream_wrapper_get_class($scheme) {
    @@ -351,25 +277,17 @@ function file_stream_wrapper_uri_normalize($uri) {
    function file_stream_wrapper_get_instance_by_uri($uri) {
    @@ -383,25 +301,17 @@ function file_stream_wrapper_get_instance_by_uri($uri) {
    function file_stream_wrapper_get_instance_by_scheme($scheme) {

    I think we should add @deprecated to docs as well.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/PrivateStream.php
    @@ -2,34 +2,41 @@
    +use Drupal\Core\StreamWrapper\LocalStream;
    +++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
    @@ -2,16 +2,25 @@
    +use Drupal\Core\StreamWrapper\LocalStream;
    +++ b/core/lib/Drupal/Core/StreamWrapper/TemporaryStream.php
    @@ -2,31 +2,41 @@
    +use Drupal\Core\StreamWrapper\LocalStream;

    We can remove use as it is in the same namespace.

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,169 @@
    +   * use the same set of methods to access both local files and remote resources.
    ...
    +   * wrappers that are appropriate for particular usage. For example, this returns
    ...
    +   * The $filter parameter can only filter to types containing a particular flag.
    ...
    +   *   STREAM_WRAPPERS_VISIBLE), then only stream wrappers with all three of these
    ...
    +   *   about the stream wrapper, as returned by hook_stream_wrappers(). If $filter
    +   *   is omitted or set to STREAM_WRAPPERS_ALL, the entire Drupal stream wrapper

    80 chars limit

Status:Needs work» Needs review
StatusFileSize
new6.41 KB
new42.44 KB
PASSED: [[SimpleTest]]: [MySQL] 58,852 pass(es).
[ View ]

I don't understand all of the concern around caching. file_get_stream_wrappers() never had DB caching, only static caching. And StreamWrapperManager provides the same level of static caching by default.

Addressed #91.

Yes, but now, it's based on hook discovery, this changes it to annotation based discovery. That's a considerable regression, happening on every request :(

Yep.

Issue tags:-Needs profiling
StatusFileSize
new29.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,633 pass(es).
[ View ]

Okay, let's just use HookDiscovery then, and be done with it. We still want to clean up file.inc

Issue tags:+Needs profiling

and be done with it.

yeah sure. There is nothing to review form coding point of view. Perhaps we can add unit tests but seems out off scope. So RTBC for me. Let's see what @catch and @Berdir has to say.

#95: stream-wrappers-2028109-95.patch queued for re-testing.

Why can't the annotation discovery be persistently cached? That's still an extra cache hit but we have an issue open to group some of those plugin cache requests together.

I realise that stream wrappers get used very early, but I'd expect hook discovery to be worse from that point of view.

Issue summary:View changes

Update issue summary from #33 to #56

Issue summary:View changes
StatusFileSize
new29.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,011 pass(es).
[ View ]

Quick reroll.

It looks that we still didn't really decide whether to go with HookDiscovery or AnnotationDiscovery + persistent cache. It looks that we'd prefer to use annotations since this is the main approach all across D8, but there are some issues that appear with caching, which makes hook approach easier to implement.

We could commit HookDiscovery for now and open a follow-up that would deal with conversion to AnnotationDiscovery. We'd finally convert stream wrappers to plugins this way, while still leaving door open to annotations if we really want them. How does this sound?

Issue tags:+Media Initiative

StatusFileSize
new46.9 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

As per IRC discussion with @tim.plunkett. This patch is a re-roll of #67, which looks like the last annotation-style patch with caching.

Status:Needs review» Needs work

The last submitted patch, 102: 2028109_102.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new46.89 KB
FAILED: [[SimpleTest]]: [MySQL] 59,325 pass(es), 2 fail(s), and 84 exception(s).
[ View ]
new955 bytes

Should install now...

The last submitted patch, 104: 2028109_104.patch, failed testing.

Issue tags:+API clean-up, +@deprecated

Hm. I'd like to raise some concerns.

While this patch certainly follows D8's architecture,

  1. There's a good chance for race conditions — if the stream wrappers are plugins, some stream wrappers depend on configuration and other services, which in turn may depend on services using plugins and/or custom stream wrappers.

  2. I never really understood why we're managing stream wrappers in such a high-level way to begin with. What's the purpose of allowing modules to override/replace a custom stream wrapper, and how can that be a safe operation regarding the original provider and its consumers in the first place?

  3. It feels Just Wrong™ that extensions are able to swap out custom stream wrappers at runtime.

  4. Consequently, I wonder why custom stream wrappers are not "hard-wired". In other words, they'd essentially be similar to a new Container CompilerPass (because some custom stream wrappers depend on other services; typically config/bootstrap-config):

    1. CoreServiceProvider registers 'public' (since it depends on Settings only).
    2. SystemServiceProvider registers 'private' + 'temporary' (they depend on the system.file config)
    3. Other service providers register additional schemes.
    4. And finally, the CompilerPass performs the actual registration using the documented failsafe algorithm (pseudo code):
      <?php
      foreach ($this->wrappers as $scheme => ($class, $flags = 0)) {
       
      $existed = in_array($scheme, stream_get_wrappers());
        if (
      $existed) {
         
      stream_wrapper_unregister($scheme);
        }
       
      stream_wrapper_register($scheme, $class, $flags);
      }
      ?>
    5. Lastly, Kernel::shutdown() unregisters all custom stream wrappers that have been overridden in the compiler pass.

    An approach like that would also allow us to properly inject dependencies (like config) into stream wrapper classes upon registration (we can't act on instantiation, because PHP instantiates them automatically).

In short, I don't really see "pluggable" aspect in stream wrappers. PHP requires them to be registered in a very controlled fashion. They need to be registered as early as possible, so other services are able to rely on them.

Since PHP 5.4, stream wrappers have matured significantly; our entire Drupal-specific StreamWrapperInterface is most likely obsolete. (cf. #2107287: PHP 5.4 calls a new stream_metadata() method on stream wrappers not implemented by Drupal)

Thoughts?

To perhaps advance on my previous comment:

Given that stream wrappers are a native construct of PHP, I wondered what Symfony is doing, and why we're re-inventing a subsystem to register and manage custom stream wrappers to begin with.

For example, I found https://github.com/KnpLabs/KnpGaufretteBundle, which is a high-level bundle that uses the https://github.com/KnpLabs/Gaufrette base filesystem library under the hood.

In short, we might be able to replace our entire Stream Wrapper API with an existing Symfony library that properly integrates with DI container services.

Gaufrette's pretty nifty, but it is an entirely separate thing from stream wrappers. Gaufrette can produce stream wrappers, but it is an edge case compared to its primary intentions.

Yeah, I'm not necessarily saying that we should use an external/existing library, but primarily, let's have a look at how Symfony apps or libraries are doing it — especially concerning dependency/configuration injection.

And I still think that the plugin system is a too high-level construct for stream wrappers. — Plugins give developers the misleading impression that each stream wrapper would be instantiated like any other plugin, but that is not the case.

In fact, there is no point in "instantiating a stream wrapper plugin", because user-space code does not instantiate a stream wrapper class.

As a developer, I'd expect DrupalKernel::boot() to register custom stream wrappers (as early as possible) and DrupalKernel::shutdown() to unregister them. That way, the kernel continues to manage application state as usual.

i agree with #110. this is low level stuff.

At one point months ago, I asked @msonnabaum to help profile this, and he said he thought plugins were too high level, and that these should just be tagged services.

That seems to jive with #110/#111...

Tagged services works for me as well, these are more on the level of database/cache backends and we definitely don't need a UI to manage them.

Assigned:Unassigned» twistor

I'll take a blind stab at this today.

I looked at this briefly and realized we need a place to put the 'types' (STREAM_WRAPPERS_LOCAL_NORMAL and friends).
@twistor and I talked briefly today, and we weren't sure whether to put them in the service definition, or in a method, or what. The service definition seemed fine at first, but we can't use constants there.

Then I talked to @msonnabaum more, and decided that a static method makes the most sense.

We need a Drupal\Component\StreamWrapper\StreamWrapperManager class to manage registrations anyway. That class can hold these constants; e.g.:

<?php
class StreamWrapperManager {
  const
LOCAL = 0x0001;
  const
READABLE = 0x0004;
  const
VISIBLE = 0x0010;
...
  const
TYPE_WRITE_VISIBLE = READABLE | WRITABLE | VISIBLE;
...
  public function
register($scheme, $class, $type) {
  }
  public function
unregister($scheme) {
  }
}
?>

Most of this class should actually be static, because, again, any particular state in this class will and can only diverge from PHP's global state.

→ PHP's global state is solely managed via stream_wrapper_register() and stream_wrapper_unregister(). Any attempt to manually synchronize that state is guaranteed to get out of sync.

Additionally, it should be clear to everyone that if we register stream wrappers as services on the container, those services will not actually be instantiated.

→ The service container would only be leveraged/used as a mechanism to "expose" (not register) stream wrapper classes via service providers, so as to collect + register them when the kernel boots (and unregister them when it shuts down).

The patch in #1376122: Stream wrappers of parent site are leaking into all tests might be helpful to understand these considerations (in particular, have a look at DUTB).

I think we'd be better off with the constants on StreamWrapperInterface, because it'd be weird for the wrappers to reference the manager when declaring its type.

I basically agree on your other points.

Sorry, I got sidetracked by other things, I'll get some code up here soon.

So I came to the same conclusion about using a static method on stream wrapper classes to expose the definitions. Mostly because of the name, description + the alter hook. I actually figured out a workaround for the constants, but it doesn't help much.

I also moved the constants to StreamWrapperInterface already, so that's nice.

I have register() unregister() methods called on boot/shutdown respectively. Although, they don't take arguments, they just register/unregister the tagged services.

@sun, I'm a bit confused about StreamWrapperManager not storing any state.

We still need a place to keep the wrapper definitions for functions that inspect wrappers. file_stream_wrapper_get_class(), file_stream_wrapper_uri_normalize(), file_stream_wrapper_get_instance_by_scheme(). These wrapper classes will be instantiated upon request.

"→ The service container would only be leveraged/used as a mechanism to "expose" (not register) stream wrapper classes via service providers, so as to collect + register them when the kernel boots (and unregister them when it shuts down)."

This confuses me. The container is not literally registering the wrappers, the compiler pass is collecting them and passing them on to StreamWrapperManager. StreamWrapperManager::register() is called on boot, and StreamWrapperManager::unregister() is called on shutdown.

I'm not sure if that is the distinction you are making, or if it's something else.

I'll get some code up so we have something more concrete to discuss.

Related issues:+#1376122: Stream wrappers of parent site are leaking into all tests
StatusFileSize
new57.33 KB
FAILED: [[SimpleTest]]: [MySQL] 62,294 pass(es), 203 fail(s), and 2,099 exception(s).
[ View ]

Alrighty, this still needs work, but here's the idea.

Title:Convert hook_stream_wrappers() to plugin systemConvert hook_stream_wrappers() to tagged services.

StatusFileSize
new59 KB
FAILED: [[SimpleTest]]: [MySQL] 62,327 pass(es), 194 fail(s), and 8 exception(s).
[ View ]

This steals one line from #1376122: Stream wrappers of parent site are leaking into all tests regarding re-registering the stream wrappers during restoreEnvironment().

The last submitted patch, 119: 2028109-119.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 121: 2028109-121.patch, failed testing.

StatusFileSize
new6.89 KB
new60.22 KB
FAILED: [[SimpleTest]]: [MySQL] 63,205 pass(es), 14 fail(s), and 4 exception(s).
[ View ]

Alright, it would be super-awesome if we didn't have to translate the name and description of the stream wrappers. Maybe move them to a getName() and getDescription() method that we don't have to call directly in boot. Not, sure.

Moved private and temporary stream wrappers to system.services.yml.

+1 to removing description/name translatability.
See the ugly workarounds needed in #2016629: Refactor bootstrap to better utilize the kernel to get that working

Status:Needs work» Needs review
StatusFileSize
new12.91 KB
new62.39 KB
FAILED: [[SimpleTest]]: [MySQL] 63,023 pass(es), 28 fail(s), and 11 exception(s).
[ View ]

This moves the name and description to getName() and getDescription(). It also adds some helper methods on StreamWrapperManager to grab names and descriptions conveniently. I think this should solve the problem and still allow name and description to be translated. We could probably even inject the translation. This also should allow us to get rid of one more UpdateModuleHandler.php hack.

The StreamWrapperInterface::info() method is replaced with getType() since that is the only thing left. If we moved the type to the server definition, we could get rid of that as well, not sure if it's worth it.

The next thing I would like to do is kill hook_stream_wrappers_alter() so we aren't loading the module handler so early. We could make dynamic stream wrappers, like private use a service provider. I wasn't having any luck yet, it seems I can't read the config, which kinda makes sense. hmmm.

Thanks a lot for working on this — that looks like a good start.

I think it would be a good idea to get #1376122: Stream wrappers of parent site are leaking into all tests into core first, so we're able to validate this change against fixed and cleaned tests.

However, there are some architectural issues that will most likely lock us into a deep catch-22:

  1. StreamWrapperManager::getWrapper() manually instantiates a stream wrapper class. This instantiated object is used directly by consuming code, instead of relying on PHP's native stream wrappers. That should not be the case, and IMO it would be wrong to design the architecture here around our existing (broken) usage of stream wrappers.

    In other words, some (but not all) consuming code works like this currently:

    <?php
     
    if ($wrapper = file_stream_wrapper_get_instance_by_uri($uri)) {
        if (
    $wrapper->chmod($mode)) {
          return
    TRUE;
        }
      }
      else {
        if (
    chmod($uri, $mode)) {
          return
    TRUE;
        }
      }
    ?>

    Whereas all consuming code should simply look like this:

    <?php
     
    if (chmod($uri, $mode)) {
        return
    TRUE;
      }
    ?>

    → Drupal is using native PHP stream wrappers for most operations currently, but for a few operations, it is circumventing the native stream wrappers and manually operates on custom stream wrapper class instances instead.

    The only reason for that is the manual call to $instance->setUri($uri) — which is a remnant of PHP <5.4 era stream wrappers: Drupal's custom StreamWrapperInterface requires stream wrapper classes to additionally implement chmod(), realpath(), and dirname() methods.

    Those custom methods are obsolete. In PHP 5.4, the native stream wrapper class template/interface of PHP core requires additional methods on each stream wrapper class to perform those operations. In fact, PHP 5.4 throws an exception, because the methods do not exist.

    The only reason for why we do not see those errors currently is because most calls throughout core are @error-suppressed. For example, the case of chmod() is tackled here:

    #2107287: PHP 5.4 calls a new stream_metadata() method on stream wrappers not implemented by Drupal

    I already provided a working patch over there. There are no issues for the other missing methods yet, but I'm 99% confident that PHP 5.4 makes our entire custom StreamWrapperInterface and their manual instantiation completely obsolete.

    But unfortunately - those additional methods are only known on PHP 5.4. Not on PHP 5.3. Thus, that fix and alike fixes are currently blocked on the testbot upgrade to PHP 5.4:

    https://drupal.org/project/issues/search?issue_tags=phpfpm-testbot

    To help resolve that deadlock, I provided patches for each of those blocking issues, too. → All we need are manual testing confirmations and commits, so (1) testbot maintainers are able to move forward with php-fpm/fastcgi, (2) we can finally bump PHP requirements for D8, and (3) we can get rid of this inappropriate dance of manually instantiating stream wrapper classes.

    Long story short:

    → The getWrapper() & Co methods here should not exist. Stream wrapper classes are never instantiated by Drupal.

    Likewise, StreamWrapperManager should not have any knowledge about "services". Only the $info property should remain.

    In fact, it would be a good idea to let the StreamWrapperCompilerPass actually remove the service definitions after processing them. → It's pointless to retain those services at runtime. Any such usage/instantiation is invalid.

  2. As already mentioned in #107, I agree that the entire idea of allowing a drupal_alter() on stream wrappers is a design flaw.

    → The alter hook should be removed without replacement. That inherently gets rid of the ModuleHandler dependency, too.

  3. Instead, we need a new static isAvailable() method that allows the registration process to check whether a declared stream wrapper can be registered.

    For simplicity, it might be best to simply inject the Container into that method. That way:

    <?php
    class PrivateStream {
      public static function
    isAvailable(Container $container) {
        return (bool)
    $container->get('config.factory')->get('system.file')->get('path.private');
      }
    }
    ?>

    Whereas it is irrelevant whether the private filesystem path may or may not be converted from config into Settings later on. → Contributed modules will most likely have this use-case (depending on configuration), so we'll need isAvailable() anyway.

  4. That said, this gets us into the overall topic of configuration/dependency injection (because stream wrapper classes are not instantiated by Drupal/the service container).

    → To resolve that, we want to consider to change isAvailable() into initialize() and denote availability based on the return value instead:

    <?php
    class PrivateStream {
      protected static
    $path;
      public static function
    initialize(Container $container) {
       
    $path = $container->get('config.factory')->get('system.file')->get('path.private');
        if (!
    $path) {
          return
    FALSE;
        }
        static::
    $path = $path;
        return
    TRUE;
      }
    }
    ?>
  5. For the getName() [→ getLabel()] and getDescription() methods, we should consider to use a similar construct of static methods, but inject the StringTranslation service instead.

    If we want to ensure a nice DX for that for consumers (debatable, because there appear to be only ~2), we can consider to add wrapping methods to the StreamWrapperManager service:

    <?php
    class StreamWrapperManager {
      public function
    getName($scheme) {
       
    $class = $this->getClass($scheme);
        return
    $class::getName($this->stringTranslation);
      }
    }
    ?>

    These two getName() and getDescription() methods will be the only methods that remain on our custom/Drupal-specific StreamWrapperInterface, which is very unfortunate, because otherwise, the entirety of this new code would be a new standalone component that is properly decoupled from Drupal.

    In the end, we should arrive at this for D8:

    Drupal\Component\StreamWrapper\StreamWrapperInterface  [ex. PhpStreamWrapperInterface]
    Drupal\Component\StreamWrapper\StreamWrapperManager
    Drupal\Component\StreamWrapper\LocalStreamBase         [sans getName()/getDescription()]
    ...
    Drupal\Core\StreamWrapper\StreamWrapperInterface       [extends Component\StreamWrapperInterface]
    Drupal\Core\StreamWrapper\PublicStream
    Drupal\Core\StreamWrapper\TemporaryStream
    Drupal\Core\StreamWrapper\PrivateStream

    To avoid that custom StreamWrapperInterface just for the getName()/getDescription() methods, we could consider to partially (only partially) go back to the former idea of "plugins" — whereas by that I exclusively mean an AnnotatedClassDiscovery:

    <?php
    namespace Drupal\Core\StreamWrapper\StreamWrapper;
    use
    Drupal\Component\StreamWrapper\StreamWrapperInterface;
    /**
    * ...
    *
    * @StreamWrapper(
    *   scheme = "public",
    *   label = @Translation("Public files"),
    *   description = @Translation("...")
    * )
    */
    class PublicStream implements StreamWrapperInterface {
    }
    ?>

    In other words:

    1. Require all stream wrapper classes to be located in a ./StreamWrapper/ subdirectory.
    2. Require a @StreamWrapper annotation on each class.
    3. Use the AnnotatedClassDiscovery of the plugin system, but only that, and only for discovery.

    → Ensure that the StreamWrapper component is able to work without this additional meta information, which is only required for Drupal's UI.

    Since the information gets compiled into the container, former performance concerns should no longer apply.

Status:Needs review» Needs work

The last submitted patch, 126: 2028109-126.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.78 KB
new63.89 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I haven't addressed #127 yet, just getting things to pass.

  1. Moved temporary stream wrapper back to core.
  2. Fixed constant math.
  3. Register an empty StreamWrapperManger during install.
  4. Stupidity fixes.

Status:Needs review» Needs work

The last submitted patch, 129: 2028109-129.patch, failed testing.

The last submitted patch, 129: 2028109-129.patch, failed testing.

Status:Needs work» Needs review

hmm

The last submitted patch, 124: 2028109-124.patch, failed testing.

StatusFileSize
new1.18 KB
new1.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2028109-134.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Forgot to remove temporary stream wrapper from system.services.

Status:Needs review» Needs work

The last submitted patch, 134: 2028109-134.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new63.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2028109-135.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Well, I suppose if I upload the right patch.

Interdiff is in #134.

Status:Needs review» Needs work

The last submitted patch, 136: 2028109-135.patch, failed testing.

The last submitted patch, 136: 2028109-135.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new73.26 KB
PASSED: [[SimpleTest]]: [MySQL] 63,308 pass(es).
[ View ]

One dependency in!

I lost the interdiff during the re-roll.

Let's see what this does.

StatusFileSize
new1.61 KB
new72.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2028109-140.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

So I want to put this back back before I forget.

In drupal_install_system() DrupalKernel::boot() is called before the system module is added to the module list, so declaring the private stream wrapper there doesn't work. Well, it works for a normal install, it just doesn't work for WebTestBase. The interdiff attached shows the change I made, which I'm now removing to point it out.

Making file_ensure_htaccess() depend on the private wrapper being available, rather than the config option, makes sense to me, but...

Issue tags:-blocker+beta blocker

The exhaustive explanation in #127 clarifies that the vast majority of our current File API + Stream Wrapper API incarnations is completely obsolete, once Drupal core has bumped requirements to PHP 5.4.

Since bumping core requirements to PHP 5.4 is a beta blocker (for many other reasons) already, this issue (and in turn, getting rid of most of our obsolete File + Stream Wrapper APIs) is a beta blocker, too.

Tagging accordingly.

Issue tags:-beta blocker

I'd let this in after beta. Beta blockers is only for patches which are going to break major APIs, which declaring/implementing stream wrappers is definitely not - at most a handful of contrib modules that do so.

140: 2028109-140.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 140: 2028109-140.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new69.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php.
[ View ]

reroll
stream wrappers are yuck in #2016629: Refactor bootstrap to better utilize the kernel needing t() too early in bootstrap, so this is a good step

Status:Needs review» Needs work

The last submitted patch, 145: stream-wrappers.145.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new70.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,820 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

doh

Status:Needs review» Needs work

The last submitted patch, 147: stream-wrappers.147.patch, failed testing.

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,273 @@
    +  protected $info = array();
    +  protected $wrappers = array();
    +  protected $services = array();

    @var block missing.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,273 @@
    +   * use the same set of methods to access both local files and remote resources.
    ...
    +   * wrappers that are appropriate for particular usage. For example, this returns
    ...
    +   * The $filter parameter can only filter to types containing a particular flag.
    ...
    +   *   bit in $filter. For example, if $filter is StreamWrapperInterface::WRITE_VISIBLE,
    +   *   which is equal to (StreamWrapperInterface::READ | StreamWrapperInterface::WRITE |
    +   *   StreamWrapperInterface::VISIBLE), then only stream wrappers with all three of these
    +   *   bits set are returned. Defaults to StreamWrapperInterface::ALL, which returns all
    ...
    +   *   about the stream wrapper, as returned by hook_stream_wrappers(). If $filter
    +   *   is omitted or set to StreamWrapperInterface::ALL, the entire Drupal stream wrapper

    more then 80 chars.

Status:Needs work» Needs review
StatusFileSize
new4.89 KB
new71.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch stream-wrappers.150.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixes #149 and hopefully failing test

I haven't worked on this since we switched from plugins, so I feel okay reviewing and hopefully RTBCing this soon

  1. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
    @@ -14,25 +14,25 @@
    use Drupal\Core\DependencyInjection\ServiceProviderInterface;
    use Drupal\Core\DependencyInjection\ContainerBuilder;
    use Drupal\Core\DependencyInjection\Compiler\ModifyServiceDefinitionsPass;
    -use Drupal\Core\DependencyInjection\Compiler\RegisterKernelListenersPass;
    use Drupal\Core\DependencyInjection\Compiler\RegisterAccessChecksPass;
    +use Drupal\Core\DependencyInjection\Compiler\RegisterAuthenticationPass;
    +use Drupal\Core\DependencyInjection\Compiler\RegisterBreadcrumbBuilderPass;
    +use Drupal\Core\DependencyInjection\Compiler\RegisterKernelListenersPass;
    +use Drupal\Core\DependencyInjection\Compiler\RegisterParamConvertersPass;
    use Drupal\Core\DependencyInjection\Compiler\RegisterPathProcessorsPass;
    -use Drupal\Core\DependencyInjection\Compiler\RegisterRouteProcessorsPass;
    -use Drupal\Core\DependencyInjection\Compiler\RegisterRouteFiltersPass;
    use Drupal\Core\DependencyInjection\Compiler\RegisterRouteEnhancersPass;
    -use Drupal\Core\DependencyInjection\Compiler\RegisterParamConvertersPass;
    +use Drupal\Core\DependencyInjection\Compiler\RegisterRouteFiltersPass;
    +use Drupal\Core\DependencyInjection\Compiler\RegisterRouteProcessorsPass;
    use Drupal\Core\DependencyInjection\Compiler\RegisterServicesForDestructionPass;
    +use Drupal\Core\DependencyInjection\Compiler\RegisterStreamWrappersPass;
    use Drupal\Core\DependencyInjection\Compiler\RegisterStringTranslatorsPass;
    -use Drupal\Core\DependencyInjection\Compiler\RegisterBreadcrumbBuilderPass;
    -use Drupal\Core\DependencyInjection\Compiler\RegisterAuthenticationPass;
    use Drupal\Core\DependencyInjection\Compiler\RegisterTwigExtensionsPass;
    use Drupal\Core\Plugin\PluginManagerPass;
    use Drupal\Core\Theme\ThemeNegotiatorPass;
    -use Symfony\Component\DependencyInjection\ContainerInterface;
    -use Symfony\Component\DependencyInjection\Reference;
    +use Symfony\Component\DependencyInjection\Compiler\PassConfig;
    use Symfony\Component\DependencyInjection\Definition;
    +use Symfony\Component\DependencyInjection\Reference;
    use Symfony\Component\DependencyInjection\Scope;
    -use Symfony\Component\DependencyInjection\Compiler\PassConfig;

    I don't understand why we're rearranging all of these. If there is some important ordering here, can we get some inline comments?

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,291 @@
    +class StreamWrapperManager extends ContainerAware {

    We should back our services with interfaces.

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,291 @@
    +   * @todo

    There are still a couple of these strewn about.

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,291 @@
    +  public function register() {
    +    $this->moduleHandler->alter('stream_wrappers', $this->info);
    +    $this->services = array_intersect_key($this->services, $this->info);
    +
    +    foreach ($this->info as $scheme => $info) {
    +      $this->registerWrapper($scheme, $info['class'], $info['type']);
    +    }
    +  }

    Nice! Clean way to handle the alterability without any of the other mess

Do we think its reasonable for unit tests here? Or should that wait until #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\Utility\File class due to the other function calls?

1) bad merge, will revert
2) will do
3) will fix
4) :)
5) (unit tests) will take a look

Issue summary:View changes

Hey, this looks pretty awesome already :-)

I still have some fundamental remarks on the architecture, but please note that I'm not sure how many of them can reasonably be addressed in this issue, and which of them can or need to be deferred to follow-up issues. — Given how flawed our legacy stream wrapper API is, that's a very tough decision to make...

  1. +++ b/core/core.services.yml
    +  stream_wrapper.public:
    +  stream_wrapper.private:
    +  stream_wrapper.temporary:

    The private stream is actually owned by System module currently.

    Technically, the public stream is too, but the installer creates + configures that stream already (without System module interaction). Thus, it is OK to register it in core.

    The temporary stream works in all cases, because it has a fallback to the OS temp directory. Thus, also OK to get registered by core.

  2. +++ b/core/includes/install.core.inc
    @@ -325,6 +325,12 @@ function install_begin_request(&$install_state) {
    +  // Register the stream wrapper manager.
    +  $container
    +    ->register('stream_wrapper_manager', 'Drupal\Core\StreamWrapper\StreamWrapperManager')

    This appears to be a bogus merge conflict resolution.

    That code spot used to manually register some services for the early installer environment, but the installer uses the Installer\InstallerServiceProvider now.

    Since that code just registers the regular definition, it looks obsolete.

  3. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
    @@ -14,25 +14,25 @@
    -use Drupal\Core\DependencyInjection\Compiler\RegisterKernelListenersPass;

    I agree with @tim.plunkett, would be great to remove the unrelated import ordering changes here.

  4. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterStreamWrappersPass.php
    @@ -0,0 +1,38 @@
    +  public function process(ContainerBuilder $container) {
    +    if (!$container->hasDefinition('stream_wrapper_manager')) {
    +      return;
    +    }

    This early-return also appears to be a remnant of the former early installer environment?

    To my knowledge, none of the other compiler passes has a check like this, and stream wrappers are bare essential...

  5. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterStreamWrappersPass.php
    @@ -0,0 +1,38 @@
    +      $stream_wrapper_manager->addMethodCall('addStreamWrapper', array($id, $class, $scheme));

    Hm.

    The service ID should be irrelevant for a stream wrapper implementation. That's an internal detail of our chosen DI approach.

    $class and $scheme are mandatory, of course.

    However, instead of $id, perhaps we should allow to pass arbitrary other parameters as $options through the tag parameters?

    i.e.:

    $class ← class
    $scheme ← scheme: public
    $options ← array_diff_key($attributes, array('name' => 0, 'scheme' => 0))

  6. +  public function getName() {
    +    return t('Private files');

    Can we rename this method to getLabel()?

    Aside from a few outliers, "name" refers to an internal machine name in D8.

  7. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperInterface.php
    +  const READ = 0x0004;
    +  const WRITE = 0x0008;

    Since we're touching all affected code anyway already, can we rename these constants to READABLE and WRITABLE?

  8. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperInterface.php
    +  const LOCAL_HIDDEN = 0x000D;
    +  const WRITE_VISIBLE = 0x001C;
    +  const READ_VISIBLE = 0x0014;
    +  const LOCAL_NORMAL = 0x001D;

    Hm. I guess it's out of scope here, but these combined helper constants do not make sense to me... The base constants appear to be bit-wise OR'ed flags already.

    But yeah, I guess that's out of scope here.

  9. +  public function getViaScheme($scheme) {
    +  public function getViaUri($uri) {

    Can we rename these methods to getBy*()?

  10. +  protected function getWrapper($scheme, $uri) {
    +    if (isset($this->services[$scheme])) {
    +      $instance = $this->container->get($this->services[$scheme]);
    ...
    +    $this->services[$scheme] = $service_id;
    ...
    +    $this->services = array_intersect_key($this->services, $this->info);

    It's not clear to me why the services are being tracked?

    As mentioned earlier in this issue, the actual registry of stream wrappers is maintained by PHP core itself. Stream wrapper instances are created by PHP core, not by the Drupal application.

    Ideally, the individual service definitions would actually cease to exist, once the compiler pass has run. That is, because a stream wrapper class is not supposed to be used on its own. (But I'm not sure whether that is possible with the proposed implementation.)

    As a direct consequence, getWrapper() should not exist, because Drupal is not supposed to instantiate these classes.

  11. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    +   * Internal use only.
    ...
    +   * Internal use only.
    ...
    +   * Internal use only.

    Nice use-case for @internal instead? :-)

  12. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    +  public function register() {
    +    $this->moduleHandler->alter('stream_wrappers', $this->info);

    I still think that allowing any stream wrapper to be altered by other modules is a fundamentally flawed design...

    However, because it's going to be hard to make a case for removing that facility in this issue, let me actually suggest to replace this mechanism with a more sensible one:

    1. Introduce 'priority' for tagged services, 0 by default.
    2. When collecting tagged services, allow a service with a higher priority to override the scheme of a service with a lower priority.

    → That gets rid of the module handler dependency + alter hook, while achieving the exact same thing.

  13. +++ b/core/modules/locale/locale.services.yml
    +  stream_wrapper.translations:

    For simplicity, I'd recommend to name all the individual stream wrapper service names "stream.$scheme"

    That is, because in some other settings.php related issue, the proposal is to standardize the setting names for specifying local stream file paths equally to:

    $settings['stream']['public'] = 'sites/default/files';

  14. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
    @@ -71,13 +72,11 @@
    -  private $streamWrappers = array();
    +  protected $streamWrappers = array();

    Obsolete?

  15. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
    @@ -267,6 +266,12 @@ public function containerBuild(ContainerBuilder $container) {
    +    // Register the stream wrapper manager.
    +    $container
    +      ->register('stream_wrapper_manager', 'Drupal\Core\StreamWrapper\StreamWrapperManager')

    I wonder why this is necessary in DUTB::containerBuild()?

  16. +++ b/core/modules/system/system.module
    @@ -715,35 +715,13 @@ function system_theme_suggestions_field(array $variables) {
    +function system_stream_wrappers_alter(&$wrappers) {
       // Only register the private file stream wrapper if a file path has been set.
    ...
    +  if (!\Drupal::config('system.file')->get('path.private')) {
    +    unset($wrappers['private']);
       }

    Hm. OK, now I remember the use-case of the alter hook...

    That said, why is the stream wrapper scheme registration bound to some configuration value...?

    The stream wrapper can be registered sans configuration. It will only blow up upon usage. I.e., registration != availability/usage.

    I think we need to decouple the registration from the usage (and possibly availability check).

    For optionally available stream wrappers, the availability check needs to happen either way. Therefore, it doesn't matter whether the stream wrapper is already registered or not. What matters is that the calling code is able to check whether the stream wrapper is able to operate.

    This could simply be a isAvailable() method on the manager, which proxies to the stream wrapper class?

    This obviously circles back into the fundamental topic of how to architect stream wrappers with configurable base storage settings. :-/

Again, please interpret this review with a fine grain of salt... — there's so much wrong in our current architecture, it's very hard to draw a line where this effort should and could reasonably end, and which parts need to be addressed in separate follow-up issues.

Obviously, we want to avoid follow-up issues causing additional API changes at this point in time, but at the same time, this patch is 70+KB already, and as @catch already mentioned, the stream wrapper API only has a handful of consumers either way...

Aside from the easy remarks, the reliance on services in the manager architecturally feels most wrong to me. I'm not sure whether that $uri property is still used by any code? If it is, then I guess my remarks are asking for too much to be tackled here. But if it is not, then that entire getWrappers() method + dependency on services appears to be obsolete.

I'm perfectly happy to defer changes to follow-ups, but yeah, let's make sure to find a reasonable middle-ground for this issue. :-)

150: stream-wrappers.150.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 150: stream-wrappers.150.patch, failed testing.