Skip to content
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

Fix a bug in ImagickDriver::pixelate that caused a DivideByZero excep… #262

Closed
wants to merge 3 commits into from

Conversation

psion-ar
Copy link
Contributor

Hi there :)

Line 477 in ImagickDriver caused a DivideByZero exception.

   FAIL  Tests\Manipulations\PixelateTest
  ⨯ it can pixelate an image with dataset "imagick" / (0)                                                                                                                                   0.02s  
  ✓ it can pixelate an image with dataset "imagick" / (50)                                                                                                                                  0.16s  
  ✓ it can pixelate an image with dataset "imagick" / (100)                                                                                                                                 0.12s  
  ✓ it can pixelate an image with dataset "gd" / (0)                                                                                                                                        0.11s  
  ✓ it can pixelate an image with dataset "gd" / (50)                                                                                                                                       0.12s  
  ✓ it can pixelate an image with dataset "gd" / (100)                                                                                                                                      0.12s  
  ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────  
  FAILED  Tests\Manipulations\PixelateTest > it can pixelate an image with dataset "imagick" / (0)                                                                            DivisionByZeroError   
  Division by zero

  at src/Drivers/Imagick/ImagickDriver.php:477
    473▕         $width = $this->getWidth();
    474▕         $height = $this->getHeight();
    475▕ 
    476▕         foreach ($this->image as $image) {
  ➜ 477▕             $image->scaleImage(max(1, (int) ($width / $pixelate)), max(1, (int) ($height / $pixelate)));
    478▕             $image->scaleImage($width, $height);
    479▕         }
    480▕ 
    481▕         return $this;

  1   src/Drivers/Imagick/ImagickDriver.php:477
  2   src/Image.php:266


  Tests:    1 failed, 5 passed (5 assertions)
  Duration: 0.74s

I just wrapped the pixelate logic in a conditional, to run only when $pixelate !== 0.
The method signatures for all 3 "effect" methods that require argument values remain the same.
The tests are updated (BlurTest and SharpenTest too) to cover edge cases and to
stay consistent across the three method's i mentioned.

btw... <3

Regard's Alex

p.s. dont know why the rotation snapshots got updated. didnt mess with those

@@ -470,12 +470,14 @@ public function flip(FlipDirection $flip): static

public function pixelate(int $pixelate = 50): static
{
$width = $this->getWidth();
$height = $this->getHeight();
if ($pixelate !== 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's check if $pixelate is 0 and then to an early return. This way, we'll lose a level of indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok new commit... hope i did not mess up.
(first contribution ever:x)

@freekmurze
Copy link
Member

This PR contains a lot of stylistic changes (how null is checked, strict type checking).

Could you remove those change and only change the bare necessities?

@psion-ar
Copy link
Contributor Author

psion-ar commented Jul 16, 2024

Of course.

commit 9b3baf3b2d2703f5984430635d91cc35815eea3a
Author: Alex Roth <[email protected]>
Date:   Tue Jul 16 20:03:48 2024 +0200

    Fix bug in ImagickDriver::pixelate, Update tests

commit 42dff38
Merge: c15fa28 862b7a8
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Mon Jul 8 05:01:25 2024 +0000

    Merge pull request spatie#263 from spatie/dependabot/github_actions/dependabot/fetch-metadata-2.2.0

    Bump dependabot/fetch-metadata from 2.1.0 to 2.2.0

commit 862b7a8
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Jul 8 05:01:13 2024 +0000

    Bump dependabot/fetch-metadata from 2.1.0 to 2.2.0

    Bumps [dependabot/fetch-metadata](https://github.com/dependabot/fetch-metadata) from 2.1.0 to 2.2.0.
    - [Release notes](https://github.com/dependabot/fetch-metadata/releases)
    - [Commits](dependabot/fetch-metadata@v2.1.0...v2.2.0)

    ---
    updated-dependencies:
    - dependency-name: dependabot/fetch-metadata
      dependency-type: direct:production
      update-type: version-update:semver-minor
    ...

    Signed-off-by: dependabot[bot] <[email protected]>
@psion-ar psion-ar closed this Jul 16, 2024
@psion-ar psion-ar reopened this Jul 16, 2024
@psion-ar
Copy link
Contributor Author

oh i messed up...big sry...

@psion-ar psion-ar closed this Jul 16, 2024
@psion-ar psion-ar deleted the psion-ar/bugfix/pixelate branch July 16, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants