Problem/Motivation

The stream wrapper manager currently calls a hook_stream_wrappers_alter() very early in the request and always, which makes it impossible to lazy load modules.

After trying different attempts and debugging this, we discovered the following:

a) Completely broken, as we call a hook before the module system is loaded, so we can never find any hooks to invoke (#2392433: Stream wrappers are registered before page cache will fix that, though)
b) It also breaks hook_module_implements_alter() as those is called on the first invoked hook, which then also doesn't find any implementations.
c) As far as we can see, the alter hook serves no purpose. It was there for system_stream_wrappers_alter(), which was used to unset the private stream wrapper (which as mentioned above was never called and there were no test fails). #2170235: file_private_path should be in $settings, like file_public_path now moved this into a core service provider to dynamically register it. All you can do in the alter hook is remove it or provide a different implementation, all of those things are possible through service providers. A hook/even would only be necessary if we want to make different stream wrappers available dynamically per request. That seems wrong to even consider supporting.

None of the other service-based things provide alter hooks like this, this was just left there because we had it before.

Proposed resolution

Remove the hook.

Remaining tasks

- Write a test that makes sure that private stream wrapper is correctly available or not (through the file system configuration form, for example)

User interface changes

* None.

API changes

Remove hook_stream_wrappers_alter().

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because hook_stream_wrappers_alter() is completely broken
Issue priority Critical performance issue
Prioritized changes The main goal of this issue is performance.
Disruption No disruption for core/contrib because the current implementation is defect anyway.
Files: 
CommentFileSizeAuthor
#61 stream-wrapper-event-2353357-61-interdiff.txt2.96 KBBerdir
#61 stream-wrapper-event-2353357-61.patch6.22 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,494 pass(es). View
#54 stream-wrapper-event-2353357-54.patch4.55 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,449 pass(es). View
#54 stream-wrapper-event-2353357-54-interdiff.txt1.43 KBBerdir
#52 stream-wrapper-event-2353357-52.patch3.11 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,376 pass(es). View
#43 stream-wrapper-event-2353357-43-interdiff.txt2.12 KBBerdir
#43 stream-wrapper-event-2353357-43.patch8.81 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,166 pass(es). View
#39 stream-wrapper-event-2353357-39-interdiff.txt4.03 KBBerdir
#39 stream-wrapper-event-2353357-39.patch8.67 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#34 stream-wrapper-event-2353357-34.patch8.66 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,061 pass(es). View
#32 stream-wrapper-event-2353357-32-interdiff.txt609 bytesBerdir
#32 stream-wrapper-event-2353357-32.patch8.64 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,023 pass(es), 2 fail(s), and 3 exception(s). View
#30 stream-wrapper-event-2353357-30-interdiff.txt3.5 KBBerdir
#30 stream-wrapper-event-2353357-30.patch8.54 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,410 pass(es), 36 fail(s), and 1,607 exception(s). View
#28 stream-wrapper-event-2353357-28.patch6.9 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,309 pass(es), 366 fail(s), and 1,089 exception(s). View
#25 lazy-load-modules-2353357-25-interdiff.txt3.88 KBBerdir
#25 lazy-load-modules-2353357-25.patch52.05 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,979 pass(es), 302 fail(s), and 110 exception(s). View
#23 lazy-load-modules-2353357-23.patch48.18 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,440 pass(es), 1,489 fail(s), and 286 exception(s). View
#16 lazy-load-modules-2353357-16-interdiff.txt27.27 KBBerdir
#16 lazy-load-modules-2353357-16.patch49.16 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,465 pass(es), 14 fail(s), and 11 exception(s). View

Comments

dawehner’s picture

I am not sure whether we really want to throw an event on container dump time, this could load easily to people injecting services and we end up with potential infinite container rebuilds.
Yes, we already load the previous container on container rebuild time, but its worth to think about it and avoid problems in the first place.

On alternative way to alter stream wrappers is to simply use a {$module}ServiceProvider::alter() already. Would that be enough?

Fabianx’s picture

Oh, no I am not talking about throwing an event at container dump time.

I am just talking about tagged services, no clue about the right syntax, but like:

[@streamWrappersAlterSubscriber]

