Problem/Motivation

In #3414627: Convert StreamWrapperManager to use a service locator we modified StreamWrapperManager.

In install_begin_request() we define a minimal container to bootstrap the install system, including:

  $container
    ->register('stream_wrapper_manager', 'Drupal\Core\StreamWrapper\StreamWrapperManager')
    ->addMethodCall('setContainer', [new Reference('service_container')]);

However, the setContainer method has been removed and the service locator (container) should be passed as an argument.

Question: why doesn't this fail?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3416697

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

longwave opened merge request !6307

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review

Testing what happens if we just remove file_system and stream_wrapper_manager entirely here.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community

Seems dead-as-dodo-code to me => RTBC (without the need for a followup...😈😇)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Fix install container definition of StreamWrapperManager » Remove install container definitions of FileSystem and StreamWrapperManager

larowlan credited sun.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Did some git ancestry

The stream wrapper manager was added in a bad reroll by me in #2028109: Convert hook_stream_wrappers() to tagged services. which was pointed out by @sun in comment 152. Crediting sun here for that all these years later.

The file system entry was added in #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class around the same time (late 2014/early 2015)

Manually ran an install and verified this hunk is where we boot an actual container now, which contains the file system service.

// Note, InstallerKernel::createFromRequest() is not used because Settings is
  // already initialized.
  $kernel = new InstallerKernel($environment, $class_loader, FALSE);
  $kernel::bootEnvironment();
  $kernel->setSitePath($site_path);
  $kernel->boot();

$kernel->boot(); loads core.services.yml which loads the actual file system service.

So the subsequent use of the file system lower down is fine

/** @var \Drupal\Core\File\FileSystemInterface $file_system */
  $file_system = $container->get('file_system');
  $container->set('string_translator.file_translation', new FileTranslation($directory, $file_system));
  $container->get('string_translation')
    ->addTranslator($container->get('string_translator.file_translation'));

crediting myself for this archeology work

  • larowlan committed 5d779b9e on 11.x 
    Issue #3416697 by longwave, Spokje, sun, larowlan: Remove install...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x, thanks!

larowlan closed merge request !6307

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3416735: Stream wrappers not registered when installing module's default config

Hm … this would be a delightfully simple explanation for #3416735: Stream wrappers not registered when installing module's default config and potentially fixes that altogether 🧐

Status: Fixed » Closed (fixed)

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