Updated: Comment #57

Problem/Motivation

Stream wrappers are yet another info hook/callback pairing, and those have moved to annotated plugins. They are low-level and better implemented as tagged container services. In addition, it helps clean up and modernize file.inc.

Proposed resolution

Rewrite stream wrappers as tagged services on the container. The service container is used as a mechanism to "expose" (not register) stream wrapper classes via service providers, so as to collect them during a compiler pass and register them when the kernel boots (and unregister them when it shuts down).
Leave the procedural wrappers intact for now.

Remaining tasks

Review patch.
Commit patch.

User interface changes

N/A

API changes

hook_stream_wrappers() is gone. Stream wrappers are now classes implementing \Drupal\Core\StreamWrapper\StreamWrapperInterface and exposed as tagged services via *.services.yml files.

CommentFileSizeAuthor
#205 2028109-stream_wrappers-205.patch72.78 KBalmaudoh
#205 interdiff.txt1.43 KBalmaudoh
#201 interdiff.txt1.36 KBalmaudoh
#201 2028109-stream_wrappers-201.patch72.32 KBalmaudoh
#199 2028109-stream_wrappers-199.patch72.2 KBalmaudoh
#199 interdiff.txt593 bytesalmaudoh
#197 2028109-stream_wrappers-197.patch72.21 KBalmaudoh
#197 interdiff.txt4.91 KBalmaudoh
#191 2028109-stream_wrappers-191.patch71.18 KBalmaudoh
#191 interdiff2.txt2.85 KBalmaudoh
#189 2028109-stream_wrappers-189.patch71.17 KBalmaudoh
#189 interdiff.txt1.14 KBalmaudoh
#187 2028109-stream_wrappers-187.patch71.02 KBalmaudoh
#187 interdiff.txt2.45 KBalmaudoh
#185 2028109-stream_wrappers-185.patch68.98 KBalmaudoh
#178 2028109-stream_wrappers-178.patch69.36 KBtim.plunkett
#176 streamwrapper_services-2028109-176.interdiff.txt3.74 KBArla
#176 streamwrapper_services-2028109-176.patch69.25 KBArla
#174 streamwrapper_services-2028109-174.patch68.31 KBArla
#172 2028109-172.patch68.14 KBneclimdul
#169 stream_wrappers-2028109-169-interdiff.txt868 bytesBerdir
#169 stream_wrappers-2028109-169.patch68.25 KBBerdir
#167 stream_wrappers-2028109-167-interdiff.txt4.02 KBBerdir
#167 stream_wrappers-2028109-167.patch67.29 KBBerdir
#164 stream_wrappers-2028109-164.patch66.06 KBtim.plunkett
#159 stream-wrappers.159-interdiff.txt2.55 KBBerdir
#159 stream-wrappers.159.patch68.08 KBBerdir
#150 stream-wrappers.150.patch71.06 KBlarowlan
#150 interdiff.txt4.89 KBlarowlan
#147 stream-wrappers.147.patch70.8 KBlarowlan
#145 stream-wrappers.145.patch69.37 KBlarowlan
#140 2028109-140.patch72.46 KBtwistor
#140 interdiff.txt1.61 KBtwistor
#139 2028109-139.patch73.26 KBtwistor
#136 2028109-135.patch63.77 KBtwistor
#134 2028109-134.patch1.18 KBtwistor
#134 interdiff.txt1.18 KBtwistor
#129 2028109-129.patch63.89 KBtwistor
#129 interdiff.txt6.78 KBtwistor
#126 2028109-126.patch62.39 KBtwistor
#126 interdiff.txt12.91 KBtwistor
#124 2028109-124.patch60.22 KBtwistor
#124 interdiff.txt6.89 KBtwistor
#121 2028109-121.patch59 KBtwistor
#119 2028109-119.patch57.33 KBtwistor
#104 interdiff.txt955 bytesslashrsm
#104 2028109_104.patch46.89 KBslashrsm
#102 2028109_102.patch46.9 KBslashrsm
#99 2028109-99.patch29.26 KBstar-szr
#95 stream-wrappers-2028109-95.patch29.23 KBtim.plunkett
#92 stream-wrapper-2028109-92.patch42.44 KBtim.plunkett
#92 interdiff.txt6.41 KBtim.plunkett
#90 stream-wrappers-2028109-90.patch42.45 KBtim.plunkett
#90 interdiff.txt1.69 KBtim.plunkett
#89 interdiff.txt7.65 KBtim.plunkett
#89 stream-wrappers-2028109-89.patch42.44 KBtim.plunkett
#86 interdiff.txt804 byteslarowlan
#86 stream-wrappers-2028109.86.patch40.08 KBlarowlan
#83 stream-wrappers-2028109.83.patch40.08 KBlarowlan
#72 stream-wrappers-2028109-72.patch40.05 KBtim.plunkett
#67 stream_wrappers-2028109-67.patch42.92 KBtim.plunkett
#67 interdiff.txt650 bytestim.plunkett
#65 interdiff-from-56.txt6.19 KBtim.plunkett
#65 interdiff-from-60.txt6.67 KBtim.plunkett
#65 stream_wrappers-2028109-65.patch42.82 KBtim.plunkett
#60 drupal8.file-system.2028109-60.patch40.95 KBslashrsm
#60 interdiff.txt1.63 KBslashrsm
#58 drupal8.file-system.2028109-58.patch39.79 KBslashrsm
#58 interdiff.txt4.23 KBslashrsm
#56 stream-wrappers-2028109-56-do-not-test.patch40.05 KBtim.plunkett
#55 stream-wrappers-2028109-55.patch22.88 KBtim.plunkett
#55 interdiff.txt732 bytestim.plunkett
#53 stream-wrappers-2028109-53.patch22.68 KBtim.plunkett
#53 interdiff.txt22.47 KBtim.plunkett
#50 stream-wrappers-2028109-50.patch40 KBtim.plunkett
#48 stream-wrappers-2028109-48.patch39.99 KBtim.plunkett
#46 interdiff.txt3.06 KBtim.plunkett
#46 stream-wrappers-2028109-46.patch40.47 KBtim.plunkett
#33 stream-wrapper-2028109-33.patch40.49 KBtim.plunkett
#29 stream-wrappers-2028109-29.patch40.49 KBtim.plunkett
#29 interdiff.txt2.08 KBtim.plunkett
#27 stream-wrappers-2028109-27.patch42.24 KBtim.plunkett
#27 interdiff.txt9.88 KBtim.plunkett
#25 stream-wrappers-2028109-25.patch42.78 KBtim.plunkett
#18 stream-wrappers-2028109-18.patch42.66 KBtim.plunkett
#18 interdiff-from-green.txt1.37 KBtim.plunkett
#18 interdiff.txt1.19 KBtim.plunkett
#16 stream-wrapper-2028109-16.patch42.49 KBtim.plunkett
#16 interdiff.txt652 bytestim.plunkett
#14 stream-wrappers-2028109-14.patch42.32 KBtim.plunkett
#14 interdiff.txt1.03 KBtim.plunkett
#13 interdiff.txt853 bytestim.plunkett
#13 stream-wrappers-2028109-13.patch41.29 KBtim.plunkett
#9 stream-wrappers-2028109-9.patch40.45 KBtim.plunkett
#9 interdiff.txt2.34 KBtim.plunkett
#7 stream-wrappers-2028109-7.patch40.93 KBtim.plunkett
#7 interdiff.txt18.23 KBtim.plunkett
#5 stream-wrappers-2028109-5.patch24.19 KBtim.plunkett
#4 stream-wrapper-2028109-3.patch24.19 KBtim.plunkett
#4 interdiff.txt2.45 KBtim.plunkett
#1 stream-wrapper-2028109-1.patch21.75 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Active » Needs review
FileSize
21.75 KB

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

