The 'create_new' and 'rotate' image operations do not release memory

Good catch. The problem with CreateNew is that it is not just used to erase the current image and create a totally new one - it is also needed by other operations to create a temporary resource for copying between the original resource and the temp one, drop the old one, and finally set the temp one as current.
But, true, in case of 'totally new image' that leaves an orphaned resource around ATM.

This patch introduces a new toolkit 'create_temp' operation (in fact it's the same code as the current 'create_new'), only specific for the GD toolkit, that is not exposed to the ImageInterface, but only meant to be used by the other GD operations (including 'create_new'). The operations then need to manage what to do with the old resource.

Needs some tests.

With tests. Test only patch == interdiff.

Small cleanup.

Fixed the rotate issue and added additional tests to check that any operation that replaces the original resource is actually destroying the original.

@MattA re #5

... can't we just add an 'is_temp' argument to the 'create_new' operation...

Yeah that was my original thinking too. Then I changed my mind because:

1. The 'create_new' operation is also wrapped by the ImageInterface::createNew method. It would mean an API change if we were to include a $is_temp argument to the interface. Besides, this is GD specific - in GD we need temporary resources to copy images. But this would be alien to a toolkit like e.g. Imagemagick. Therefore I am reluctant to add a GD specific parameter to a toolkit-agnostic interface.
2. Implementers of other toolkits will likely just copy/paste the GD 'create_new' operation code to start their toolkits - I though to separate clearly the operation of getting a temp resource (GD specific) from actually creating a brand new image (non GD specific).

Fix patch in #7 is green.

So your proposal is to add the 'is_temp' argument to the plugin, without adding a corresponding argument to the createNew() method of ImageInterface?

Could work. Is there any other opinion out there?

OK, implemented #14.

1. #19 is not addressed and I find that correct.

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Rotate.php
   @@ -98,17 +98,25 @@ protected function execute(array $arguments) {
   +      // GIFs need to reassign the transparent color after performing the rotate,
   +      // but only do so, if the image already had transparency of its own, or the
   +      // rotate added a transparent background.
   

   Should wrap at 80 chars.

3. +++ b/core/modules/system/src/Tests/Image/ToolkitGdTest.php
   @@ -278,9 +278,19 @@ function testManipulations() {
   +          $this->assertFalse(is_resource($old_res), SafeMarkup::format("'%operation' destroyed the original resource.", ['%operation' => $values['function']]));
   
   @@ -389,6 +399,15 @@ function testManipulations() {
   +    $this->assertTrue(is_resource($old_res), 'Successfully loaded an existing image.');
   ...
   +    $this->assertFalse(is_resource($old_res), 'CreateNew destroyed the original resource.');
   +    $this->assertTrue(is_resource($new_res), 'CreateNew has created a new resource.');
   

   You can avoid the assertion messages.

4. +++ b/core/modules/system/src/Tests/Image/ToolkitGdTest.php
   @@ -389,6 +399,15 @@ function testManipulations() {
   +    // Test that CreateNew does not leave orphaned GD resources.
   

   The capitalization for CreateNew seems wrong. Maybe ImageInterface::createNew() or, simply, the ID: 'create_new'?

Overall looks OK to me.

Addressed all the comments in #20.

#22 thanks, on it

#22.1 - reverted
#22.2 - done
#22.3 - fixed - but it was 'resize' and not 'rotate' that was missing restoring the original resource. 'rotate' seems ok to me.

Whoops, missed to destroy the temporary resource.

#28 LOL :)

I would really like to see this in before RC, current workarounds are funky and this patch makes memory management just much better - what's missing to RTBC?

Looks good to me, but needs a change record.

Added draft CR https://www.drupal.org/node/2579991

This could go into a patch release afaict, hence not 'rc deadline'.

I think cleaning up after ourselves is a good idea. The change is minimal and not disruptive or risky however it does impact performance of some of the most memory intensive operations we have and therefore worth doing. Committed 42da7fa and pushed to 8.0.x. Thanks!