Skip to content

Try-catch in Imagine\Gmagick\Effects::convolve should catch GmagickException instead of ImagickException #757

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.

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

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

dmitry-kulikov
Copy link
Contributor

  1. Try-catch in Imagine\Gmagick\Effects::convolve should catch GmagickException instead of ImagickException.
  2. Changed the case of the Gmagick methods calls to lowercase to match their declarations.
  3. Imagine\Test\Gmagick\EffectsTest::testColorize now expects more specific NotSupportedException instead of rather common RuntimeException.

Main reason of this pull-request is 1), it is small bugfix.
I know that tests wanted, but I'm not sure that it is required in such case because looks like its copy-paste mistake.
I tested code using the following test:
in \Imagine\Test\Gmagick\EffectsTest add

public function testConvolutionFailed()
{
    $image = $this->getImagine()->create(new \Imagine\Image\Box(1, 1));
    $matrix = new \Imagine\Utils\Matrix(3, 3, array(
        0, 0.5, 0,
        0.5, 1, 0.5,
        0, 0.5, 0,
    ));

    $this->isGoingToThrowException('Imagine\Exception\RuntimeException');
    $image->effects()->convolve($matrix);
}

then run

./vendor/bin/phpunit tests/tests/Gmagick/EffectsTest.php

Image is too small and it leads to exception. At least for
gmagick version 2.0.4RC1
GraphicsMagick version GraphicsMagick 1.3.28 2018-01-20 Q16
I'm not sure about other versions. But if gmagick will throw an exception it should be GmagickException.

…Exception instead of \ImagickException.

Changed the case of the Gmagick methods calls to lowercase to match their declarations.
\Imagine\Test\Gmagick\EffectsTest::testColorize now expects more specific NotSupportedException instead of rather common RuntimeException.
@mlocati
Copy link
Collaborator

mlocati commented Sep 30, 2021

Thanks!

@mlocati mlocati merged commit 200d66d into php-imagine:develop Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants