Temporary copies of remote files are never deleted

Excellent catch, thank you! We need to fix 8.x-3.x first, then backport.

+++ b/src/EventSubscriber/ImagemagickEventSubscriber.php
@@ -295,4 +303,18 @@ class ImagemagickEventSubscriber implements EventSubscriberInterface {
+  /**
+   * Removes a temporary file created during operations on a remote file.
+   *
+   * Used with drupal_register_shutdown_function().
+   *
+   * @param string $path
+   *   The temporary file realpath.
+   */
+  public function removeTemporaryRemoteCopy($path) {
+    if (file_exists($path)) {
+      $this->fileSystem->unlink($path);
+    }
+  }
+

I would prefer to use a static function here. See for example how this was done for Image Effects in #3046522: Remove imagemagick texttowrapper temporary files on shutdown, see commit https://git.drupalcode.org/project/image_effects/commit/3893f53.

+++ b/tests/src/Functional/ToolkitImagemagickTest.php
@@ -65,6 +65,44 @@ class ToolkitImagemagickTest extends BrowserTestBase {
+    $this->assertEqual(count(file_scan_directory('temporary://', '/ima.*/')), 0, 'There are no leftovers in the temporary directory after getting metadata from a remote file.');

We could use assertCount here instead, and be more PHPUnit compliant...

#12 well that was changed in Image Effects 8.x-3.x likewise, see https://git.drupalcode.org/project/image_effects/blob/8.x-3.x/src/Plugin...

Also, see the getSupportedExtensions method in the ImagemagickToolkit class, which is a static - we have an example of using the service from the static method, too.

I do not know, honestly. What concerns me is that keeping it with the instantiated service in the container may have side effects when running the callbacks in drupal_register_shutdown_function. Now it works, but in the future who knows... just for an example, if you were to run the callbacks in register_shutdown_function (i.e. normal PHP, not Drupal), at the stage of executing the callbacks the container is already destructed (reason why we made a change in #3046522-5: Remove imagemagick texttowrapper temporary files on shutdown). OTOH, I see in drupal core usages also with instantiated methods in services.

Thank you @Leksat.

Just a couple minor things, then it's ready:

1. +++ b/tests/src/Functional/ToolkitImagemagickTest.php
   @@ -61,6 +61,38 @@ class ToolkitImagemagickTest extends BrowserTestBase {
   +    // Get metadata from a remote file.
   +    $image = $this->imageFactory->get('dummy-remote://image-test.png');
   +    $image->getToolkit()->getExifOrientation();
   +
   

   Let's add an assertCount for the files in tmp here too, to show that the file exists before being deleted.

2. +++ b/tests/src/Functional/ToolkitImagemagickTest.php
   @@ -61,6 +61,38 @@ class ToolkitImagemagickTest extends BrowserTestBase {
   +      if ($callback['callback'] === ['Drupal\imagemagick\EventSubscriber\ImagemagickEventSubscriber', 'removeTemporaryRemoteCopy']) {
   

   Let's use the ::class notation here, so ImagemagickEventSubscriber::class instead of the FQCN string.

Crediting

Committed to 8.x-3.x. Leaving open for backport to 8.x-2.x, if necessary. Thanks!

+++ b/src/EventSubscriber/ImagemagickEventSubscriber.php
@@ -295,4 +303,18 @@ class ImagemagickEventSubscriber implements EventSubscriberInterface {
+      \Drupal::service('file_system')->delete($path);

I know, I know... but we need to use the old file_unmanaged_delete here since 8.x-2.x is still supporting Drupal core before 8.7.

Committed #31 to 8.x-2.x. Thanks!