Add focal point cropping - #48
Conversation
|
Thanks for contributing! This repository is a read-only mirror; development for this library happens in |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Greptile SummaryThis PR extends
Confidence Score: 4/5The PR appears safe to merge, with non-blocking hardening needed for non-finite coordinate validation and animated focal-point regression coverage. The focal geometry and frame lifecycle are consistent with existing crop behavior, but Files Needing Attention: src/Image/Image.php, tests/Image/ImageTest.php Important Files Changed
Prompt To Fix All With AI### Issue 1
src/Image/Image.php:114
**NaN Bypasses Coordinate Validation**
The public float parameters accept `NAN`, but comparisons such as `NAN < 0` and `NAN > 1` are false in PHP. This lets a non-finite coordinate reach the crop-offset calculation and produce an unintended crop instead of the expected `InvalidArgumentException`. Reject non-finite coordinates before checking their range.
```suggestion
if ($x !== null && (!\is_finite($x) || !\is_finite($y) || $x < 0 || $x > 1 || $y < 0 || $y > 1)) {
```
### Issue 2
tests/Image/ImageTest.php:126
**Animated Focal Path Untested**
The successful focal-point test covers only a single-frame PNG, while focal coordinates send animated images through a separate per-frame scale-and-crop path. Add an animated focal-point test that verifies frame dimensions, selected content, and aggregate delay so regressions in this newly reachable path are detected.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat: support focal point cropping" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| throw new \InvalidArgumentException('Both focal point coordinates are required'); | ||
| } | ||
|
|
||
| if ($x !== null && ($x < 0 || $x > 1 || $y < 0 || $y > 1)) { |
There was a problem hiding this comment.
The public float parameters accept NAN, but comparisons such as NAN < 0 and NAN > 1 are false in PHP. This lets a non-finite coordinate reach the crop-offset calculation and produce an unintended crop instead of the expected InvalidArgumentException. Reject non-finite coordinates before checking their range.
| if ($x !== null && ($x < 0 || $x > 1 || $y < 0 || $y > 1)) { | |
| if ($x !== null && (!\is_finite($x) || !\is_finite($y) || $x < 0 || $x > 1 || $y < 0 || $y > 1)) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Image/Image.php
Line: 114
Comment:
**NaN Bypasses Coordinate Validation**
The public float parameters accept `NAN`, but comparisons such as `NAN < 0` and `NAN > 1` are false in PHP. This lets a non-finite coordinate reach the crop-offset calculation and produce an unintended crop instead of the expected `InvalidArgumentException`. Reject non-finite coordinates before checking their range.
```suggestion
if ($x !== null && (!\is_finite($x) || !\is_finite($y) || $x < 0 || $x > 1 || $y < 0 || $y > 1)) {
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| $source->drawImage($draw); | ||
|
|
||
| $image = new Image($source->getImageBlob()); | ||
| $image->crop(2, 2, x: 0.75, y: 0.5); |
There was a problem hiding this comment.
The successful focal-point test covers only a single-frame PNG, while focal coordinates send animated images through a separate per-frame scale-and-crop path. Add an animated focal-point test that verifies frame dimensions, selected content, and aggregate delay so regressions in this newly reachable path are detected.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/Image/ImageTest.php
Line: 126
Comment:
**Animated Focal Path Untested**
The successful focal-point test covers only a single-frame PNG, while focal coordinates send animated images through a separate per-frame scale-and-crop path. Add an animated focal-point test that verifies frame dimensions, selected content, and aggregate delay so regressions in this newly reachable path are detected.
**Knowledge Base Used:**
- [Image manipulation library](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/image/-/docs/image-manipulation-library.md)
- [Animated image processing](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/image/-/docs/animated-image-processing.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 374f6b1c4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| throw new \InvalidArgumentException('Both focal point coordinates are required'); | ||
| } | ||
|
|
||
| if ($x !== null && ($x < 0 || $x > 1 || $y < 0 || $y > 1)) { |
There was a problem hiding this comment.
When either coordinate is NAN, all four range comparisons evaluate to false, so the value passes validation despite not being between 0 and 1. The subsequent min/max calculation can silently select an image boundary (or produce conversion warnings depending on the PHP version), yielding an incorrect crop; explicitly reject non-finite coordinates before calculating the offsets.
Useful? React with 👍 / 👎.
1 participant
Summary
Image::crop()to accept normalizedxandyfocal coordinatesTests
vendor/bin/phpunit --testsuite unit --filter CropFocal(PHP 8.3 + Imagick): 3 tests, 4 assertionsvendor/bin/phpstan analyse --level max src tests --memory-limit=512Mphp -l src/Image/Image.phpgit diff --check