Title: Convert hook_stream_wrappers() to plugin system? » Convert hook_stream_wrappers() to plugin system
FileSize
2.45 KB
24.19 KB
tim.plunkett’s picture

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');
   }
 
 }
tim.plunkett’s picture

This worked nicely.

tim.plunkett’s picture

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.

tim.plunkett’s picture

Issue tags: -Plugin-conversion

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

tim.plunkett’s picture

Issue tags: +Plugin-conversion
FileSize
41.29 KB
853 bytes

We need an empty StreamWrapperManager during installation.

tim.plunkett’s picture

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

tim.plunkett’s picture

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

tim.plunkett’s picture

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.

Berdir’s picture

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.

jibran’s picture

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

tim.plunkett’s picture

tim.plunkett’s picture

Priority: Normal » Critical
tim.plunkett’s picture

Issue tags: -Approved API change

The person who told me to tag this was misinformed.

tim.plunkett’s picture

tim.plunkett’s picture

Rerolled!

tim.plunkett’s picture

We can put plugins wherever we want now!

tim.plunkett’s picture

Stupid mistakes.

tim.plunkett’s picture

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

tim.plunkett’s picture

Issue tags: +blocker
tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

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

amateescu’s picture

+++ 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 :)

tim.plunkett’s picture

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 :)

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Ok then, let's do this.

