Allow to pass save options to ImageInterface::save

So we agree to attach to Image kind of internal storage? Well I thought exactly the same and hope that this will work also as a general storage for internal processing (like resource for GD and so on...).

Before reviewing this I'm asking myself if this couldn't be merged with #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately — mostly after reading this from summary: "This allows to fully utilize the abilities of the underlying toolkit, we are not restricting what parameters can be passed".

Switched back to 'normal' till having an agreement on the solution.

Please add an interdiff to each patch making easier to review the patch. Also, because I don't have a 100% understanding of the scope, can't you point us to a use case (even from D7)? That imagecache actions effect that would eventually need this.

Code looks good. It's concise and looks like it applies to the right places.
On visual review, I can't see any problems this would be introducing, and architecturally, moving towards a set of arbitrary keyed default options that can be over-ridden is solid.

Some of the use cases - that ARE currently supported in D7 and are in use in the real world (hence regression) can be seen in the features list of imagecache_actions
* have different recompression parameters for different derivatives
* Change file type is the big one
* - as putting transparent rounded corners or masks on a JPG requires saving as PNG
* - as PNGs can be really large filesizes when generating a thumbnail
* - once upon a time I had SVG support (really! Only unsupported because it required too much server-side install instructions) and that can come back
* We allow PHP-code image convolution, so users can add their own algorithms for arbitrary effects. . If there is some need for parameters to be passed during the process, attaching them to the object is better than using globals..
* there is now provision for imagemagick to insert unpredicted wierdness during pre-processing, and then pull it out again at save time.

eg, if you need to set an environment variable that points to a system library or font folder or ??? then do things with that at the other end when about to save. This is convoluted, but I HAVE had to do that before, and leaving a storage array open for things we don't know is handy for future overloading. Previously, as described in the summary, I just threw anything into the $info array,

I've not thought through whether this patch or a similar one helps, but I have had requests to use different storage methods depending on the derivative. (eg full-size is 'private' or 'cdn') - this may not be directly the right place for that, and may even already be solved elsewhere (?) but it's a powerful use-case to consider.

I would like to actually RUN the code with this patch and see that an example of what it's doing does what I hope. I also would like to supply an eg of it in action - but that will require upgrading a harness module all the way up, so I dunno.
In the meantime
- this is a request for hook-ability in a reasonable place to give outside modules the control they need, and used to have in D7
Upgrading the modules that will use the optional feature will have to wait until it's practical to use it.

I'm going to thumbs-up this to move it along.

