Allow creating image derivatives from an Image object

Great idea and looks good.

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -271,6 +272,14 @@ public function flush($path = NULL) {
+    $image = \Drupal::service('image.factory')->get($original_uri);

Not possible to inject?

Your idea is good, The current createDerivative() does too many things in 1 method, making it indeed hard to reuse functionality in unforeseen ways. However, I think that the new method should be named apply() (conforming with the image - toolkit - operation naming) or applyStyle() (conforming with applyEffect() on ImageEffect). I prefer the first, as IMO the latter should also be renamed to apply() (for reasons of consistency and DRY: do not repeat the name of the class on method names). This would be perfectly consistent as all apply() methods accept an ImageInterface as 1st argument (except on ImageInterface itself of course).

Regarding the comment in #5 and the answer in #6: I prefer to do it correct the 1st time, the patch will remain small and reviewable and as we are already in beta, structural/creational choices should be done correct by now.

Can we create a follow-up for ImageEfect?

This is a nice change, I agree.

1. +++ b/core/modules/image/image.admin.inc
   @@ -32,11 +32,11 @@ function template_preprocess_image_style_preview(&$variables) {
   +  $image = $image_factory->get($original_path);
   
   @@ -51,13 +51,15 @@ function template_preprocess_image_style_preview(&$variables) {
   +  else {
   +    $image = $image_factory->get($preview_file);
      }
   -  $preview_image = $image_factory->get($preview_file);
   

   Oh, very nice! This also deserves a 'performance' tag?

2. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -163,16 +163,23 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   +    if (file_exists($derivative_uri)) {
   ...
   +    else {
   +      // Generate the derivative image.
   +      $image = $this->imageFactory->get($image_uri);
   +      $success = $image_style->applyAndSave($image, $derivative_uri);
   +    }
   ...
        if ($success) {
   -      $image = $this->imageFactory->get($derivative_uri);
   

   So, in this case $image is the original image, right? Then inside if ($success) {...} we'll send headers with info from the original image pretending that is the derivative?

3. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -94,6 +100,68 @@ class ImageStyle extends ConfigEntityBase implements ImageStyleInterface, Entity
   +  public function __construct(array $values, $entity_type, FileSystemInterface $file_system = NULL, ImageFactory $image_factory = NULL, PluginManagerInterface $image_effect_plugin_manager = NULL, StreamWrapperManagerInterface $stream_wrapper_manager = NULL, PrivateKey $private_key = NULL) {
   ...
   +    $this->fileSystem = $file_system ?: \Drupal::service('file_system');
   +    $this->imageFactory = $image_factory ?: \Drupal::service('image.factory');
   +    $this->effectManager = $image_effect_plugin_manager ?: \Drupal::service('plugin.manager.image.effect');
   +    $this->streamWrapperManager = $stream_wrapper_manager ?: \Drupal::service('stream_wrapper_manager');
   +    $this->privateKey = $private_key ?: \Drupal::service('private_key');
   

   Sorry, it's my fault from #5. We cannot do that because we are initialising all the services in constructor and maybe we don't need them every time. Let's use the static \Drupal::whatever() or, better, we can create some getters. See how \Drupal\editor\Editor::editorPluginManager() is used to lazy load the plugin.manager.editor service.

4. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -177,7 +245,7 @@ protected static function replaceImageStyle(ImageStyleInterface $style) {
   -    $scheme = $this->fileUriScheme($uri);
   +    $scheme = $this->fileSystem->uriScheme($uri);
   
   @@ -205,7 +273,7 @@ public function buildUrl($path, $clean_urls = NULL) {
   -      $original_uri = file_uri_scheme($path) ? file_stream_wrapper_uri_normalize($path) : file_build_uri($path);
   +      $original_uri = $this->fileSystem->uriScheme($path) ? file_stream_wrapper_uri_normalize($path) : file_build_uri($path);
   
   @@ -225,8 +293,8 @@ public function buildUrl($path, $clean_urls = NULL) {
   -    if ($clean_urls === FALSE && file_uri_scheme($uri) == 'public' && !file_exists($uri)) {
   -      $directory_path = \Drupal::service('stream_wrapper_manager')->getViaUri($uri)->getDirectoryPath();
   +    if ($clean_urls === FALSE && $this->fileSystem->uriScheme($uri) == 'public' && !file_exists($uri)) {
   +      $directory_path = $this->streamWrapperManager->getViaUri($uri)->getDirectoryPath();
   
   @@ -253,7 +321,7 @@ public function flush($path = NULL) {
   -    $wrappers = \Drupal::service('stream_wrapper_manager')->getWrappers(StreamWrapperInterface::WRITE_VISIBLE);
   +    $wrappers = $this->streamWrapperManager->getWrappers(StreamWrapperInterface::WRITE_VISIBLE);
   
   @@ -330,7 +424,7 @@ public function getDerivativeExtension($extension) {
   -    return substr(Crypt::hmacBase64($this->id() . ':' . $this->addExtension($uri), $this->getPrivateKey() . $this->getHashSalt()), 0, 8);
   +    return substr(Crypt::hmacBase64($this->id() . ':' . $this->addExtension($uri), $this->privateKey->get() . $this->getHashSalt()), 0, 8);
   
   @@ -354,7 +448,7 @@ public function getEffect($effect) {
   -      $this->effectsCollection = new ImageEffectPluginCollection($this->getImageEffectPluginManager(), $this->effects);
   +      $this->effectsCollection = new ImageEffectPluginCollection($this->effectManager, $this->effects);
   
   @@ -399,26 +493,6 @@ public function setName($name) {
   -  protected function getImageEffectPluginManager() {
   -    return \Drupal::service('plugin.manager.image.effect');
   -  }
   ...
   -  protected function getPrivateKey() {
   -    return \Drupal::service('private_key')->get();
   -  }
   
   @@ -454,26 +528,6 @@ protected function addExtension($path) {
   -  protected function fileUriScheme($uri) {
   -    return file_uri_scheme($uri);
   -  }
   

   No, please! Are unrelated to this issue. Let's open another issue to handle those case.

5. +++ b/core/modules/image/src/ImageStyleInterface.php
   @@ -113,10 +114,51 @@ public function flush($path = NULL);
   +   * @deprecated as of Drupal 8.1.0, will be removed in Drupal 9.0.0.
   

   I think, "... will be removed before Drupal 9.0.0." is more appropriate.

Overall:

1. Revert changes not related to this issue. Open a followup for the other issue.
2. We need a test to prove that we can create a derivative from a resource without having the original on disk.
3. I grepped for 'createDerivative' and there are not too many occurrences in core, so let's convert them in this patch.
4. We need a change notice. Draft for now.
5. Followup to remove createDerivative() in 9.x?

1. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -309,6 +320,23 @@ public function createDerivative($original_uri, $derivative_uri) {
   +    // Apply the effects to the image object.
   +    foreach ($this->getEffects() as $effect) {
   +      $effect->applyEffect($image);
   +    }
   ...
   +    return TRUE;
   

   Only now I see this crap. So ImageEffectInterface::applyEffect() does return a success flag but we are not using it here. We just return TRUE regardless if the effects were applied correct. Is there a good reason for this? I don't remember. If there's a reason, we need to document it, otherwise we need something like:

   if (!$effect->applyEffect($image)) {
     return FALSE;
   }
   

2. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -508,4 +536,23 @@ protected function fileDefaultScheme() {
   +  protected function getImageFactory() {
   +    return \Drupal::service('image.factory');
   +  }
   ...
   +  protected function getFileSystem() {
   +    return \Drupal::service('file_system');
   +  }
   

   So, we're not caching like in \Drupal\editor\Editor::editorPluginManager() in a protected variable? In the case the service is requested more than once per request, is there a cost to call again \Drupal::service()? I'm not sure, I think no and maybe there's no need to the protected cache var. Opinions?

3. +++ b/core/modules/image/src/Tests/FileMoveTest.php
   @@ -36,7 +36,7 @@ function testNormal() {
   +    $style->applyAndSave(\Drupal::service('image.factory')->get($original_uri), $derivative_uri);
   
   @@ -52,4 +52,43 @@ function testNormal() {
   +    $image = \Drupal::service('image.factory')->get();
   ...
   +    $image = \Drupal::service('image.factory')->get($derivative_uri);
   
   +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -394,7 +394,7 @@ public function testFlushUserInterface() {
   +    $this->assertTrue($style->applyAndSave(\Drupal::service('image.factory')->get($image_uri), $derivative_uri));
   
   +++ b/core/modules/image/src/Tests/ImageStyleFlushTest.php
   @@ -31,7 +31,7 @@ function createSampleImage($style, $wrapper) {
   +    $derivative = $style->applyAndSave(\Drupal::service('image.factory')->get($source_uri), $derivative_uri);
   
   +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
   @@ -276,7 +276,7 @@ protected function doTestResponsiveImageFieldFormatters($scheme, $empty_styles =
   +    $large_style->applyAndSave(\Drupal::service('image.factory')->get($image_uri), $large_style->buildUri($image_uri));
   
   @@ -501,7 +501,7 @@ private function assertResponsiveImageFieldFormattersLink($link_type) {
   +    $large_style->applyAndSave(\Drupal::service('image.factory')->get($image_uri), $large_style->buildUri($image_uri));
   

   In tests the container is available:

   $this->container->get('image.factory')->...
   </li>
   

4. +++ b/core/modules/image/src/ImageStyleInterface.php
   @@ -113,10 +114,51 @@ public function flush($path = NULL);
   +   * Applies the style to an Image object.
   ...
   +   * Alter the source image applying all image effects.
   ...
   +  public function apply(ImageInterface $image);
   

   $image opens with the original image, but after applyAndSave it has all the properties aligned to the derivative that was saved.

   I'm not sure now that 'apply' verb is the best choice. We generally have the $image (the subject) and we 'apply' (the verb) something on it. See $image->apply('crop') —­ this makes perfectly sense. Here, the logic is reverted. "something" (image style) is the subject. Im not sure about the naming... Can we reflect more on this?

5. +++ b/core/modules/image/src/Tests/FileMoveTest.php
   @@ -52,4 +52,43 @@ function testNormal() {
   +    $this->assertEqual($image->getFileSize(), NULL);
   ...
   +    $this->assertEqual($image->getFileSize(), NULL);
   

   We have $this->assertNull().

6. +++ b/core/modules/image/src/Tests/FileMoveTest.php
   @@ -52,4 +52,43 @@ function testNormal() {
   +    $this->assertTrue(file_exists($derivative_uri), 'Derivative image is generated successfully.');
   

   This is just a recommendation: messages in assertions are obsolete.

The point is if we want to stop the derivative generation as soon as any effect fails, or (like now), just run all effects regardless of their failure, with each effect logging its own failure. A change here would be out of scope of this issue IMO. Let's have a separate issue for that, please.

I agree that this is off-topic. Also, I guess we still want to "show something" instead of nothing but also in this case we may want to return $image->isValid();.

OK, let's discuss this elsewhere.

->applyToImage() ?

I think, better: ->applyTo() and ->applyToAndSave(). What you think?

IMO, apply() is perfectly OK.

Most standard OO guidelines will tell us:
- method names are concise and clear.
- the combination of method name with object type and the name and type of arguments should be complete. Thus createDerivative() seems OK as create() alone is not enough.
- methods are preferably named using a single verb that tells what the object is doing (to or using the arguments of the method). Thus apply() is OK: the image style applies its effect to the (image) argument.
- method names do not repeat the type of the argument, thus not applyToImage() (or the shorter incomplete applyTo() that leaves out the type of the argument but still tries to connect the verb to the argument).
- method names that have 2 verbs indicate that the method does too much. Thus applyToAndSave() indicates that it would be better to split the method in a separate apply() and save() (I didn't look at the code if that is the case here).

Moreover, in this case we once decided that for consistency we would name all methods in the apply chain apply(). So better stick with that.

#24 is correct. I see two options:

1. Create a new interface ImageStyleActionsInterface and make ImageStyle implements also ImageStyleActionsInterface. Add next statements into docblock of this interface: "@deprecated in Drupal 8.1.x, will be merged into \Drupal\image\ImageStyleInterface in Drupal 9.0.x" and "@todo Remove this interface and merge its methods into \Drupal\image\ImageStyleInterface in 9.0.x".
2. Remove the methods from the interface, move their docblocks to ImageStyle and add "@todo Add this method to \Drupal\image\ImageStyleInterface in Drupal 9.0.x."

Which one? Maybe we should ask a core committer?

#24: technically correct, but I think this will result in nobody being able to:
- add new features unless we are talking completely self contained modules.
- refactor code, other then at the level of renaming internal variables and such.
- improve naming and staying BC by keeping the old method (explicit example in the docs about staying BC)
- extract services, out of existing code and referring to it.

Furthermore we are talking about:
- (IMO) a (to be) @internal API (https://www.drupal.org/node/2116841, https://www.drupal.org/node/2550249). I don't think that ImageStyleInterface is implemented in contrib by bypassing the ImageStyle core class (I even think it is not implemented at all in contrib)
- Not a disruptive change.

But let's indeed ask a core committer about the current consensus on this.

If the Interface has to remain locked for the whole D8 life cycle, I would go for the 1st option from @25 but not using @deprecated as there is no alternative so it doesn't make sense to have our IDEs warn us about something we can't change anyway.

[EDIT, crossposted]
re #26: If you can extract all this into a separate class, I would call that a new feature being allowed in a minor release.