Refactor the image rotation logic

Found this issue via issue #3401578. I'm not able to use the solution here because of the change from using hook_file_presave to only using a form alter. In my use case, images are created via another module (media_acquiadam, but I imagine any situation where images come in via an API would be affected), so the form alter approach gets skipped while the the hook_file_presave approach would work.

Here's a patch for this MR which adds support for the imagick toolkit, if you find it useful.

Made a merge request into the 3477919-overall-combined-fix branch that adds the `hook_file_presave()` hook back in (applies orientation changes in cases where the upload validation isn't applicable), and fixes an issue where the orientation value doesn't get removed for orientations other than 5, 6, 7, and 8.
See 3477919-overall_combined_fix-tweaks

@greenskin, adding back the exif_orientation_file_presave() fixed a bug that we had where the exif rotation wasn't working when adding an image through the media library, so I'm adding this in the main MR.

For the other part regarding fixing "the orientation value doesn't get removed for orientations" can you describe how to test it ?
I haven't added it for the moment, and if it's needed then we should also patch the GD part in src/Plugin/ImageToolkit/Operation/gd/AutoOrient.php

@tostinni, as I recall the $this->addArguments(['-auto-orient']); did auto orient the image but without the exif orientation information removed, the browser was orienting the image again when viewing. I believe I specifically ran into this issue when images were converted to webp (via WebP module).

After looking at the comments, the status should've been set back to Needs Review.

This fixes "The selected image handling toolkit 'imagemagick' can not process operation 'rotate'." and works with the photos / plupload modules.

This solution works well for gd and imagemagick toolkits, but not for other (remote) toolkits like Imgix and Bunny Optimizer. Why don't we hardcode the gd toolkit for performing the rotation to the original image? It's a required PHP extension for Drupal so we can assume it's always present. This way the process doesn't break when sites are using other toolkits for creating image derivatives. Or at least make it configurable what toolkit is used.

As an extra I also added a Drush command to rotate all existing images.

This solution works well for gd and imagemagick toolkits, but not for other (remote) toolkits like Imgix and Bunny Optimizer. Why don't we hardcode the gd toolkit for performing the rotation to the original image? It's a required PHP extension for Drupal so we can assume it's always present. This way the process doesn't break when sites are using other toolkits for creating image derivatives. Or at least make it configurable what toolkit is used.

This sounds like a good solution, since gd is required. Is that something you could add to the MR?

I did! Not configurable for now though.

Never mind, forgot to push. Will add now.

I would add a comment about why gd is hardcoded there.

I was just wondering, in the gd ImageToolkitOperation, where exactly is the Orientation tag being removed from the EXIF data? If this is a side effect of the rotate operation or imageflip call we should probably document it and make sure that the tag is removed anytime it's set. Currently, it's not being removed when its value is 1 for example.

I checked and rotating the image with GD seems to also remove the Orientation tag:

~/Projects/drupal-modules/exif_orientation 3477919-overall-combined-fix ❯ php -r '$file="/Users/dieterholvoet/Projects/drupal-modules/exif_orientation/tests/rotate90cw.jpg"; $exif=@exif_read_data($file); var_export($exif["Orientation"] ?? null); echo PHP_EOL;'
6
~/Projects/drupal-modules/exif_orientation 3477919-overall-combined-fix ❯ php -m | grep -i '^exif$' || true
exif
~/Projects/drupal-modules/exif_orientation 3477919-overall-combined-fix ❯ php -r '$file="/Users/dieterholvoet/Projects/drupal-modules/exif_orientation/tests/rotate90cw.jpg"; $exif=function_exists("exif_read_data") ? @exif_read_data($file) : false; var_dump(function_exists("exif_read_data"), $exif === false ? false : ($exif["Orientation"] ?? null));'
bool(true)
int(6)
~/Projects/drupal-modules/exif_orientation 3477919-overall-combined-fix ❯ tmpdir=$(mktemp -d)
~/Projects/drupal-modules/exif_orientation 3477919-overall-combined-fix ❯ cp /Users/dieterholvoet/Projects/drupal-modules/exif_orientation/tests/rotate90cw.jpg "$tmpdir/in.jpg"
~/Projects/drupal-modules/exif_orientation 3477919-overall-combined-fix ❯ php -r '$src=$argv[1]; $img=imagecreatefromjpeg($src); imagerotate($img, -90, 0); imagejpeg($img, $argv[2]); imagedestroy($img);' "$tmpdir/in.jpg" "$tmpdir/out.jpg"
~/Projects/drupal-modules/exif_orientation 3477919-overall-combined-fix ❯ php -r '$out=$argv[1]; $exif=@exif_read_data($out); var_dump($exif === false ? false : ($exif["Orientation"] ?? null));' "$tmpdir/out.jpg"
NULL
~/Projects/drupal-modules/exif_orientation 3477919-overall-combined-fix ❯ rm -rf "$tmpdir"

I'll add a comment explaining this and update the tests as well.

@dieterholvoet Thank you for your work!

I wonder if it will be better to return false when image is not rotated to avoid unneeded file operation?

In \Drupal\exif_orientation\Plugin\ImageToolkit\Operation\gd\AutoOrient plugin:

    if (!$source_path = $this->getToolkit()->getSource()) {
      return FALSE; // changed from TRUE
    }

        default:
          $result = FALSE;
      }

      return $result;
    }

    return FALSE; // changed from TRUE
  }

}

Forget this if is pointless or wrong.