effulgentsia’s picture

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

tim.plunkett’s picture

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.

effulgentsia’s picture

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.

effulgentsia’s picture

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.

tim.plunkett’s picture

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.

amateescu’s picture

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?

tim.plunkett’s picture

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.

amateescu’s picture

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.

dlu’s picture

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

tim.plunkett’s picture

Component: base system » file system
Status: Needs work » Needs review
FileSize
40.47 KB
3.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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
39.99 KB
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.

tim.plunkett’s picture

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!

tim.plunkett’s picture

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

benjy’s picture

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

#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?

tim.plunkett’s picture

Okay.

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
732 bytes
22.88 KB

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

tim.plunkett’s picture

Here's a reroll of the old patch.

tim.plunkett’s picture

Issue summary: View changes

Updated

slashrsm’s picture

Updated issue summary.

slashrsm’s picture

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.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
40.95 KB

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.

tim.plunkett’s picture

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?

slashrsm’s picture

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

benjy’s picture

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%
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
42.82 KB
6.67 KB
6.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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
650 bytes
42.92 KB

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

benjy’s picture

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

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.

benjy’s picture

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%
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
40.05 KB

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.

slashrsm’s picture

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

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

catch’s picture

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.

slashrsm’s picture

You are right. Diff was inverted. Fixed.

jibran’s picture

Status: Needs review » Needs work

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

tim.plunkett’s picture

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.

jibran’s picture

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.

h3rj4n’s picture

Issue tags: +Needs reroll

This issue needs a reroll.

larowlan’s picture

Status: Needs work » Needs review
FileSize
40.08 KB

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.

larowlan’s picture

will try and nut this out later today

larowlan’s picture

Status: Needs work » Needs review
FileSize
40.08 KB
804 bytes

Lets see how this goes

andypost’s picture

+++ 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::

mcrittenden’s picture

Issue tags: -Needs reroll
tim.plunkett’s picture

Reroll.

tim.plunkett’s picture

Missed the review in #87

jibran’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.41 KB
42.44 KB

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.

Berdir’s picture

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 :(

catch’s picture

Yep.

tim.plunkett’s picture

Issue tags: -needs profiling
FileSize
29.23 KB

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

jibran’s picture

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.

tim.plunkett’s picture

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

catch’s picture

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.

catch’s picture

Issue summary: View changes

Update issue summary from #33 to #56

star-szr’s picture

Issue summary: View changes
FileSize
29.26 KB

Quick reroll.

slashrsm’s picture

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?

Dave Reid’s picture

Issue tags: +Media Initiative
slashrsm’s picture

FileSize
46.9 KB

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.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
46.89 KB
955 bytes

Should install now...

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

sun’s picture

Issue tags: +API clean-up, +@deprecated
sun’s picture

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):
      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?