Then (if I do understand the event system correctly) we can just call all services tagged with that tag in the container to alter the stream wrappers.

Similar to how Twig Extensions work.

Unless I am missing something that should allow us to not having to load all module to enable them to be able to alter stream wrappers.

Unrelated:

- Tonight I dreamed of a hook_ "registry" in the container using my updated find_functions code ( similar to drupal_find_theme_functions) that would allow us for defined and specially registered hooks (not all hooks) to lazy load the modules that implement that hook.

Fabianx’s picture

Issue summary: View changes
catch’s picture

Agreed on critical - if we don't make this change before release, there's no chance to do it after (compared to other performance issues which would just be optimisation with no refactoring). Critical performance issue in its own right vs. performance being critically bad.

Berdir’s picture

Status: Active » Needs review
FileSize
3.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 53,383 pass(es), 13,170 fail(s), and 3,431 exception(s). View

Just playing around a bit, seeing what happens.

Most obvious issue atm is lots of issues about constants not being defined, like DRUPAL_OPTIONAL.

On an empty front page, the call into loadAll() doesn't happen until the first _theme() call.

Status: Needs review » Needs work

The last submitted patch, 5: lazy-load-modules-2353357-5.patch, failed testing.

Fabianx’s picture

Nice! So a new meta to put all 'define' constants to static properties on classes (which can be autoloaded)?

That could be a lot of nice Novice issues.

/me excited!

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
718 bytes

Well, all of those are going to be API changes unless we leave the old ones in place...

More hacks to fix the apparently most common case.

Status: Needs review » Needs work

The last submitted patch, 8: lazy-load-modules-2353357-8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
2.27 KB

We still have lots of random helper/api functions in module files, unsurprisingly. Fixing a few of those with manual load calls, we probably want to identify them and move to services or static methods or something.

Status: Needs review » Needs work

The last submitted patch, 10: lazy-load-modules-2353357-10.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,238 pass(es), 542 fail(s), and 341 exception(s). View
2.48 KB

More fixes.

Status: Needs review » Needs work

The last submitted patch, 12: lazy-load-modules-2353357-12.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
31.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,348 pass(es), 230 fail(s), and 251 exception(s). View
24.46 KB

More fixes, killing a few deprecated menu callbacks, loading module files in other cases. Everything very hacky, just trying to get to a point where it is working stable enough to do some performance tests, for example on a ajax route that doesn't invoke hooks.

Status: Needs review » Needs work

The last submitted patch, 14: lazy-load-modules-2353357-14.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
49.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,465 pass(es), 14 fail(s), and 11 exception(s). View
27.27 KB

More of the same.

Status: Needs review » Needs work

The last submitted patch, 16: lazy-load-modules-2353357-16.patch, failed testing.

Arla’s picture

Issue summary: View changes

Since #2028109: Convert hook_stream_wrappers() to tagged services., the hook is not only inconvenient for performance reasons. It is now also broken, and breaks module_implements_alter hooks.

The stream_wrappers_alter hook is now invoked before modules are loaded. Consequently, the module_implements_alter hook is, too. This blocks modules from having their module_implements_alter implementations discovered.

catch’s picture

Title: Remove hook_stream_wrappers_alter() and move to event that is added at compile time, so we don't have to load all modules » hook_stream_wrappers_alter() is broken since modules are not loaded on demand, also change to an event since it's the last hook that forces this during bootstrap
Category: Task » Bug report
Fabianx’s picture

#18: Uhm, should there not be a moduleHandler->loadAll() or such before calling hook_stream_wrappers_alter()?

And yes the module_implements_cache corruption is a variation of #496170: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion ...

I would suggest we get a quick-fix in, in another issue as this one is for the performance parts of this.

Berdir’s picture

There is no quick fix. The stream wrapper manager is initalized in DrupalKernel::boot(), the modules are loaded in preHandle() which comes much later.

There are only two options, rolling back the stream wrapper manager (which would be sad) or fixing this.

Fabianx’s picture

So we can't call ModuleHandler()->loadAll() from that stage - before calling alter? - at least temporarily.

OR:

And we can't move the alter hook to preHandle? -- Nope, we can't.

Berdir’s picture

