Stream wrappers not registered when installing module's default config

Is it still needed?

I am still having this problem using Drupal 11.x version. What I do to reproduce it is installing a Drupal recipe that adds a editor with a image upload configuration.

I've added a MR that registers the stream wrapper information into \Drupal\Core\StreamWrapper\StreamWrapperManager::$wrappers when the stream wrapper is being added. It fixes my problem.

Please review if this can be a good solution. Let me know if it needs work / a different approach. Thanks!

Thanks for working on this!

Even though a 1 liner the issue is marked major so think maybe we should add some kind of test coverage that shows the issue.

Then if we determine that fix addresses the problem we should update the issue summary.

I stumbled on this issue because I found a scenario where stream wrappers are not registered properly.

When more than 1 module is enabled, stream wrappers are not registered, see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co... which calls $this->updateKernel([]) and doesn't call \Drupal::service('stream_wrapper_manager')->register(); after it.
Notice that just above it we have a call to ::updateKernel and then it registers stream wrappers.
That particular code seems to originate from #3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller.

This can cause all kind of issues.
In my case, I have a config import that has to enable more than 1 module and delete an image style, and noticed the image style folder is not being deleted, which should happen in \Drupal\image\Entity\ImageStyle::postDelete but stream wrappers are not registered.

The same also happens when uninstalling a module: ::updateKernel is called but stream wrappers are not registered. So a config import that has to uninstall a module and delete an image style won't delete the corresponding folder.

The IS only mentions "in some scenarios" for the original case reported, but module configs are installed right before the updateKernel call so I suspect it is the same scenario? when multiple modules are enabled at once? or a module is uninstalled?

---

MR 10421 here seems to solve those cases as well, and seems like the most reliable way.
Perhaps we should delete StreamWrapperManager::register entirely?

Static patch of MR 10421

Added a test https://git.drupalcode.org/issue/drupal-3416735/-/compare/main...3416735...

It fails when

  public function addStreamWrapper($service_id, $class, $scheme) {
    $this->info[$scheme] = [
      'class' => $class,
      'type' => $class::getType(),
      'service_id' => $service_id,
    ];
  }

It passes when

  public function addStreamWrapper($service_id, $class, $scheme) {
    $this->info[$scheme] = [
      'class' => $class,
      'type' => $class::getType(),
      'service_id' => $service_id,
    ];

    $this->registerWrapper($scheme, $class, $class::getType());
  }

Changed to needs review.

Added a bunch of comments so that phpstan turned green.

I just ran vendor/bin/phpstan analyse -c core/phpstan.neon.dist core/tests/Drupal/Tests/Core/StreamWrapper/StreamWrapperManagerTest.php and vendor/bin/phpstan analyse \ -c core/phpstan.neon.dist \ core/tests/Drupal/Tests/Core/StreamWrapper/StreamWrapperManagerTest.php I was surprised and confused as to why such detailed commentary was necessary to pass these local tests!

I'm super confused by this as well, I see that you added some of the things I looked at in my review that made raise an eyebrow were added in a phpstan commit. It has so many things that should not be needed to satisfy our coding standards and phpstan rules.

I suspect maybe you aren’t picking up core standards? It’s a little slower but you can pull all that stuff out and rely on the pipeline.

The bracket being in line is really weird because that’s been a standard forever

You may need to point phpcs to the phpcs.xml file in the core directory so it runs with the correct standard, it looks like it may be defaulting to another standard?

Left some comments on the MR, another problem I'm seeing is the MR is almost 2000 commits back :) should be rebased.

Pipeline needs to be passing to move forward.

I've just pushed a fix for this that I found while working on #3498026: RecipeRunner::processInstall() should allow for installing multiple modules at once ... also #3579611: StreamWrapperManager::getNames() and ::getDescription() broken will fix some of the other recent issues with stream wrappers.

Left one comment on the code because I don't really understand it, but test coverage looks to be good.

Well the problem we have is that when the container is reset the stream wrapper manager service loses the information about what wrappers have been registered because a new object is created. So we need to rebuild \Drupal\Core\StreamWrapper\StreamWrapperManager::$wrappers.

This leads to two thoughts... either we should unregister the wrappers during the container reset - or we should make \Drupal\Core\StreamWrapper\StreamWrapperManager::$wrappers a static. I think the first option is better as would allow a reset to remove a stream wrapper.

#3575642: Always free up old container on rebuild, not only in the installer also adds support for an upstream feature so that Services can react to a reset, sounds like that might be useful for that?

Actually I was wrong - new services are not going to be added to the container during a container reset... so I've removed the unregister and added a comment.

From reading the code and the comments here, as well as the thread on slack, this seems to make a lot of sense.

I looked at the rest of the lines in $ignoredPropertyPaths, and the rest is search module and contact module, I think that means we can create a followup for this to remove all of them (and that functionality) after both of those are removed as well?

Committed/pushed to main and 11.x, thanks!