Issue #3509696 by eelkeblok: Cleaning files may fail with a PelException

.gitignore

+.ddev
+.editorconfig
+.idea
+.gitattributes
+composer.lock
+vendor
+web

.gitlab-ci.yml

+################
+# DrupalCI GitLabCI template
+#
+# See https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-drupal/gitlab-ci
+# Based on https://git.drupalcode.org/project/gitlab_templates/-/blob/1.0.x/gitlab-ci/template.gitlab-ci.yml
+#
+################
+
+include:
+  ################
+  # DrupalCI includes:
+  # As long as you include this, any future includes added by the Drupal Association will be accessible to your pipelines automatically.
+  # View these include files at https://git.drupalcode.org/project/gitlab_templates/
+  ################
+  - project: $_GITLAB_TEMPLATES_REPO
+    ref: $_GITLAB_TEMPLATES_REF
+    file:
+      - '/includes/include.drupalci.main.yml'
+      # EXPERIMENTAL: For Drupal 7, remove the above line and uncomment the below.
+      # - '/includes/include.drupalci.main-d7.yml'
+      - '/includes/include.drupalci.variables.yml'
+      - '/includes/include.drupalci.workflows.yml'
+
+################
+# Pipeline configuration variables
+#
+# These are the variables provided to the Run Pipeline form that a user may want to override.
+#
+# Docs at https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes/include.drupalci.variables.yml
+variables:
+  # Broaden test coverage.
+  OPT_IN_TEST_CURRENT: 0
+  OPT_IN_TEST_PREVIOUS_MAJOR: 1
+  OPT_IN_TEST_MAX_PHP: 1
+  # Convenient, and we have no secrets.
+  _SHOW_ENVIRONMENT_VARIABLES: 1

composer.json

@@ -11,5 +11,8 @@
   "require": {
     "drupal/core": "^10.0",
     "fileeye/pel": "^0.10.0"
+  },
+  "require-dev": {
+    "drush/drush": "*"
   }
 }

exif_manipulate.module

@@ -2,16 +2,15 @@
 
 /**
  * @file
- * Contains exif_manipulate.module.
+ * Hook implementations for exif_manipulate.module.
  */
 
+use Drupal\exif_manipulate\Hooks\FileEntity;
 use Drupal\file\Entity\File;
 
 /**
  * Implements hook_ENTITY_TYPE_insert().
  */
 function exif_manipulate_file_insert(File $file): void {
-  /** @var Drupal\exif_manipulate\Services\FileExifProcessorInterface $exifProcessor */
-  $exifProcessor = \Drupal::service('exif_manipulate.file_exif_processor');
-  $exifProcessor->manipulate($file);
+  \Drupal::classResolver(FileEntity::class)->fileInsert($file);
 }

src/Form/ExifManipulateForm.php

@@ -116,13 +116,6 @@ class ExifManipulateForm extends ConfirmFormBase {
       '#default_value' => 'sites/default/files',
     ];
 
-    $form['actions']['#type'] = 'actions';
-    $form['actions']['submit'] = [
-      '#type' => 'submit',
-      '#value' => $this->t('Clean directory'),
-      '#button_type' => 'primary',
-    ];
-
     return parent::buildForm($form, $form_state);
   }
 

src/Hooks/FileEntity.php

+<?php
+
+namespace Drupal\exif_manipulate\Hooks;
+
+use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
+use Drupal\Core\Logger\LoggerChannelTrait;
+use Drupal\Core\Messenger\MessengerInterface;
+use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Drupal\Core\Utility\Error;
+use Drupal\exif_manipulate\Services\FileExifProcessorInterface;
+use Drupal\file\Entity\File;
+use lsolesen\pel\PelException;
+use Psr\Log\LoggerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+
+/**
+ * Hook implementations dealing with file entities.
+ */
+final class FileEntity implements ContainerInjectionInterface {
+
+  use StringTranslationTrait;
+
+  public function __construct(
+    readonly protected FileExifProcessorInterface $exifProcessor,
+    readonly protected MessengerInterface $messenger,
+    readonly protected LoggerInterface $logger,
+  ) {
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    $instance = new self(
+      $container->get('exif_manipulate.file_exif_processor'),
+      $container->get('messenger'),
+      $container->get('logger.channel.exif_manipulate'),
+    );
+
+    $instance->stringTranslation = $container->get('string_translation');
+
+    return $instance;
+  }
+
+  /**
+   * Implements hook_ENTITY_TYPE_insert().
+   */
+  public function fileInsert(File $file): void {
+    try {
+      $this->exifProcessor->manipulate($file);
+    }
+    catch (PelException $e) {
+      Error::logException(
+        $this->logger,
+        $e,
+        'There was a problem removing the EXIF metadata from %image. %type: @message in %function (line %line of %file).',
+        ['%image' => $file->getFileUri()]
+      );
+      $this->messenger->addWarning('There was a problem removing the metadata from your file.');
+    }
+  }
+
+}

src/Plugin/QueueWorker/CleanExifDataQueueWorker.php

@@ -3,11 +3,12 @@
 namespace Drupal\exif_manipulate\Plugin\QueueWorker;
 
 use Drupal\Core\Entity\EntityTypeManagerInterface;
-use Drupal\Core\Logger\LoggerChannelInterface;
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Drupal\Core\Queue\QueueWorkerBase;
 use Drupal\Core\Utility\Error;
 use Drupal\exif_manipulate\Services\FileExifProcessorInterface;
+use Psr\Log\LoggerInterface;
+use Psr\Log\LogLevel;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
@@ -38,9 +39,9 @@ class CleanExifDataQueueWorker extends QueueWorkerBase implements ContainerFacto
   /**
    * Our logger channel.
    *
-   * @var \Drupal\Core\Logger\LoggerChannelInterface
+   * @var \Psr\Log\LoggerInterface
    */
-  protected LoggerChannelInterface $logger;
+  protected LoggerInterface $logger;
 
   /**
    * {@inheritdoc}
@@ -69,8 +70,8 @@ class CleanExifDataQueueWorker extends QueueWorkerBase implements ContainerFacto
    *   The entity type manager.
    * @param \Drupal\exif_manipulate\Services\FileExifProcessorInterface $fileExifProcessor
    *   The file Exif processor service.
-   * @param \Drupal\Core\Logger\LoggerChannelInterface $logger
-   *   A logger channel.
+   * @param \Psr\Log\LoggerInterface $logger
+   *   A logger.
    */
   public function __construct(
     array $configuration,
@@ -78,7 +79,7 @@ class CleanExifDataQueueWorker extends QueueWorkerBase implements ContainerFacto
     $plugin_definition,
     EntityTypeManagerInterface $entityTypeManager,
     FileExifProcessorInterface $fileExifProcessor,
-    LoggerChannelInterface $logger
+    LoggerInterface $logger
   ) {
     parent::__construct($configuration, $plugin_id, $plugin_definition);
     $this->entityTypeManager = $entityTypeManager;
@@ -95,7 +96,7 @@ class CleanExifDataQueueWorker extends QueueWorkerBase implements ContainerFacto
         ->loadByProperties(['uri' => $data['uri']]);
     }
     catch (\Exception $e) {
-      Error::logException($this->logger, $e);
+      Error::logException($this->logger, $e, 'Could not load the file entity for processing. %type: @message in %function (line %line of %file).', [], LogLevel::WARNING);
 
       return;
     }
