From 7fe9bfe8e5a58c25dea8bcc5b8d82ae9c365cb35 Mon Sep 17 00:00:00 2001 From: Andre Klapper Date: Tue, 10 Feb 2026 18:03:58 +0100 Subject: [PATCH] Support ImageMagick 7's "magick" command to supersede "convert" Summary: Make Phorge also look for the "magick" binary which supersedes "convert" since ImageMagick version 7. Closes T16474 Test Plan: 1. Rename the commands in the two "Filesystem::binaryExists()" checks to some nonsense 2. As an admin, set http://phorge.localhost/config/edit/files.enable-imagemagick/ to `enabled` 3. See in top bar a new setup warning pointing to http://phorge.localhost/config/issue/files.enable-imagemagick/ about "'magick' or 'convert' binary not found or Imagemagick is not installed." 4. Correct the two commands in the two "Filesystem::binaryExists()" checks, as they are in this patch 5. Upload an animated GIF image file `F1`, go to http://phorge.localhost/file/transforms/1/ and manually trigger a transform by clicking a "Regenerate" button, get a result (optionally insert phlog() statements in PhabricatorFileImageTransform::applyImagemagick() to prove that ImageMagick is used) 6. Probably create some Macro meme? I did not. 7. Check the "Behavioral Changes" section on https://imagemagick.org/script/porting.php and do not spot any commands used in the Phorge codebase Reviewers: O1 Blessed Committers, avivey Reviewed By: O1 Blessed Committers, avivey Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T16474 Differential Revision: https://we.phorge.it/D26742 --- .../PhabricatorImagemagickSetupCheck.php | 47 +++++++++++++------ .../config/PhabricatorFilesConfigOptions.php | 5 +- .../PhabricatorFileImageTransform.php | 5 +- .../macro/engine/PhabricatorMemeEngine.php | 11 +++-- 4 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/applications/config/check/PhabricatorImagemagickSetupCheck.php b/src/applications/config/check/PhabricatorImagemagickSetupCheck.php index a69781132e..18f86e5edd 100644 --- a/src/applications/config/check/PhabricatorImagemagickSetupCheck.php +++ b/src/applications/config/check/PhabricatorImagemagickSetupCheck.php @@ -6,24 +6,41 @@ final class PhabricatorImagemagickSetupCheck extends PhabricatorSetupCheck { return self::GROUP_OTHER; } + /** + * Get the name of the ImageMagick binary. Since ImageMagick version 7, the + * "magick" command is replacing the old "convert" command. + * + * @return string|null + */ + public function getImageMagickBinaryName() { + if (Filesystem::binaryExists('magick')) { + return 'magick'; + } else if (Filesystem::binaryExists('convert')) { + return 'convert'; + } else { + return null; + } + } + protected function executeChecks() { $imagemagick = PhabricatorEnv::getEnvConfig('files.enable-imagemagick'); - if ($imagemagick) { - if (!Filesystem::binaryExists('convert')) { - $message = pht( - "You have enabled Imagemagick in your config, but the '%s' ". - "binary is not in the webserver's %s. Disable imagemagick ". - "or make it available to the webserver.", - 'convert', - '$PATH'); + if ($imagemagick && $this->getImageMagickBinaryName() === null) { + $message = pht( + "You have enabled Imagemagick in your config, but the '%s' or '%s' ". + "binary is not in the webserver's %s. Disable imagemagick ". + "or make it available to the webserver.", + 'magick', + 'convert', + '$PATH'); - $this->newIssue('files.enable-imagemagick') - ->setName(pht( - "'%s' binary not found or Imagemagick is not installed.", 'convert')) - ->setMessage($message) - ->addRelatedPhabricatorConfig('files.enable-imagemagick') - ->addPhabricatorConfig('environment.append-paths'); - } + $this->newIssue('files.enable-imagemagick') + ->setName(pht( + "'%s' or '%s' binary not found or Imagemagick is not installed.", + 'magick', + 'convert')) + ->setMessage($message) + ->addRelatedPhabricatorConfig('files.enable-imagemagick') + ->addPhabricatorConfig('environment.append-paths'); } } } diff --git a/src/applications/files/config/PhabricatorFilesConfigOptions.php b/src/applications/files/config/PhabricatorFilesConfigOptions.php index a908f24508..e5af6fbdf6 100644 --- a/src/applications/files/config/PhabricatorFilesConfigOptions.php +++ b/src/applications/files/config/PhabricatorFilesConfigOptions.php @@ -215,8 +215,9 @@ final class PhabricatorFilesConfigOptions pht( 'This option will use Imagemagick to rescale images, so animated '. 'GIFs can be thumbnailed and set as profile pictures. Imagemagick '. - 'must be installed and the "%s" binary must be available to '. - 'the webserver for this to work.', + 'must be installed and the "%s" or "%s" binary must be available '. + 'to the webserver for this to work.', + 'magick', 'convert')), ); diff --git a/src/applications/files/transform/PhabricatorFileImageTransform.php b/src/applications/files/transform/PhabricatorFileImageTransform.php index a5050ac7d0..8d8e90a03b 100644 --- a/src/applications/files/transform/PhabricatorFileImageTransform.php +++ b/src/applications/files/transform/PhabricatorFileImageTransform.php @@ -118,7 +118,10 @@ abstract class PhabricatorFileImageTransform extends PhabricatorFileTransform { $out = new TempFile(); - $future = new ExecFuture('convert %s %Ls %s', $tmp, $argv, $out); + $binary = id(new PhabricatorImagemagickSetupCheck()) + ->getImageMagickBinaryName(); + + $future = new ExecFuture('%s %s %Ls %s', $binary, $tmp, $argv, $out); // Don't spend more than 60 seconds resizing; just fail if it takes longer // than that. $future->setTimeout(60)->resolvex(); diff --git a/src/applications/macro/engine/PhabricatorMemeEngine.php b/src/applications/macro/engine/PhabricatorMemeEngine.php index 1dd6b17fdf..ef54d6d2c7 100644 --- a/src/applications/macro/engine/PhabricatorMemeEngine.php +++ b/src/applications/macro/engine/PhabricatorMemeEngine.php @@ -211,11 +211,14 @@ final class PhabricatorMemeEngine extends Phobject { return null; } + $binary = id(new PhabricatorImagemagickSetupCheck()) + ->getImageMagickBinaryName(); + // Test of the GIF is an animated GIF. If it's a flat GIF, we'll fall // back to GD. $input = new TempFile(); Filesystem::writeFile($input, $template_data); - list($err, $out) = exec_manual('convert %s info:', $input); + list($err, $out) = exec_manual('%s %s info:', $binary, $input); if ($err) { return null; } @@ -231,7 +234,8 @@ final class PhabricatorMemeEngine extends Phobject { $output = new TempFile(); $future = new ExecFuture( - 'convert %s -coalesce +adjoin %s_%s', + '%s %s -coalesce +adjoin %s_%s', + $binary, $input, $input, '%09d'); @@ -250,7 +254,8 @@ final class PhabricatorMemeEngine extends Phobject { } $future = new ExecFuture( - 'convert -dispose background -loop 0 %Ls %s', + '%s -dispose background -loop 0 %Ls %s', + $binary, $output_files, $output); $future->setTimeout(10)->resolvex(); -- 2.51.2