Derivatives should not be created on read-only stream wrappers

Good catch, @claudiu.cristea

+++ b/core/modules/file/tests/file_test/src/StreamWrapper/DummyRemoteStreamWrapper.php
@@ -16,6 +17,13 @@ class DummyRemoteStreamWrapper extends PublicStream {
+  public static function getType() {
+    return StreamWrapperInterface::READ_VISIBLE;
+  }

can we keep this test wrapper writeable, please? I'm using this wrapper in contrib tests to write too and this would break them. How about a separate DummyRemoteReadOnlyStreamWrapper.

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -495,4 +507,15 @@ protected function fileDefaultScheme() {
+    // @todo Properly inject this service in Drupal 9.0.x.

I think the todo needs to go above in the method's docblock.

Other than that looks good.

+++ b/core/modules/file/tests/file_test/src/StreamWrapper/DummyRemoteReadOnlyStreamWrapper.php
@@ -0,0 +1,19 @@
+<?php
+
+namespace Drupal\file_test\StreamWrapper;
+
+use Drupal\Core\StreamWrapper\StreamWrapperInterface;
+
+/**
+ * Dummy read-only remote stream wrapper (dummy-remote-readonly://).
+ */
+class DummyRemoteReadOnlyStreamWrapper extends DummyRemoteStreamWrapper {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getType() {
+    return StreamWrapperInterface::READ_VISIBLE;
+  }
+
+}

I know it's a test only wrapper, but still shouldn't we implement also ::getName and ::getDescription?

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -162,15 +162,27 @@ protected static function replaceImageStyle(ImageStyleInterface $style) {
+      // Optimisation: Avoid computing for source with default scheme.

I would suggest: "The scheme of derivative image files only needs to be computed for source files not stored in the default scheme."

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -162,15 +162,27 @@ protected static function replaceImageStyle(ImageStyleInterface $style) {
+        // Compute the derivative uri scheme. Derivatives created from writable

I think we want "URI" uppercase in docs.

+++ b/core/modules/image/tests/src/Kernel/ImageStyleCustomStreamWrappersTest.php
@@ -0,0 +1,117 @@
+  /**
+   * Tests derivative creation with several source on a local writable stream.
+   *
+   * @dataProvider providerTestCustomStreamWrappers
+   *
+   * @param string $source_scheme
+   *   The source stream wrapper.
+   * @param string $target
+   *   The target part to be tested.
+   * @param string $class
+   *   The stream wrapper service class.
+   */
+  public function testCustomStreamWrappers($source_scheme, $target, $class) {
+    $is_writable = $class::getType() & StreamWrapperInterface::WRITE;
+
+    // Compute the derivative uri scheme. Derivatives created from writable
+    // source stream wrappers will inherit the scheme from source. Derivatives
+    // created from read-only stream wrappers will have the default scheme.
+    $expected_scheme = $is_writable ? $source_scheme : $this->config('system.file')->get('default_scheme');
+
+    $derivative_uri = $this->imageStyle->buildUri("$source_scheme://$target");
+    $derivative_scheme = $this->fileSystem->uriScheme($derivative_uri);
+
+    // Check that the derivative scheme is the expected scheme.
+    $this->assertSame($expected_scheme, $derivative_scheme);
+
+    // Check that the derivative uri is the expected one.
+    $expected_uri = "$expected_scheme://styles/{$this->imageStyle->id()}/$source_scheme/$target";
+    $this->assertSame($expected_uri, $derivative_uri);
+  }
+
+  /**
+   * Provide test cases for testCustomStreamWrappers().
+   *
+   * @return array[]
+   *   An array having each element an array with threes items:
+   *   - The source stream wrapper.
+   *   - The target part to be tested.
+   *   - The stream wrapper service class.
+   */
+  public function providerTestCustomStreamWrappers() {
+    return [
+      ['public', 'image.png', PublicStream::class],
+      ['private', 'system/files/image.png', PrivateStream::class],
+      ['dummy', 'image.png', DummyStreamWrapper::class],
+      ['dummy-readonly', 'image.png', DummyReadOnlyStreamWrapper::class],
+      ['dummy-remote-readonly', 'image.png', DummyRemoteReadOnlyStreamWrapper::class],
+    ];
+  }

wouldn't it be better to have in the dataProvider

1. the source stream wrapper
2. the class of the source stream wrapper
3. and the expected target scheme instead?

the file name is not really involved in the calculation. This way we can avoid recalculating the target scheme in test code and just check what prod code returns.

Side note, going back to #7 for a moment, I noticed that DummyRemoteStreamWrapper extends PublicStream. This means that DummyRemoteStreamWrapper inherits StreamWrapperInterface::LOCAL_NORMAL type which is probably wrong as it has the local flag set on. I think we should have NORMAL or WRITE_VISIBLE there. But that's for a separate issue?

Looks good to me, thanks

Based on the title and the patch, I think this fix is probably best targeted against a minor version.

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -495,4 +508,16 @@ protected function fileDefaultScheme() {
+  /**
+   * Gets the stream wrapper manager service.
+   *
+   * @return \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface
+   *   The stream wrapper manager service
+   *
+   * @todo Properly inject this service in Drupal 9.0.x.
+   */
+  protected function getStreamWrapperManager() {
+    return \Drupal::service('stream_wrapper_manager');
+  }

This makes this change only eligible for 8.2.x. I think this could be backported without this method and just using \Drupal::service('stream_wrapper_manager'); if we feel it is worth it. It probably is not.

Committed f18da3e and pushed to 8.2.x. Thanks!

The new backport policy says that a new Drupal 7 issue should be created. One it exists please mark this one as fixed.

diff --git a/core/modules/image/tests/src/Kernel/ImageStyleCustomStreamWrappersTest.php b/core/modules/image/tests/src/Kernel/ImageStyleCustomStreamWrappersTest.php
index bd875d9..6a45a98 100644
--- a/core/modules/image/tests/src/Kernel/ImageStyleCustomStreamWrappersTest.php
+++ b/core/modules/image/tests/src/Kernel/ImageStyleCustomStreamWrappersTest.php
@@ -5,7 +5,6 @@
 use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\Core\StreamWrapper\PrivateStream;
 use Drupal\Core\StreamWrapper\PublicStream;
-use Drupal\Core\StreamWrapper\StreamWrapperInterface;
 use Drupal\file_test\StreamWrapper\DummyReadOnlyStreamWrapper;
 use Drupal\file_test\StreamWrapper\DummyRemoteReadOnlyStreamWrapper;
 use Drupal\file_test\StreamWrapper\DummyStreamWrapper;

Fixed on commit. Unused use.

Opened #2791283: Derivatives should not be created on read-only stream wrappers [D7 backport], closing this one.