hook_stream_wrappers_alter() should be removed as it is broken since modules are not loaded on demand

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?

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.

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.

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.

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.

More fixes.

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.

More of the same.

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.

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.

Just a reroll for now.

A lot changed since this worked, trying some fixes.

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

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

Made the two services optional, fixed kernel tests.

...

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

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

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

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?

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.

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.

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.

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?

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

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.

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

It passed, actually :)

I mean the previous patch that I cancelled manually :)

I mean the previous patch that I cancelled manually :)

Ah, that makes more sense :)

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.

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.

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.

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.

Updated issue summary. Working on extending the tests now.

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.

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

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?

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.

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.

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

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.

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.

Fixing issue title.

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!

Commit message still mentions "event".

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