@@ -103,7 +104,13 @@ class CleanExifDataQueueWorker extends QueueWorkerBase implements ContainerFacto
     if (!empty($files)) {
       /** @var \Drupal\file\FileInterface $file */
       $file = reset($files);
-      $this->fileExifProcessor->manipulate($file);
+
+      try {
+        $this->fileExifProcessor->manipulate($file);
+      }
+      catch (\Exception $e) {
+        Error::logException($this->logger, $e, 'There was a problem removing the EXIF metadata from %image. %type: @message in %function (line %line of %file).', ['%image' => $file->getFileUri()]);
+      }
     }
   }
 

src/Services/FileExifProcessor.php

@@ -12,7 +12,7 @@ use lsolesen\pel\PelTiff;
 
 /**
  * Class FileProcessorInterface processes EXIF data for files.
- * */
+ */
 class FileExifProcessor implements FileExifProcessorInterface {
 
   /**

src/Services/FileExifProcessorInterface.php

@@ -14,6 +14,11 @@ interface FileExifProcessorInterface {
    *
    * @param \Drupal\file\FileInterface $file
    *   The file entity.
+   *
+   * @throws \lsolesen\pel\PelException
+   *   The operations in the method may throw a PelException when there is
+   *   anything wrong with the Exif data. The calling code needs to take
+   *   appropriate action.
    */
   public function manipulate(FileInterface $file): void;
 

tests/src/Functional/ImageAssertionTrait.php

+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\exif_manipulate\Functional;
+
+use lsolesen\pel\PelJpeg;
+use lsolesen\pel\PelTag;
+
+trait ImageAssertionTrait {
+
+  // Add abstract copies of the methods from BrowserTestBase.
+  static abstract public function assertNotNull($actual, string $message = ''): void;
+  static abstract public function assertContains($needle, \Traversable|array $haystack, string $message = ''): void;
+  static abstract public function fail(string $message = ''): void;
+
+  /**
+   * Verify that an image no longer contains any EXIF data except orientation.
+   */
+  private function assertImageIsClean($file) {
+    $pelFile = new PelJpeg($file->getFileUri());
+
+    $exif = $pelFile->getExif();
+    $this->assertNotNull($exif, 'The image has EXIF data.');
+    $tiff = $exif->getTiff();
+    $this->assertNotNull($tiff, 'There is a TIFF object in the EXIF data.');
+    $ifd = $tiff->getIfd();
+    $this->assertNotNull($ifd, 'There is an Image File Directory object in the TIFF data.');
+    $tagsFound = [];
+    $pelTagReflection = new \ReflectionClass(PelTag::class);
+    $tagsAvailable = $pelTagReflection->getConstants();
+
+    do {
+      foreach ($tagsAvailable as $tag) {
+        $entry = $ifd->getEntry($tag);
+        if ($entry) {
+          $tagsFound[$entry->getTag()] = $ifd->getType();
+        }
+      }
+    } while ($ifd = $ifd->getNextIfd());
+
+    $this->assertContains(PelTag::ORIENTATION, array_keys($tagsFound), 'The image should have orientation data.');
+
+    foreach ($tagsFound as $tag => $type)  {
+      if ($tag === PelTag::ORIENTATION) {
+        continue;
+      }
+      $this->fail('The image should not have any tags other than Orientation. Found: ' . PelTag::getName($type, $tag));
+    }
+  }
+
+}

tests/src/Functional/QueueTest.php

+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\exif_manipulate\Functional;
+
+use ColinODell\PsrTestLogger\TestLogger;
+use Drupal\Core\File\FileSystemInterface;
+use Drupal\Core\Logger\LoggerChannelInterface;
+use Drupal\Core\Site\Settings;
+use Drupal\file\FileInterface;
+use Drupal\Tests\BrowserTestBase;
+use Drupal\Tests\UiHelperTrait;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Tests for the cleanup queue.
+ *
+ * @group exif_manipulate
+ */
+class QueueTest extends BrowserTestBase {
+
+  use ImageAssertionTrait;
+  use UiHelperTrait;
+
+  const QUEUE_NAME = 'exif_manipulate_clean_exif_data';
+
+  /**
+   * The profile to install as a basis for testing.
+   *
+   * @var string
+   */
+  protected $profile = 'minimal';
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';
+
+  /**
+   * Admin user account.
+   *
+   * @var \Drupal\user\Entity\User
+   */
+  protected $adminUser;
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  protected static $modules = ['system', 'image', 'exif_manipulate'];
+
+  /**
+   * The file directory for the tested site.
+   *
+   * @var string $fileDir
+   */
+  protected string $fileDir;
+
+  /**
+   * The test logger.
+   */
+  protected LoggerInterface $logger;
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setUp(): void {
+    parent::setUp();
+    $this->adminUser = $this->drupalCreateUser([
+      'administer exif manipulate',
+    ]);
+    $this->drupalLogin($this->adminUser);
+
+    // Test if directories specified in settings exist in filesystem.
+    $this->fileDir = Settings::get('file_public_path');
+    \Drupal::service('file_system')
+      ->prepareDirectory($this->fileDir, FileSystemInterface::CREATE_DIRECTORY);
+
+    $this->logger = new TestLogger();
+
+    $container = \Drupal::getContainer();
+    $container->set('logger.channel.exif_manipulate', $this->logger);
+  }
+
+  /**
+   * Tests that the cleanup queue is working.
+   */
+  public function testCleanupQueue(): void {
+    $file = $this->createEntityForFile('location.jpg');
+
+    $this->drupalGet('admin/config/media/exif_manipulate');
+    $this->assertSession()->statusCodeEquals(200);
+
+    // Submit the form to clean the EXIF data.
+    $this->submitForm(['directory' => $this->fileDir], 'Clean images');
+
+    // CHeck that we see the success message.
+    $this->assertSession()->pageTextContains('The images have been queued for cleaning.');
+
+    // Process the queue.
+    $queue = \Drupal::queue(self::QUEUE_NAME);
+    $this->processQueue($queue);
+
+    // Check that the queue is now empty.
+    $this->assertEquals(0, $queue->numberOfItems(), 'The queue should be empty.');
+
+    // Check that the image was cleaned.
+    $this->assertImageIsClean($file);
+  }
+
+  /**
+   * Test that an invalid image does not trip up the queue.
+   */
+  public function testQueueWithInvalidImage(): void {
+    $file = $this->createEntityForFile('invalid.jpg');
+
+    $this->drupalGet('admin/config/media/exif_manipulate');
+    $this->assertSession()->statusCodeEquals(200);
+
+    // Submit the form to clean the EXIF data.
+    $this->submitForm(['directory' => $this->fileDir], 'Clean images');
+
+    // Process the queue.
+    $queue = \Drupal::queue(self::QUEUE_NAME);
+    $this->processQueue($queue);
+
+    // Check that the queue is now empty.
+    $this->assertEquals(0, $queue->numberOfItems(), 'The queue should be empty.');
+    $this->assertTrue($this->logger->hasErrorThatContains('There was a problem removing the EXIF metadata from %image.'), 'The error message should be logged.');
+  }
+
+  /**
+   * Creates a file entity for a given file in the tests/files directory.
+   */
+  public function createEntityForFile(string $filename): FileInterface {
+    $image = \Drupal::service('extension.list.module')
+        ->getPath('exif_manipulate') . '/tests/files/' . $filename;
+    $destination = Settings::get('file_public_path') . '/' . $filename;
+    copy($image, $destination);
+    $fileStorage = \Drupal::entityTypeManager()->getStorage('file');
+    $file = $fileStorage->create([
+      'uri' => 'public://' . $filename,
+    ]);
+    assert($file instanceof FileInterface);
+    $file->save();
+
+    return $file;
+  }
+
+  /**
+   * Processes the queue.
+   *
+   * @param \Drupal\Core\Queue\QueueInterface $queue
+   *   The queue to process.
+   */
+  protected function processQueue($queue): void {
+    // Get the queue worker.
+    $queueWorker = \Drupal::service('plugin.manager.queue_worker')
+      ->createInstance(self::QUEUE_NAME);
+
+    while ($item = $queue->claimItem()) {
+      $queueWorker->processItem($item->data);
+      $queue->deleteItem($item);
+    }
+  }
+}

tests/src/Functional/StripTest.php

+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\exif_manipulate\Functional;
+
+use Drupal\Core\File\FileSystemInterface;
+use Drupal\Core\Site\Settings;
+use Drupal\file\Entity\File;
+use Drupal\Tests\BrowserTestBase;
+use Drupal\Tests\UiHelperTrait;
+
+/**
+ * Tests stripping Exif data.
+ *
+ * @group exif_manipulate
+ */
+class StripTest extends BrowserTestBase {
+
+  use ImageAssertionTrait;
+  use UiHelperTrait;
+
+  /**
+   * The profile to install as a basis for testing.
+   *
+   * Using the standard profile to test user picture config provided by the
+   * standard profile.
+   *
+   * @var string
+   */
+  protected $profile = 'standard';
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';
+
+  /**
+   * Admin user account.
+   *
+   * @var \Drupal\user\Entity\User
+   */
+  protected $adminUser;
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  protected static $modules = ['system', 'image', 'exif_manipulate'];
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setUp(): void {
+    parent::setUp();
+    $this->adminUser = $this->drupalCreateUser([
+      'administer site configuration',
+    ]);
+    $this->drupalLogin($this->adminUser);
+
+    // Test if directories specified in settings exist in filesystem.
+    $file_dir = Settings::get('file_public_path');
+    \Drupal::service('file_system')->prepareDirectory($file_dir, FileSystemInterface::CREATE_DIRECTORY);
+
+    $picture_dir = \Drupal::state()->get('user_picture_path', 'pictures');
+    $picture_path = $file_dir . $picture_dir;
+
+    \Drupal::service('file_system')->prepareDirectory($picture_path, FileSystemInterface::CREATE_DIRECTORY);
+    $directory_writable = is_writable($picture_path);
+    $this->assertTrue($directory_writable, "The directory $picture_path should exist and should be writable.");
+  }
+
+  /**
+   * Test stripping data from user profile pictures.
+   */
+  public function testUserPicture() {
+    $this->drupalLogin($this->adminUser);
+    $this->assertSession()->statusCodeEquals(200);
+    $image = \Drupal::service('extension.list.module')->getPath('exif_manipulate') . '/tests/files/location.jpg';
+    $file = $this->saveUserPicture($image);
+    $this->assertImageIsClean($file);
+  }
+
+  /**
+   * Test stripping data from an invalid image.
+   */
+  public function testInvalidImage() {
+    $this->drupalLogin($this->adminUser);
+    $this->assertSession()->statusCodeEquals(200);
+    $image = \Drupal::service('extension.list.module')
+        ->getPath('exif_manipulate') . '/tests/files/invalid.jpg';
+    $this->saveUserPicture($image);
+    $textContent = $this->getTextContent();
+    $this->assertStringContainsString('There was a problem removing the metadata from your file.', $textContent, 'Feedback must be give about the failed operation.');
+  }
+
+  /**
+   * Uploads a user picture.
+   */
+  private function saveUserPicture($image) {
+    $edit = ['files[user_picture_0]' => \Drupal::service('file_system')->realpath($image)];
+    $uid = $this->adminUser->id();
+    $this->drupalGet('user/' . $uid . '/edit');
+    $this->submitForm($edit, 'Save');
+
+    // Load actual user data from database.
+    $user_storage = $this->container->get('entity_type.manager')->getStorage('user');
+    $user_storage->resetCache([$this->adminUser->id()]);
+
+    $account = $user_storage->load($this->adminUser->id());
+    return File::load($account->user_picture->target_id);
+  }
+
+}