Status: Needs work » Needs review
FileSize
48.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,440 pass(es), 1,489 fail(s), and 286 exception(s). View

Just a reroll for now.

Status: Needs review » Needs work

The last submitted patch, 23: lazy-load-modules-2353357-23.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
52.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,979 pass(es), 302 fail(s), and 110 exception(s). View
3.88 KB

A lot changed since this worked, trying some fixes.

Status: Needs review » Needs work

The last submitted patch, 25: lazy-load-modules-2353357-25.patch, failed testing.

jibran’s picture

+++ b/core/modules/system/tests/modules/menu_test/menu_test.module
@@ -81,6 +81,7 @@ function menu_test_menu_local_tasks_alter(&$data, $route_name) {
+<<<<<<< ours

@@ -121,6 +122,8 @@ function menu_test_theme_page_callback($inherited = FALSE) {
+=======
+>>>>>>> theirs

conflicts.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,309 pass(es), 366 fail(s), and 1,089 exception(s). View

Posted a reroll of the previous patch at #1905334: Only load all modules when a hook gets invoked.

Fresh start here that converts to an event. Not sure about some things, especially naming. I did move system_stream_wrappers_alter() inline, no point in basically altering our own definitions IMHO when we can do it right in there. There's also an issue to convert that to a setting, would avoid loading config.

I suspect this is a regression at first, but it does fix a critical bug and allows for #1905334: Only load all modules when a hook gets invoked.

It's technically not an API change, because right now, it is impossible to implement the alter hook, as mentioned above :)

Status: Needs review » Needs work

The last submitted patch, 28: stream-wrapper-event-2353357-28.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,410 pass(es), 36 fail(s), and 1,607 exception(s). View
3.5 KB

Made the two services optional, fixed kernel tests.

Status: Needs review » Needs work

The last submitted patch, 30: stream-wrapper-event-2353357-30.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs profiling
FileSize
8.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,023 pass(es), 2 fail(s), and 3 exception(s). View
609 bytes

...

Status: Needs review » Needs work

The last submitted patch, 32: stream-wrapper-event-2353357-32.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,061 pass(es). View

Reroll. Can't reproduce those tests fails locally, let's try again.

Berdir’s picture

Issue tags: +Ghent DA sprint
chx’s picture

I don't get it , why are we using setter method calls instead of a factory method since we are injecting the container anyways?

Berdir’s picture

I'm using setter injection because I made it optional so we don't have to check/trigger that event in installer/kernel tests.

dawehner’s picture

Its really looking good.

One thing I was wondering, whether we need all the methods on the event class. Would you really for example use the "hasStreamWrapper"?

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperEvents.php
    @@ -0,0 +1,22 @@
    +  /**
    +   * Name of the event when altering stream wrappers.
    +   *
    +   * @see \Drupal\Core\StreamWrapper\StreamWrapperManager::register()
    +   */
    +  const ALTER = 'stream_wrappers.alter';
    

    Note: we do use now @Event to mark them as available event.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -279,7 +299,14 @@ public function addStreamWrapper($service_id, $class, $scheme) {
    +    // Only expose the private file stream wrapper if a file path has been set.
    +    if (!$this->configFactory || !$this->configFactory->get('system.file')->get('path.private')) {
    

    Can we link to the issue which tries to get rid of the configuration and the switch to the setting?

Berdir’s picture

Issue tags: -Needs profiling
FileSize
8.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
4.03 KB

I'm not sure about the methods, but I felt like that might be to be a common case when altering, if it exists, get and change it. Also changed back to optional constructor arguments, as discussed with @dawehner.

Added @Event, and added a link to that private file system issue.

Profiled this together with @dawehner. It is hard to do fairly, because in HEAD, the alter hook is broken and we don't call the config. Also, injecting the config still results in loading the module handler internally (through config typed somehow), so we don't actually save on that.

Despite that, it was not slower as executing the no-op event is very fast. Which is what I was worried about and added the tag for.

What we must look into is moving this back into preHandle(). It being in boot() means that we call it for page cache responses and that seems very wrong, especially in combination with the config get.

Wim Leers’s picture

Berdir already opened an issue for

What we must look into is moving this back into preHandle(). It being in boot() means that we call it for page cache responses and that seems very wrong, especially in combination with the config get.

This is the one: #2392433: Stream wrappers are registered before page cache.

dawehner’s picture

Issue tags: +Needs change record

Given that I think we need to add a change record.

Went with @berdir through the patch. I think its ready as it is.

I'm not sure about the methods, but I felt like that might be to be a common case when altering, if it exists, get and change it.

Okay this is fair.

Wim Leers’s picture

Status: Needs review » Needs work

Profiling already happened.

Patch is indeed looking great.

This does not yet bring the big improvement to page cache performance that it enables, because the one @todo still needs to happen in a follow-up.

Only marking NW for a nitpick, what seems like a genuine bug and a missing follow-up issue that needs to be linked to.

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -50,20 +51,32 @@ class StreamWrapperManager extends ContainerAware {
    +   *   (optional) The event dispatcher to dispatch the stream wrapper alter
    +   *   event.
    ...
    +   *   (optional) The config factory, used to check if the private stream
    +   *   wrapper should be made available or not.
        */
    

    Would be good to document why these are optional? (AFAICT: for the installer.)

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -279,7 +292,16 @@ public function addStreamWrapper($service_id, $class, $scheme) {
    +    // @todo Convert this to settings to avoid loading this from the
    +    //   configuration system early in the request.
    

    You added this @todo, but an issue is still missing. Would be great if you could add one.

  3. +++ b/core/modules/simpletest/src/KernelTestBase.php
    @@ -314,8 +314,7 @@ public function containerBuild(ContainerBuilder $container) {
    +      ->setArguments([new Reference('service_container'), new Reference('config.factory')]);
    

    The first argument should be the event dispatcher, not the service container?

    Why didn't this trigger any test failures?

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,166 pass(es). View
2.12 KB

Thanks for the review, added the documentation, issue link and fixed the wrong KernelTestBase. This has not been tested yet, and it will fail :)

The last submitted patch, 39: stream-wrapper-event-2353357-39.patch, failed testing.

Wim Leers’s picture

It passed, actually :)

Adding the linked issue as a related issue here.

That CR is still needed. Keeping at NR to get more reviews hopefully. But IMHO this is RTBC as soon as there is a CR.

Berdir’s picture

Weird, looks like we never created a change record for #2028109: Convert hook_stream_wrappers() to tagged services., I'll write one tomorrow.

Berdir’s picture

It passed, actually :)

I mean the previous patch that I cancelled manually :)

Wim Leers’s picture

I mean the previous patch that I cancelled manually :)

Ah, that makes more sense :)

chx’s picture

Issue tags: +sad chx

Actual problem: What if I want to remove a stream wrapper?

Rant to be ignored: this is making a 1 line alter $info['foo']['bar'] to a get, the change and the set. Also, alterEvent is a fantastic textbook example of how haphazard hooks and events are now. Yes, I know there is no other solution at this point but this issue shows a lot of Drupal 8 problems so clearly that it deserves this tag.

Wim Leers’s picture

Actual problem: What if I want to remove a stream wrapper?

Good point. That needs to be supported.

Rant to be ignored: this is making a 1 line alter $info['foo']['bar'] to a get, the change and the set. Also, alterEvent is a fantastic textbook example of how haphazard hooks and events are now. Yes, I know there is no other solution at this point but this issue shows a lot of Drupal 8 problems so clearly that it deserves this tag

I share reservations about some of the verbosity (dare I say "Javaness"?) we have in Drupal 8. But indeed that ship has sailed, and this is solving a critical performance problem. So indeed, unfortunately to be ignored at this point.

Berdir’s picture

Status: Needs review » Postponed

Ok, we decided to change this completely.

Discussed with @alexpott and we can't see a reason to change stream wrappers dynamically. Anything else can be done in a ServiceProvider now, so we are raising #2170235: file_private_path should be in $settings, like file_public_path to critical and will register the private service dynamically there. Then we can just remove the hook here. Postponing on that.

Berdir’s picture

Status: Postponed » Needs review
Issue tags: +Needs tests, +Needs issue summary update
FileSize
3.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,376 pass(es). View

Ok, fresh start, just removing the hook, as explained above.

Need to add tests for the correct behavior of private, as that is broken right now and we should have a test for it.

Berdir’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated issue summary. Working on extending the tests now.

Berdir’s picture

Issue tags: -Needs tests
FileSize
1.43 KB
4.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,449 pass(es). View

Updated ConfigTest. I removed the check for file_temporary_path, as drupalPost() verifies that for us, I just kept the checks for public/private.

Note that there is no bug anymore related to this, as #2170235: file_private_path should be in $settings, like file_public_path already fix this, this just provides test coverage for it.

I think the only thing left here is the change record, I'm about to leave, so if someone wants to work on it, assign the issue.

As mentioned above, we need to explain the whole change around stream wrappers, not just this alter hook:

- That the hook is gone and stream wrappers are tagged services instead, include public as exampl
- That the alter hook was removed and that modules can instead use a ServiceProvider to dynamically register a service or not if it is their own (include code from CoreServiceProvider as example)
- If they want to change another service, the can implement alter() on a ServiceProvider and change the service, e.g. to remove it or to replace the class.
- The current alter hook documentation/example still uses the hook to change the label, that no longer work as label/description is a method on the class now, what must be done instead is replace the class for that.

chx’s picture

Issue tags: -sad chx

Well that, I guess, is better. But definitely needs change record.

chx’s picture

Another rant but this not to be ignored: again and again and again, this issue went to 50 comments and was close to ready when I entered sad puppy mode and then the issue took a 180 degree turn for the better despite I had no good idea what to do. What about doing the latter without me getting frustrated and sad in all of these?

webchick’s picture

My recommendation: Next time, instead of entering 'sad puppy' mode, engage respectfully and collaboratively with the people trying to solve the problem and see if you can put your heads together on a more optimal solution.

chx’s picture

That's the Drupal 7 way. Those days are gone.

Edit: even when I point out on IRC that the approach is flawed and trying to find a solution, people will not engange but rather relentlessly chase the flawed approach ostensibly because Symfony and it drives me absolutely mad (see fileinfo for more). Thanks but I am not interested in these conversations.

znerol’s picture

+++ b/core/modules/system/src/Tests/File/ConfigTest.php
@@ -37,15 +37,28 @@ function testFileConfigurationPage() {
+    drupal_flush_all_caches();

Do not use this function in tests. Either use $this->resetAll() or $this->rebuildContainer(). Those methods will also update the $container property of WebTestBase.

Berdir’s picture

Issue tags: -Needs change record

I don't think this issue is the right place for this discussion.

Created change record: https://www.drupal.org/node/2393323, please review.

Will fix the dfac() call asap.

@chx: Your comment had little to do with how this issue worked out, I first implemented the approach suggested in the issue summary, but when we were looking closer at it for writing tests and the private path change, we noticed there is no use case for dynamically altering the stream wrappers that can not be covered with service providers so we decided to simply remove the hook after I discussed it with @alexpott.

Berdir’s picture

FileSize
6.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,494 pass(es). View
2.96 KB

Using rebuildContainer() in the test, also switched to assert on the labels. Added a note to the documentation that caches must be cleared and removed the handbook link from the UI, that is already in default.settings.php anyway.

znerol’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
znerol’s picture

Issue summary: View changes

  • catch committed 14189a7 on 8.0.x
    Issue #2353357 by Berdir: hook_stream_wrappers_alter() is broken since...
alexpott’s picture

Title: hook_stream_wrappers_alter() is broken since modules are not loaded on demand, also change to an event since it's the last hook that forces this during bootstrap » hook_stream_wrappers_alter() should be removed as it is broken since modules are not loaded on demand

Fixing issue title.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Prefer the diffstat with this approach :)

@chx: this is a fairly unique situation to not want to have to invoke the hook for.

Additionally even if we'd changed to an event (which I agree has much worse developer experience, particularly for small alters), we may have realised the event was pointless later on, and could still have removed it then too (assuming it was before release). This issue would still be an incremental improvement in terms of improving bootstrap performance.

Committed/pushed to 8.0.x, thanks!

znerol’s picture

Commit message still mentions "event".

catch’s picture

It does, I cross-posted with Alex...

Status: Fixed » Closed (fixed)

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