sun’s picture

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.

twistor’s picture

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.

sun’s picture

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.

Anonymous’s picture

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

tim.plunkett’s picture

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

catch’s picture

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.

twistor’s picture

Assigned: Unassigned » twistor

I'll take a blind stab at this today.

tim.plunkett’s picture

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.

sun’s picture

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

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

tim.plunkett’s picture

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.

twistor’s picture

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.

twistor’s picture

Related issues: +#1376122: Stream wrappers of parent site are leaking into all tests
FileSize
57.33 KB

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

twistor’s picture

Title: Convert hook_stream_wrappers() to plugin system » Convert hook_stream_wrappers() to tagged services.
twistor’s picture

FileSize
59 KB

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.

twistor’s picture

FileSize
6.89 KB
60.22 KB

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.

larowlan’s picture

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

twistor’s picture

Status: Needs work » Needs review
FileSize
12.91 KB
62.39 KB

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.

sun’s picture

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:

      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:

      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:

    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:

    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:

    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:

    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.

twistor’s picture

Status: Needs work » Needs review
FileSize
6.78 KB
63.89 KB

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.

twistor’s picture

Status: Needs work » Needs review

hmm

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

twistor’s picture

FileSize
1.18 KB
1.18 KB

Forgot to remove temporary stream wrapper from system.services.

Status: Needs review » Needs work

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

twistor’s picture

Status: Needs work » Needs review
FileSize
63.77 KB

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.

twistor’s picture

Status: Needs work » Needs review
FileSize
73.26 KB

One dependency in!

I lost the interdiff during the re-roll.

Let's see what this does.

twistor’s picture

FileSize
1.61 KB
72.46 KB

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

sun’s picture

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.

catch’s picture

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.

dawehner’s picture

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

Status: Needs review » Needs work

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
69.37 KB

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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
70.8 KB

doh

Status: Needs review » Needs work

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

jibran’s picture

  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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
4.89 KB
71.06 KB

Fixes #149 and hopefully failing test

tim.plunkett’s picture

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\File\FileSystem class due to the other function calls?

larowlan’s picture

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

sun’s picture

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. :-)

sun’s picture

saltednut’s picture

saltednut’s picture

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.

xjm’s picture

The summary and the issue title disagree:

Rewrite stream wrappers to be plugins (they are already PSR-0 classes)

Berdir’s picture

Status: Needs work » Needs review
FileSize
68.08 KB
2.55 KB

Re-roll and addressed #151.1.

Status: Needs review » Needs work

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

Berdir’s picture

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

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

Berdir’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
66.06 KB

Attempted PSR-4 reroll.

