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.
Comment | File | Size | Author |
---|---|---|---|
#205 | 2028109-stream_wrappers-205.patch | 72.78 KB | almaudoh |
#205 | interdiff.txt | 1.43 KB | almaudoh |
#201 | interdiff.txt | 1.36 KB | almaudoh |
#201 | 2028109-stream_wrappers-201.patch | 72.32 KB | almaudoh |
#199 | 2028109-stream_wrappers-199.patch | 72.2 KB | almaudoh |
Comments
Comment #1
tim.plunkettHere'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.
Comment #2
tim.plunkettThis 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.
Comment #4
tim.plunkettComment #5
tim.plunkettLet's see if this breaks anything:
Comment #7
tim.plunkettThis worked nicely.
Comment #9
tim.plunkettSince stream wrappers need to be registered on every request, we don't want persistant caching, the static caching is equivalent to what HEAD does.
Comment #11
tim.plunkett#9: stream-wrappers-2028109-9.patch queued for re-testing.
Comment #13
tim.plunkettWe need an empty StreamWrapperManager during installation.
Comment #14
tim.plunkettPer chx, we can remove yet another special case from UpdateModuleHandler.
Comment #16
tim.plunkettSilly mistake with the fall-through on that switch/case
Comment #18
tim.plunkettSeriously, 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.
Comment #19
BerdirJust 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.
Comment #20
jibranDear 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?
Comment #21
tim.plunkettAs part of #2044203: [meta] Convert info hooks to plugins
Comment #22
tim.plunkettComment #23
tim.plunkettThe person who told me to tag this was misinformed.
Comment #24
tim.plunkettThis is needed for #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class.
Comment #25
tim.plunkettRerolled!
Comment #27
tim.plunkettWe can put plugins wherever we want now!
Comment #29
tim.plunkettStupid mistakes.
Comment #31
tim.plunkett#29: stream-wrappers-2028109-29.patch queued for re-testing.
Comment #32
tim.plunkettIn order to do #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class properly, this needs to go in first.
Comment #32.0
tim.plunkettUpdated issue summary.
Comment #33
tim.plunkettReroll for patch fuzz, and I updated the issue summary.
Comment #34
amateescu CreditAttribution: amateescu commentedWould 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 :)
Comment #35
tim.plunkettI 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 :)
Comment #36
amateescu CreditAttribution: amateescu commentedOk then, let's do this.
Comment #37
effulgentsia CreditAttribution: effulgentsia commentedStream 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.
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.
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.
Comment #38
tim.plunkettThe
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
, thenDrupal\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.
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedNot 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.
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedWell, 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.
Comment #41
tim.plunkettI 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.
Comment #42
amateescu CreditAttribution: amateescu commentedI'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?Comment #43
tim.plunkettIf 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.
Comment #44
amateescu CreditAttribution: amateescu commentedDude, 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.
Comment #45
dlu CreditAttribution: dlu commentedMoving 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...).
Comment #46
tim.plunkettMy apologies to @amateescu, my comment about being offtopic was in reference to *me* derailing the issue, not him.
Rerolling, and seeing what caching breaks.
Comment #48
tim.plunkettYeah 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.
Comment #49
tim.plunkettI fixed devel_generate so it would work with images :)
Some kind soul, have at it!
Comment #50
tim.plunkettRerolled for the config() to Drupal::config() conversion.
Comment #51
benjy CreditAttribution: benjy commentedOK I did some xhprof profiling, overall results here: without patch: 520f15feb42f4 with patch: 520f15bcce8ff
Comment #52
effulgentsia CreditAttribution: effulgentsia commented#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?
Comment #53
tim.plunkettOkay.
Comment #55
tim.plunkettRight, we don't get the nice defaults from the annotation anymore.
Comment #56
tim.plunkettHere's a reroll of the old patch.
Comment #56.0
tim.plunkettUpdated
Comment #57
slashrsm CreditAttribution: slashrsm commentedUpdated issue summary.
Comment #58
slashrsm CreditAttribution: slashrsm commentedAttached 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.
Comment #60
slashrsm CreditAttribution: slashrsm commentedSome semi-smart cache clears added. Hope this will help.
Comment #62
tim.plunkettHow 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?
Comment #63
slashrsm CreditAttribution: slashrsm commented@tim.plunkett: getWrappers() is called from file_get_stream_wrappers(), which is called from _drupal_bootstrap_full(). Shouldn't that be enough?
Comment #64
benjy CreditAttribution: benjy commentedOK I've tested #55 and #56.
HEAD: 5217f06eec80f
#55: 5217f0ea61cc2
#56: 5217f152d5f71
HEAD and #55
HEAD and #56
Comment #65
tim.plunkettI 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.
Comment #67
tim.plunkettThe Locale stream wrapper isn't working. No idea.
Comment #68
benjy CreditAttribution: benjy commentedTested #67
HEAD: 521851729b64b
#67: 52185125e7a9d
Comment #69
benjy CreditAttribution: benjy commentedOK, 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
Comment #71
benjy CreditAttribution: benjy commentedAs requested, I tested #56 against HEAD again this time doing each test 5 times and excluding any outliers.
HEAD: 5219456189fcc
#56: 5219477745219
Comment #72
tim.plunkettSo 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.
Comment #73
slashrsm CreditAttribution: slashrsm commentedDid some more profiling. 5 subsequent requests to homepage, caches are empty at 1st request.
Results:
https://docs.google.com/spreadsheet/ccc?key=0AuCzOZ1L52ODdDhSWU1kOGQ5RXh...
Comment #74
catchThe 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.
Comment #75
slashrsm CreditAttribution: slashrsm commentedYou are right. Diff was inverted. Fixed.
Comment #76
jibran#72: stream-wrappers-2028109-72.patch queued for re-testing.
Comment #78
tim.plunkett#72: stream-wrappers-2028109-72.patch queued for re-testing.
Comment #80
jibran#72: stream-wrappers-2028109-72.patch queued for re-testing.
Comment #82
h3rj4n CreditAttribution: h3rj4n commentedThis issue needs a reroll.
Comment #83
larowlanRe-roll
Still doesn't install
Tried to find the issue but it looks annotation related and they all look ok to me :(
Comment #85
larowlanwill try and nut this out later today
Comment #86
larowlanLets see how this goes
Comment #87
andypostshould use
\Drupal::
Comment #88
mcrittenden CreditAttribution: mcrittenden commentedComment #89
tim.plunkettReroll.
Comment #90
tim.plunkettMissed the review in #87
Comment #91
jibranI think we should add @deprecated to docs as well.
We can remove use as it is in the same namespace.
80 chars limit
Comment #92
tim.plunkettI 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.
Comment #93
BerdirYes, but now, it's based on hook discovery, this changes it to annotation based discovery. That's a considerable regression, happening on every request :(
Comment #94
catchYep.
Comment #95
tim.plunkettOkay, let's just use HookDiscovery then, and be done with it. We still want to clean up file.inc
Comment #96
jibranyeah 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.
Comment #97
tim.plunkett#95: stream-wrappers-2028109-95.patch queued for re-testing.
Comment #98
catchWhy 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.
Comment #98.0
catchUpdate issue summary from #33 to #56
Comment #99
star-szrQuick reroll.
Comment #100
slashrsm CreditAttribution: slashrsm commentedIt 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?
Comment #101
Dave ReidComment #102
slashrsm CreditAttribution: slashrsm commentedAs per IRC discussion with @tim.plunkett. This patch is a re-roll of #67, which looks like the last annotation-style patch with caching.
Comment #104
slashrsm CreditAttribution: slashrsm commentedShould install now...
Comment #106
sunComment #107
sunHm. I'd like to raise some concerns.
While this patch certainly follows D8's architecture,
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.
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?
It feels Just Wrong™ that extensions are able to swap out custom stream wrappers at runtime.
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):
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?
Comment #108
sunTo 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.
Comment #109
twistor CreditAttribution: twistor commentedGaufrette'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.
Comment #110
sunYeah, 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) andDrupalKernel::shutdown()
to unregister them. That way, the kernel continues to manage application state as usual.Comment #111
Anonymous (not verified) CreditAttribution: Anonymous commentedi agree with #110. this is low level stuff.
Comment #112
tim.plunkettAt 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...
Comment #113
catchTagged 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.
Comment #114
twistor CreditAttribution: twistor commentedI'll take a blind stab at this today.
Comment #115
tim.plunkettI 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.
Comment #116
sunWe need a Drupal\Component\StreamWrapper\StreamWrapperManager class to manage registrations anyway. That class can hold these constants; e.g.:
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()
andstream_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).
Comment #117
tim.plunkettI 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.
Comment #118
twistor CreditAttribution: twistor commentedSorry, 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.
Comment #119
twistor CreditAttribution: twistor commentedAlrighty, this still needs work, but here's the idea.
Comment #120
twistor CreditAttribution: twistor commentedComment #121
twistor CreditAttribution: twistor commentedThis steals one line from #1376122: Stream wrappers of parent site are leaking into all tests regarding re-registering the stream wrappers during restoreEnvironment().
Comment #124
twistor CreditAttribution: twistor commentedAlright, 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.
Comment #125
larowlan+1 to removing description/name translatability.
See the ugly workarounds needed in #2016629: Refactor bootstrap to better utilize the kernel to get that working
Comment #126
twistor CreditAttribution: twistor commentedThis 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.
Comment #127
sunThanks 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:
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:
Whereas all consuming code should simply look like this:
→ 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 implementchmod()
,realpath()
, anddirname()
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.
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.
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:
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.
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()
intoinitialize()
and denote availability based on the return value instead: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:
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:
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
:In other words:
./StreamWrapper/
subdirectory.@StreamWrapper
annotation on each class.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.
Comment #129
twistor CreditAttribution: twistor commentedI haven't addressed #127 yet, just getting things to pass.
Comment #132
twistor CreditAttribution: twistor commentedhmm
Comment #134
twistor CreditAttribution: twistor commentedForgot to remove temporary stream wrapper from system.services.
Comment #136
twistor CreditAttribution: twistor commentedWell, I suppose if I upload the right patch.
Interdiff is in #134.
Comment #139
twistor CreditAttribution: twistor commentedOne dependency in!
I lost the interdiff during the re-roll.
Let's see what this does.
Comment #140
twistor CreditAttribution: twistor commentedSo 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...
Comment #141
sunThe 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.
Comment #142
catchI'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.
Comment #143
dawehner140: 2028109-140.patch queued for re-testing.
Comment #145
larowlanreroll
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
Comment #147
larowlandoh
Comment #149
jibran@var block missing.
more then 80 chars.
Comment #150
larowlanFixes #149 and hopefully failing test
Comment #151
tim.plunkettI haven't worked on this since we switched from plugins, so I feel okay reviewing and hopefully RTBCing this soon
I don't understand why we're rearranging all of these. If there is some important ordering here, can we get some inline comments?
We should back our services with interfaces.
There are still a couple of these strewn about.
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?
Comment #152
larowlan1) bad merge, will revert
2) will do
3) will fix
4) :)
5) (unit tests) will take a look
Comment #153
sunHey, 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...
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.
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.
I agree with @tim.plunkett, would be great to remove the unrelated import ordering changes here.
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...
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))
Can we rename this method to
getLabel()
?Aside from a few outliers, "name" refers to an internal machine name in D8.
Since we're touching all affected code anyway already, can we rename these constants to READABLE and WRITABLE?
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.
Can we rename these methods to
getBy*()
?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.Nice use-case for @internal instead? :-)
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:
→ That gets rid of the module handler dependency + alter hook, while achieving the exact same thing.
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';
Obsolete?
I wonder why this is necessary in DUTB::containerBuild()?
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 entiregetWrappers()
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. :-)
Comment #154
sunComment #155
saltednutComment #156
saltednut150: stream-wrappers.150.patch queued for re-testing.
Comment #158
xjmThe summary and the issue title disagree:
Comment #159
BerdirRe-roll and addressed #151.1.
Comment #161
Berdir159: stream-wrappers.159.patch queued for re-testing.
Comment #163
Berdir159: stream-wrappers.159.patch queued for re-testing.
Comment #164
tim.plunkettAttempted PSR-4 reroll.
Comment #165
slashrsm CreditAttribution: slashrsm commentedShould we mark this as @deprecated?
Same as above (+ some other similar cases).
PHPDoc missing.
PHPDoc missing.
Comment #166
Gábor HojtsyWill 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?
Comment #167
BerdirRe-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().
Comment #169
BerdirOh 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 :(
Comment #171
BerdirOk, 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.
Comment #172
neclimdulStraight re-roll since I don't have the larger understanding to fix this but needed to re-roll to test. Review incoming.
Comment #173
neclimdulGrabbed a couple tests locally and ran them. This is what I found:
These are what are causing the exception.
On unregister this iterates over $this->wrappers[StreamWrapperInterface::ALL] without checking it exists which is what's throwing the warning locally.
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.
Comment #174
ArlaRerolled again and changed status to trigger tests. Have not considered the suggestion in #173.
Comment #176
ArlaWith the risk of repeating #171, as @Berdir explained it to me,
register()
is called once in kernel, so stream wrappers are registered in PHP withstream_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 invokeregister()
,$this->wrappers
will be empty when in the endunregister()
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.Comment #178
tim.plunkettI can't deal with ConfigImportAllTest right now, I've had enough of it in #2326409: Annotate render element plugins. But here's a reroll.
Comment #180
tim.plunkettComment #184
almaudoh CreditAttribution: almaudoh commentedLet me take a look at this.
Comment #185
almaudoh CreditAttribution: almaudoh commentedStraight reroll. Let's see what testbot says...
Comment #187
almaudoh CreditAttribution: almaudoh commentedOld 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.
Comment #189
almaudoh CreditAttribution: almaudoh commentedThis should fix the 2 exceptions.
Comment #190
almaudoh CreditAttribution: almaudoh commentedComment #191
almaudoh CreditAttribution: almaudoh commentedRemoved some old dead code.
Comment #192
Fabianx CreditAttribution: Fabianx commentedOMG, its green! Great work!
Comment #193
Fabianx CreditAttribution: Fabianx commentedRTBC - 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?
Comment #194
andypost@Fabianx nice idea, please file issue
Comment #195
Fabianx CreditAttribution: Fabianx commentedFiled #2353357: hook_stream_wrappers_alter() should be removed as it is broken since modules are not loaded on demand for the follow-up.
Comment #196
catchWhy can't this use $this->container?
Should this be injected into the ModuleHandler?
Shouldn't this also remove drupal_static_reset('file_get_stream_wrappers'); above?
Comment #197
almaudoh CreditAttribution: almaudoh commentedThanks @catch. Removed unnecessary
drupal_static_reset()
calls and also cleaned up multiple superfluous calls toStreamWrapperManager::register()
. Hope I didn't break something :pPer #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?Comment #199
almaudoh CreditAttribution: almaudoh commentedMeh
Comment #201
almaudoh CreditAttribution: almaudoh commentedI did go out on a limb :)
Interdiff is against #197
Comment #205
almaudoh CreditAttribution: almaudoh commentedInterdiff still against #197
Comment #206
Fabianx CreditAttribution: Fabianx commentedCore committer feedback has been addressed, interdiff looks good, back to RTBC.
Comment #207
catchCommitted/pushed to 8.0.x, thanks!
Comment #209
almaudoh CreditAttribution: almaudoh commentedCoool!!! :)
Comment #210
almaudoh CreditAttribution: almaudoh commentedEdit: duplicate comment removed
Comment #212
yched CreditAttribution: yched commentedFollowup : this issue moved TranslationsStream.php, but left the old file in place
#2430927: Duplicate TranslationsStream class