Skip to content

Use Imagick methods instead of mogrify() for repo theme images #2375

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sbreker wants to merge 4 commits into
base: qa/2.x
Choose a base branch
from
+193 −29
Open

Use Imagick methods instead of mogrify() for repo theme images #2375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
410068e
Use Imagick methods for resizing repo theme images
sbreker Jun 19, 2026
4ff2a7c
Update repository logo and banner crop logic
sbreker Jun 27, 2026
333640d
Add Imagick resource limit theme img processing
sbreker Jun 27, 2026
a484528
Use class method to check if Imagick in use
sbreker Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions apps/qubit/modules/repository/actions/editThemeAction.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ protected function addField($name)
sfContext::getInstance()->getConfiguration()->loadHelpers('Url');

$this->form->setValidator($name, new sfValidatorFile([
'max_size' => '262144', // 256K
'max_size' => '512000', // 500K
'mime_types' => ['image/png'],
// Crop image, it is synchronous but it should be fast
'validated_file_class' => 'arRepositoryThemeCropValidatedFile',
Expand All @@ -129,7 +129,7 @@ protected function addField($name)
$this->form->setWidget($name, new arB5WidgetFormInputFileEditable([
'label' => $this->context->i18n->__('Banner'),
'help' => $this->context->i18n->__(
'Requirements: PNG format, 256K max. size.<br />Recommended dimensions of %1%x%2%px, it will be cropped if ImageMagick is installed.',
'Requirements: PNG format, 500K max. size.<br />Recommended dimensions of %1%x%2%px, it will be cropped if ImageMagick is installed.',
[
'%1%' => arRepositoryThemeCropValidatedFile::BANNER_MAX_WIDTH,
'%2%' => arRepositoryThemeCropValidatedFile::BANNER_MAX_HEIGHT,
Expand All @@ -153,7 +153,7 @@ protected function addField($name)
sfContext::getInstance()->getConfiguration()->loadHelpers('Url');

$this->form->setValidator($name, new sfValidatorFile([
'max_size' => '262144', // 256K
'max_size' => '512000', // 500K
'mime_types' => ['image/png'],
// Crop image, it is synchronous but it should be fast
'validated_file_class' => 'arRepositoryThemeCropValidatedFile',
Expand All @@ -164,7 +164,7 @@ protected function addField($name)
$this->form->setWidget($name, new arB5WidgetFormInputFileEditable([
'label' => $this->context->i18n->__('Logo'),
'help' => $this->context->i18n->__(
'Requirements: PNG format, 256K max. size.<br />Recommended dimensions of %1%x%2%px, it will be cropped if ImageMagick is installed.',
'Requirements: PNG format, 500K max. size.<br />Recommended dimensions of %1%x%2%px, it will be cropped if ImageMagick is installed.',
[
'%1%' => arRepositoryThemeCropValidatedFile::LOGO_MAX_WIDTH,
'%2%' => arRepositoryThemeCropValidatedFile::LOGO_MAX_HEIGHT,
Expand Down
115 changes: 90 additions & 25 deletions lib/form/arRepositoryThemeCropValidatedFile.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,48 +29,113 @@ class arRepositoryThemeCropValidatedFile extends sfValidatedFile
public const LOGO_MAX_HEIGHT = 270;
public const BANNER_MAX_WIDTH = 800;
public const BANNER_MAX_HEIGHT = 300;
public const IMAGICK_MEMORY_LIMIT_MB = 64;
public const IMAGICK_MAP_LIMIT_MB = 64;
public const IMAGICK_AREA_LIMIT_PIXELS = 16000000;

public function save($file = null, $fileMode = 0666, $create = true, $dirMode = 0777)
{
$file = parent::save($file, $fileMode, $create, $dirMode);

// Check if mogrify is available in the system
exec('which mogrify', $output, $status);
if (0 < $status) {
if (!$this->shouldCropImages()) {
return $file;
}

// Figure out necessary dimensions from the filename
$pathInfo = pathinfo($this->savedName);
if (null === $dimensions = self::getTargetDimensionsFromPath($this->savedName)) {
return $file;
}

$this->resizeAndCropImage($dimensions['width'], $dimensions['height']);

return $file;
}

public static function getTargetDimensionsFromPath($path)
{
$pathInfo = pathinfo($path);

switch ($pathInfo['filename']) {
case 'logo':
$width = self::LOGO_MAX_WIDTH;
$height = self::LOGO_MAX_HEIGHT;

break;
return [
'width' => self::LOGO_MAX_WIDTH,
'height' => self::LOGO_MAX_HEIGHT,
];

case 'banner':
$width = self::BANNER_MAX_WIDTH;
$height = self::BANNER_MAX_HEIGHT;

break;
return [
'width' => self::BANNER_MAX_WIDTH,
'height' => self::BANNER_MAX_HEIGHT,
];
}
}

// Stop execution if dimensions were not set
if (!isset($width, $height)) {
return $file;
public static function getResizeGeometry($sourceWidth, $sourceHeight, $targetWidth, $targetHeight)
{
if (
0 >= $sourceWidth
|| 0 >= $sourceHeight
|| 0 >= $targetWidth
|| 0 >= $targetHeight
) {
return null;
}

// mogrify overwrites the original image file
$command = sprintf(
'mogrify -crop %sx%s+0+0 %s',
$width,
$height,
$this->savedName
);
exec($command, $output, $status);
// Scale to fully cover the target box while preserving aspect ratio.
$scale = max($targetWidth / $sourceWidth, $targetHeight / $sourceHeight);
$resizeWidth = (int) ceil($sourceWidth * $scale);
$resizeHeight = (int) ceil($sourceHeight * $scale);

return $file;
return [
// Crop back to the exact target size from the centered overflow.
'resizeWidth' => $resizeWidth,
'resizeHeight' => $resizeHeight,
'cropX' => max(0, (int) floor(($resizeWidth - $targetWidth) / 2)),
'cropY' => max(0, (int) floor(($resizeHeight - $targetHeight) / 2)),
];
}

protected function shouldCropImages()
{
return QubitDigitalObject::imagickExtensionLoaded();
}
Comment on lines +97 to +100

@anvit anvit Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of defining this method, could we just re-use QubitDigitalObject::imagickExtensionLoaded since that is a static method?

@sbreker sbreker Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea - I have updated this to use the static method 👍


protected function resizeAndCropImage($width, $height)
{
try {
$image = new Imagick();
$image->setResourceLimit(Imagick::RESOURCETYPE_MEMORY, self::IMAGICK_MEMORY_LIMIT_MB);
$image->setResourceLimit(Imagick::RESOURCETYPE_MAP, self::IMAGICK_MAP_LIMIT_MB);
$image->setResourceLimit(Imagick::RESOURCETYPE_AREA, self::IMAGICK_AREA_LIMIT_PIXELS);
$image->readImage($this->savedName);
$geometry = self::getResizeGeometry(
$image->getImageWidth(),
$image->getImageHeight(),
$width,
$height
);

if (null === $geometry) {
$image->clear();
$image->destroy();

return;
}

$image->resizeImage(
$geometry['resizeWidth'],
$geometry['resizeHeight'],
Imagick::FILTER_LANCZOS,
1
);
$image->cropImage($width, $height, $geometry['cropX'], $geometry['cropY']);
$image->setImagePage(0, 0, 0, 0);
$image->stripImage();
$image->setImageCompressionQuality(90);
$image->writeImage($this->savedName);
$image->clear();
$image->destroy();
} catch (Exception $e) {
// Leave the uploaded file untouched if Imagick cannot process it.
}
}
}
99 changes: 99 additions & 0 deletions test/phpunit/lib/form/arRepositoryThemeCropValidatedFileTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
<?php

use PHPUnit\Framework\TestCase;

require_once __DIR__.'/../../../../lib/form/arRepositoryThemeCropValidatedFile.class.php';

/**
* @internal
*
* @covers \arRepositoryThemeCropValidatedFile
*/
class arRepositoryThemeCropValidatedFileTest extends TestCase
{
public function testLogoUploadsUseExpectedTargetDimensions()
{
$this->assertSame(
[
'width' => arRepositoryThemeCropValidatedFile::LOGO_MAX_WIDTH,
'height' => arRepositoryThemeCropValidatedFile::LOGO_MAX_HEIGHT,
],
arRepositoryThemeCropValidatedFile::getTargetDimensionsFromPath('/tmp/logo.png')
);
}

public function testBannerUploadsUseExpectedTargetDimensions()
{
$this->assertSame(
[
'width' => arRepositoryThemeCropValidatedFile::BANNER_MAX_WIDTH,
'height' => arRepositoryThemeCropValidatedFile::BANNER_MAX_HEIGHT,
],
arRepositoryThemeCropValidatedFile::getTargetDimensionsFromPath('/tmp/banner.png')
);
}

public function testUnknownUploadNamesAreNotCropped()
{
$this->assertNull(arRepositoryThemeCropValidatedFile::getTargetDimensionsFromPath('/tmp/other.png'));
}

public function testResizeGeometryForLandscapeLogoCentersHorizontalCrop()
{
$this->assertSame(
[
'resizeWidth' => 540,
'resizeHeight' => 270,
'cropX' => 135,
'cropY' => 0,
],
arRepositoryThemeCropValidatedFile::getResizeGeometry(1200, 600, 270, 270)
);
}

public function testResizeGeometryForPortraitLogoCentersVerticalCrop()
{
$this->assertSame(
[
'resizeWidth' => 270,
'resizeHeight' => 540,
'cropX' => 0,
'cropY' => 135,
],
arRepositoryThemeCropValidatedFile::getResizeGeometry(600, 1200, 270, 270)
);
}

public function testResizeGeometryForBannerCentersVerticalCrop()
{
$this->assertSame(
[
'resizeWidth' => 800,
'resizeHeight' => 600,
'cropX' => 0,
'cropY' => 150,
],
arRepositoryThemeCropValidatedFile::getResizeGeometry(1600, 1200, 800, 300)
);
}

public function testResizeGeometryRejectsInvalidDimensions()
{
$this->assertNull(arRepositoryThemeCropValidatedFile::getResizeGeometry(0, 1200, 270, 270));
}

public function testCroppingRequiresImagickExtension()
{
$file = new TestRepositoryThemeCropValidatedFile('logo.png', 'image/png', '/tmp/logo.png', 0);

$this->assertSame(extension_loaded('imagick'), $file->shouldCropImagesForTest());
}
}

class TestRepositoryThemeCropValidatedFile extends arRepositoryThemeCropValidatedFile
{
public function shouldCropImagesForTest()
{
return $this->shouldCropImages();
}
}
Loading