Bug: ImageMagickHandler throws exception without imagick extension

[blurpy]

PHP Version

7.4

CodeIgniter4 Version

4.2.1

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

cli-server (PHP built-in webserver)

Database

MariaDB v5.5

What happened?

I'm migrating an app from CodeIgniter 3 to 4, and when trying to use the new image library it throws an exception:

CodeIgniter\Images\Exceptions\ImageException

The framework needs the following extension(s) installed and loaded: IMAGICK.

I have never installed the imagick extension with ci3, and according to the documentation it should not be required in ci4 either:

The ImageMagick handler does NOT require the imagick extension to be loaded on the server. As long as your script can access the library and can run exec() on the server, it should work.

Steps to Reproduce

Having just the command line imagemagick tools installed without the imagick extension.

.env:

images.defaultHandler = 'imagick'
images.libraryPath = '/usr/bin/convert'

class Images extends BaseController {

    private BaseHandler $image;

    public function initController(RequestInterface $request, ResponseInterface $response, LoggerInterface $logger) {
        // Do Not Edit This Line
        parent::initController($request, $response, $logger);

        $this->image = Services::image(); // Exception thrown here
    }
...

Expected Output

Would expect the library to load without the imagick extension installed.

Anything else?

Looking at the description in the code, it seems to agree with the documentation in that the extension is not required:

CodeIgniter4/system/Images/Handlers/ImageMagickHandler.php

Lines 22 to 24 in cee5528

	* To make this library as compatible as possible with the broadest
	* number of installations, we do not use the Imagick extension,
	* but simply use the command line version.

However, the actual code looks for the extension and if it's not there it throws an exception in the constructor:

CodeIgniter4/system/Images/Handlers/ImageMagickHandler.php

Lines 46 to 54 in cee5528

	public function __construct($config = null)
	{
	parent::__construct($config);
	// We should never see this, so can't test it
	// @codeCoverageIgnoreStart
	if (! (extension_loaded('imagick') || class_exists(Imagick::class))) {
	throw ImageException::forMissingExtension('IMAGICK');
	}

If I comment out throw ImageException::forMissingExtension('IMAGICK'); then the library allows me to resize an image, so the documentation seems to be correct in that the extension is not required.

I have not finished migration yet, so there could be some cases that do not work though.

[MGatner]

It seems that the extension is actually required, as noted in that header comment:

hmm - the width & height accessors at the end use the imagick extension

Even though the User Guide explicitly states it is not necessary: https://codeigniter4.github.io/CodeIgniter4/libraries/images.html#initializing-the-class

I'm not familiar with the "width & height accessors" but that seems to be the area of need.

[kenjis]

It seems these accessors are used only for testing. These methods should be @internal?

CodeIgniter4/system/Images/Handlers/ImageMagickHandler.php

Lines 421 to 424 in f77d6f4

	public function _getWidth()
	{
	return imagesx(imagecreatefromstring(file_get_contents($this->resource)));
	}

CodeIgniter4/system/Images/Handlers/BaseHandler.php

Lines 755 to 764 in f77d6f4

	* Return image width.
	*
	* accessor for testing; not part of interface
	*
	* @return int
	*/
	public function getWidth()
	{
	return ($this->resource !== null) ? $this->_getWidth() : $this->width;
	}

[MGatner]

Definitely. I assume the _ was in part supposed to indicate this.

If we make these @internal and remove the exception the this class should work without the extension?

[kenjis]

If we make these @internal and remove the exception the this class should work without the extension?

I think so.

[MGatner]

Can't be worse than it already is 🤷‍♂️

[kenjis]

Sorry, I missed this:

CodeIgniter4/system/Images/Handlers/ImageMagickHandler.php

Lines 315 to 324 in e92a5d0

	protected function supportedFormatCheck()
	{
	switch ($this->image()->imageType) {
	case IMAGETYPE_WEBP:
	if (! in_array('WEBP', Imagick::queryFormats(), true)) {
	throw ImageException::forInvalidImageCreate(lang('images.webpNotSupported'));
	}
	break;
	}
	}

[kenjis]

It looks like we were planning to change it to use the imagick extension.

CodeIgniter4/system/Images/Handlers/ImageMagickHandler.php

Line 28 in e92a5d0

* FIXME - This needs conversion & unit testing, to use the imagick extension

[kenjis]

@blurpy Why don't you install the imagick extension?

[blurpy]

I guess I could. I checked the production server, and both of these return true:

var_dump(extension_loaded('imagick'));
var_dump(class_exists(Imagick::class));

If the plan is to actually use the imagick extension then I can update my development environment.

[MGatner]

If the current implementation relies on the extension and we have an exception enforcing that then the straightforward thing is to change the UG to match. I'm not familiar enough with Imagick and the differences between the installation and extension to make suggestions in implementation changes.

[kenjis]

@blurpy I've created the issue: #6317

There is a problem with the current ImageMagickHandler and it seems to need to be fixed.

[blurpy]

@kenjis Thanks, that issue with multiple calls to convert is a bit worrisome as I need scaling to be fast enough for users not to notice. And that works really well in ci3.

I also have a feature request I added a few days ago in the forums: https://forum.codeigniter.com/showthread.php?tid=82610
No one has approved it so it's not visible yet. But I need to be able to strip the exif when scaling images. Would be nice for that to be included while work is being done on the class.