Skip to content

Fix Image Rotator - #478

Open
andrewdalpino wants to merge 3 commits into
masterfrom
fix-image-rotator
Open

Fix Image Rotator#478
andrewdalpino wants to merge 3 commits into
masterfrom
fix-image-rotator

Conversation

@andrewdalpino

Copy link
Copy Markdown
Member

Fix in src/Transformers/ImageRotator.php:114-132 — Replaced the imagecrop call (whose box went negative when width/height swapped) with a resize onto a new canvas of original dimensions. This:

  • Always yields the original W×H (matching the documented "original size" contract).
  • Never returns false, so can't clobber the sample.
  • Throws Rubix\ML\Exceptions\RuntimeException if GD allocation fails — consistent with ImageResizer.php:118-126.

@andrewdalpino
andrewdalpino requested review from a team and a lite review from Copilot September 7, 2026 03:54

Copilot AI left a comment

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.

🟡 Changes recommended

The newly added tests destroy images stored inside a dataset while reusing the same dataset across loop iterations, which can cause subsequent transformations to run on destroyed GD image handles.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates ImageRotator to avoid negative crop boxes when rotating images (notably at 90°/270° on non-square aspect ratios) by replacing the crop step with a resize onto a new canvas sized to the original image dimensions, and adds regression tests for those rotations.

Changes:

  • Replace imagecrop() with a “resize back to original W×H” path and throw RuntimeException on GD allocation/copy failures.
  • Update docblocks to reflect resizing behavior and potential exceptions.
  • Add test coverage for wide/tall/extreme-ratio 90°/270° rotations and a 45° square rotation.
File summaries
File Description
src/Transformers/ImageRotator.php Replaces center-cropping after rotation with resizing back to the original dimensions, adding runtime error handling.
tests/Transformers/ImageRotatorTest.php Adds new rotation/shape regression tests to ensure output images remain original size.
Review details

Suppressed comments (1)

tests/Transformers/ImageRotatorTest.php:100

  • This test has the same issue as transformWideImage90Degrees(): the dataset is reused across iterations, but the image stored in it is destroyed at the end of the first loop. Recreate the image+dataset per iteration (or avoid destroying until after the loop) so the second apply() doesn't run on a destroyed GdImage.
        foreach ([90.0, 270.0] as $degrees) {
            $mock = $this->createPartialMock(ImageRotator::class, ['rotationAngle']);
            $mock->method('rotationAngle')->will($this->returnValue($degrees));

            $dataset->apply($mock);
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/Transformers/ImageRotatorTest.php Outdated
Comment thread src/Transformers/ImageRotator.php Outdated
andrewdalpino and others added 2 commits September 6, 2026 23:02
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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