slashrsm’s picture

  1. +++ b/core/includes/file.inc
    @@ -156,122 +85,40 @@
    +function file_get_stream_wrappers($filter = StreamWrapperInterface::ALL) {
    +  return \Drupal::service('stream_wrapper_manager')->getWrappers($filter);
     }
     
    

    Should we mark this as @deprecated?

  2. +++ b/core/includes/file.inc
    @@ -156,122 +85,40 @@
     function file_stream_wrapper_get_class($scheme) {
    -  $wrappers = file_get_stream_wrappers();
    -  return empty($wrappers[$scheme]) ? FALSE : $wrappers[$scheme]['class'];
    +  return \Drupal::service('stream_wrapper_manager')->getClass($scheme);
     }
     
    

    Same as above (+ some other similar cases).

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,291 @@
    +  /**
    +   * @todo
    +   */
    +  public function getNames($filter = StreamWrapperInterface::ALL) {
    +    $names = array();
    +    foreach (array_keys($this->getWrappers($filter)) as $scheme) {
    

    PHPDoc missing.

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,291 @@
    +  /**
    +   * @todo
    +   */
    +  public function getDescriptions($filter = StreamWrapperInterface::ALL) {
    +    $descriptions = array();
    +    foreach (array_keys($this->getWrappers($filter)) as $scheme) {
    

    PHPDoc missing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Will not apply anymore due to #2244447: Translation of low-level info/annotations leads to circular dependencies. Seeing how the new tagged services have methods to get metadata instead of annotations, it may get confusing that these would be the only one using TranslationWrapper direcly. Or are the methods called late enough that is not a problem?

Berdir’s picture

Status: Needs work » Needs review
FileSize
67.29 KB
4.02 KB

Re-roll.

- Added @deprecated and documentation. Probably needs an interface too?

@Gabor: Exactly, the t() calls there were only a problem because they were always called during discovery and it wasn't even possible to cache this hook. The names and descriptions are only needed for UI's and forms, so that happens late enough to just call t() or $this->t().

Status: Needs review » Needs work

The last submitted patch, 167: stream_wrappers-2028109-167.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
68.25 KB
868 bytes

Oh man. I was debugging this for hours but I had the right workaround all along, but I was then running into #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in which confused the hell out of me.

WebTestBase::setUp() is a mess, there are so many container rebuilds and cache clears, somehow, $this->kernel is not the right kernel and/or does not have the right container injected, so the bootstrap there doesn't actually bootstrap anything on the right container :(

Status: Needs review » Needs work

The last submitted patch, 169: stream_wrappers-2028109-169.patch, failed testing.

Berdir’s picture

Ok, so the remaining exceptions are very likely because a container rebuild happens during those tests and then they're not longer registered.

I could easily fix the warnings but not sure if that's the right approach because we will likely end up with too many registered stream wrappers.

neclimdul’s picture

FileSize
68.14 KB

Straight re-roll since I don't have the larger understanding to fix this but needed to re-roll to test. Review incoming.

neclimdul’s picture

Grabbed a couple tests locally and ran them. This is what I found:

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -381,6 +381,7 @@ public function boot() {
    +    $this->container->get('stream_wrapper_manager')->register();
    
    @@ -393,6 +394,7 @@ public function shutdown() {
    +    $this->container->get('stream_wrapper_manager')->unregister();
    

    These are what are causing the exception.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,317 @@
    +    foreach (array_keys($this->wrappers[StreamWrapperInterface::ALL]) as $scheme) {
    

    On unregister this iterates over $this->wrappers[StreamWrapperInterface::ALL] without checking it exists which is what's throwing the warning locally.

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -0,0 +1,317 @@
    +  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']);
    +    }
    +  }
    

    This is where we register wrappers, and this is called by the kernel. registerWrapper is where $this->wrappers gets initialized sort of lazily. This is where the bug happens though because I assume no modules are registering stream wrappers in the kernel tests. Because wrappers is then unitialized in unregister, everything fails.

My suggestion would be to initialize the wrappers structure on construction. Not being familiar with prior work or design there may be a better solution though.

Arla’s picture

Status: Needs work » Needs review
FileSize
68.31 KB

Rerolled again and changed status to trigger tests. Have not considered the suggestion in #173.

Status: Needs review » Needs work

The last submitted patch, 174: streamwrapper_services-2028109-174.patch, failed testing.

Arla’s picture

Status: Needs work » Needs review
FileSize
69.25 KB
3.74 KB

With the risk of repeating #171, as @Berdir explained it to me, register() is called once in kernel, so stream wrappers are registered in PHP with stream_wrapper_register(). Upon a container rebuild, the StreamWrapperManager instance is thrown away and a new one created. The wrappers are still registered in PHP, so they can be used, but since the container rebuild does not invoke register(), $this->wrappers will be empty when in the end unregister() is called.

So this patch simply only calls stream_wrapper_unregister() if $this->wrappers is not empty. What's wrong with this is that we still don't keep the StreamWrapperManager state synchronized with the global PHP state. (The crux is of course that a service should not have global state, while stream wrapper registration is very much global state.) But it works on a few tests that I ran locally.

I also noted that $info and $services are overlapping, so I merged them. Hope I didn't miss something there.

Status: Needs review » Needs work

The last submitted patch, 176: streamwrapper_services-2028109-176.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
69.36 KB

I can't deal with ConfigImportAllTest right now, I've had enough of it in #2326409: Annotate render element plugins. But here's a reroll.

Status: Needs review » Needs work

The last submitted patch, 178: 2028109-stream_wrappers-178.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 178: 2028109-stream_wrappers-178.patch, failed testing.

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

almaudoh’s picture

Assigned: twistor » almaudoh

Let me take a look at this.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
68.98 KB

Straight reroll. Let's see what testbot says...

Status: Needs review » Needs work

The last submitted patch, 185: 2028109-stream_wrappers-185.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
71.02 KB

Old file_get_stream_wrappers() used to register the stream wrappers as well, but new deprecated file_get_stream_wrappers() doesn't, so we invoke the StreamWrapperManager::register() method instead.

This should fix the one fail.

Status: Needs review » Needs work

The last submitted patch, 187: 2028109-stream_wrappers-187.patch, failed testing.

almaudoh’s picture

Assigned: almaudoh » Unassigned
Status: Needs work » Needs review
FileSize
1.14 KB
71.17 KB

This should fix the 2 exceptions.

almaudoh’s picture

almaudoh’s picture

Removed some old dead code.

Fabianx’s picture

OMG, its green! Great work!

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - This looks great. Too bad this still needs to call

hook_stream_wrappers_alter() as we need to load all modules for that, so no lazy bootstrap, yet ... :-/

Can we convert that to an event and subscribe to that event at compile time instead of at run time in a follow-up?

andypost’s picture

@Fabianx nice idea, please file issue

Fabianx’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -431,7 +433,7 @@ public function preHandle(Request $request) {
    +    \Drupal::service('stream_wrapper_manager')->register();
    

    Why can't this use $this->container?

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -865,7 +865,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
             drupal_static_reset('file_get_stream_wrappers');
    ...
    +        \Drupal::service('stream_wrapper_manager')->register();
    

    Should this be injected into the ModuleHandler?

    Shouldn't this also remove drupal_static_reset('file_get_stream_wrappers'); above?

almaudoh’s picture

Status: Needs work » Needs review
FileSize
4.91 KB
72.21 KB

Thanks @catch. Removed unnecessary drupal_static_reset() calls and also cleaned up multiple superfluous calls to StreamWrapperManager::register(). Hope I didn't break something :p

Per #196.2: found out \Drupal::service('xxx') is the general pattern in ModuleHandler (it's not container aware). Making it container aware would be too much for this patch. Maybe a follow up?

Status: Needs review » Needs work

The last submitted patch, 197: 2028109-stream_wrappers-197.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
593 bytes
72.2 KB

Meh

Status: Needs review » Needs work

The last submitted patch, 199: 2028109-stream_wrappers-199.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
72.32 KB
1.36 KB

I did go out on a limb :)
Interdiff is against #197

Status: Needs review » Needs work

The last submitted patch, 201: 2028109-stream_wrappers-201.patch, failed testing.

The last submitted patch, 201: 2028109-stream_wrappers-201.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
72.78 KB

Interdiff still against #197

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Core committer feedback has been addressed, interdiff looks good, back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed bf49837 on 8.0.x
    Issue #2028109 by tim.plunkett, almaudoh, twistor, larowlan, slashrsm,...
almaudoh’s picture

Coool!!! :)

almaudoh’s picture

Edit: duplicate comment removed

Status: Fixed » Closed (fixed)

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

yched’s picture

Followup : this issue moved TranslationsStream.php, but left the old file in place
#2430927: Duplicate TranslationsStream class