Yeah, lets not talk about that here. I was just blue-skying.

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\KeyValueStore\MemoryStorage;
   
   @@ -100,6 +108,7 @@ class Image implements ImageInterface {
   +    $this->saveOptions = new MemoryStorage('image save options');
   

   A couple things about this:
   1) I think you should be using KeyValueMemoryFactory, not MemoryStorage directly.
   2) One step further, why not just use the 'keyvalue' service directly, and not worry about which implementation it is?
   3) 'image save options', without underscores, seems very strange.

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -223,6 +223,16 @@ public function load(ImageInterface $image) {
   +        'quality' => \Drupal::config('system.image.gd')->get('jpeg_quality'),
   

   Plugins can use dependency injection, see ContainerFactoryPluginInterface

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\KeyValueStore\MemoryStorage;
   

   Not needed now

2. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -100,6 +108,7 @@ class Image implements ImageInterface {
   +    $this->saveOptions = \Drupal::service('keyvalue')->get('image_save_options');
   

   This should be in ImageFactory, which is a service.

3. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
   @@ -12,6 +12,27 @@
   +    $configFactory = $this->configuration['config_factory'];
   
   +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitManager.php
   @@ -72,6 +72,14 @@ public function getDefaultToolkit() {
   +    return parent::createInstance($plugin_id, $configuration + array('config_factory' => $this->configFactory));
   

   That's not exactly what I meant. You should use ContainerFactoryPluginInterface to do the injection. If you'd like me to jump in and fix it up, I can, I just didn't want to step on any toes.

I got about halfway done fixing the injection, when I thought "why oh why is the Image class now explicitly dependent on KeyValue?"
My last few comments were just on the implementation, but didn't question the architectural decisions.

Reading through the issue, it doesn't seem like there is any strong reason to couple this to KV. We use getter and setters in dozens of other places, and it keeps the architecture cleaner IMO.

I added an @todo in testGetSaveOptions, since it was previously just testing the KV system, and now is testing how arrays work...

Whoops, forgot the other half: config factory. This is what I meant earlier.

The only thing that worries me is that toolkits are not forced to use getSaveOptions(), so even if someone set an option, a toolkit might skip it completely.

Few comments

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -90,6 +90,13 @@ class Image implements ImageInterface {
   +   ....
   +  protected $saveOptions = array();
   +  .....
   +  public function getSaveOptions() {
   +  ....
   +  public function getSaveOption($key) {
   +  ....
   +  public function setSaveOptions(array $save_options) {
   +  ...
   +  public function setSaveOption($key, $save_option) {
   +  ...
   

   Why not making one abstraction step further and just name these $options, getOption/s(), setOption/s()? It has been discussed elsewhere that it would help to have in Image a place where contrib can store additional information about the image itself (example: define a background color to use across a sequence of image effects, so set it on in the first effect and carry it over - I have that use case in Textimage). More semantics though, the setup here would allow that anyway, just in that case the naming would be misleading.

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -31,7 +31,7 @@ public function settingsForm() {
   +      '#default_value' => $this->configFactory->get('system.image.gd')->get('jpeg_quality'),
   

   We could add a $config property to the toolkit implementation and set it up in its constructor to $this->config = $this->configFactory->get('system.image.gd'), then in the methods just call $this->config->get('jpeg_quality') to avoid repeating the ->get('system.image.gd') piece in all calls.

3. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -236,12 +246,12 @@ public function save(ImageInterface $image, $destination) {
   -    $function = 'image' . image_type_to_extension($image->getType(), FALSE);
   +    $function = 'image' . image_type_to_extension($options['format'], FALSE);
        if (!function_exists($function)) {
          return FALSE;
        }
        if ($image->getType() == IMAGETYPE_JPEG) {
   -      $success = $function($image->getResource(), $destination, \Drupal::config('system.image.gd')->get('jpeg_quality'));
   +      $success = $function($image->getResource(), $destination, $options['quality']);
        }
   

   In case we are changing the image format, shouldn't we be aligning for consistency also the relevant properties on the Image class ($source, $extension, $type, $mimeType)?

   E.g. we load 'public://image.jpg'. This will lead to

   $source = 'public://image.jpg'
   $extension = 'jpg'
   $type = IMAGETYPE_JPEG
   $mimeType = 'image/jpeg'

   then we set option 'format' = IMAGETYPE_PNG; I should expect that the image properties after save result in

   $source = 'public://image.png'
   $extension = 'png'
   $type = IMAGETYPE_PNG
   $mimeType = 'image/png'

   Isn't it?

4. In general, wouldn't it make sense to have a convertFormat() method in the toolkit to cover this, instead of binding it to the save operation? That would cover concern in #25, plus allow some advanced stuff like e.g. load a jpeg, convert it to png to do some in-memory manipulation that involve transparency, then convert back to jpeg before saving to minimize file size.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -222,9 +222,9 @@ public function load(ImageInterface $image) {
-  public function save(ImageInterface $image, $destination) {
-    // Get the save options and complete them with some defaults.
-    $options = $image->getSaveOptions() + array(
+  public function save(ImageInterface $image, $destination, array $options) {
+    // Complete the options with defaults from the image and the config.
+    $options += array(

Oh this is sooo much better, and resolves my concern in #25.

Is there a reason why we don't pass the compression parameter to the imagepng function?

i.e.

    else if ($this->getType() == IMAGETYPE_PNG) {
      // Always save PNG images with full transparency.
      imagealphablending($image->getResource(), FALSE);
      imagesavealpha($this->getResource(), TRUE);
      $success = $function($image->getResource(), $destination, $options['compression']);
    } 
    else {
      $success = $function($image->getResource(), $destination);
    }

Not having a visual interface to edit a png compression (/filters) is not an acceptable answer.
What if the compression variable won't enter core (from some reason or the other)?

The GD library should allow a 3rd party developer to use the png compression regardless.

I'm happy you say that it is foreseen.
Is there an active issue that deals with it?

Patch no longer applies.

A long way has gone since patch #27. #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect introduced image format conversion, and #2096703: Image toolkits should use PluginFormInterface and ContainerFactoryPluginInterface service injection in the toolkit.

IMHO, the only open point here is about allowing contrib to alter the JPEG image save quality at effect/operation level. Currently it is always fetched from the config.

My proposal would be to simply add to GDToolkit a protected property with getter/setter that contrib can use to change the JPEG quality. I do not see need to introduce the getter/setter on ImageToolkitInterface as contrib toolkits may address this differently.