[3.x] Add edge image editor saves#168
Conversation
7ca1d8b to
19827b5
Compare
![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: 19827b5f8d
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Adds a Craft Cloud image-editor save handler that converts Craft’s image editor state (crop/rotate/flip/zoom + focal point) into Cloudflare image transform options, allowing edited images to be saved/replaced without doing server-side image decoding.
Changes:
- Register an asset-image save event handler and implement a new
ImageEditorclass that downloads the transformed image and saves/replaces the asset. - Add
ImageTransformer::fromImageEditor()to map editor state into a CloudImageTransform+ Cloudflare options. - Expand Cloudflare option output to include
quality, and add unit tests covering quality + editor mappings.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/ImageTransformTest.php | Adds unit tests for quality mapping and image-editor state → Cloudflare option mapping. |
| src/Module.php | Hooks Craft’s “before save image” event to the new Cloud image editor save handler when Asset CDN is enabled. |
| src/imagetransforms/ImageTransformer.php | Adds fromImageEditor() to build a Cloud image transform from editor crop/rotate/flip/zoom. |
| src/imagetransforms/ImageTransformBehavior.php | Updates Cloudflare docs link and includes quality in generated Cloudflare options. |
| src/imagetransforms/ImageEditor.php | New handler to intercept image-editor saves, compute focal point/transform, and replace or create assets by downloading Cloudflare-transformed output. |
| composer.json | Tightens supported Craft CMS versions to those exposing the image-editor save event. |
| composer.lock | Updates dependency lockfile (including Craft CMS and transitive packages). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3d2a075 to
2b63be8
Compare
2b63be8 to
a0c1aa8
Compare
a0c1aa8 to
14a2c65
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/imagetransforms/ImageTransformBehavior.php:90
qualityis declared asint|string|null, but transform-array normalization only attempts integer validation when a union containsint(see ImageTransformer::normalizeCloudOption). As a result, string quality values like 'low'/'high' will currently be rejected and silently dropped when passed via transform arrays/options.
/**
* @var int<1, 100>|'high'|'medium-high'|'medium-low'|'low'|null
*/
public int|string|null $quality = null;
src/imagetransforms/ImageEditor.php:119
crop()can produce negativeleft/topor widths/heights that extend outside the edited image. That can cascade into invalid focal point math; it’s safer to constrain the crop rectangle to the rotated image bounds before returning it.
$focal['x'] = 1 - $focal['x'];
}
if (!empty($flipData['y'])) {
$focal['y'] = 1 - $focal['y'];
}
return $focal;
}
protected function crop(Asset $asset, int $rotation, array $cropData, array $imageDimensions, float $zoom): array
{
$adjustmentRatio = min(
$asset->width / $imageDimensions['width'],
$asset->height / $imageDimensions['height'],
);
$editedDimensions = $this->rotatedDimensions($asset->width, $asset->height, $rotation);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
src/imagetransforms/ImageTransformer.php:53
fromImageEditor()reads$imageDimensions[...],$cropData[...], and divides by$zoom/ dimension values without validating the expected keys and that values are > 0. Because these come from the image-editor request payload, malformed input can trigger undefined-index notices or DivisionByZeroError (e.g.$zoom= 0, width/height = 0). Add a small guard that validates required keys and positive numbers and throwNotSupportedExceptionso the controller can return a 400.
$sourceReferenceDimensions = self::rotatedDimensions($imageDimensions['width'], $imageDimensions['height'], 0);
$cropReferenceDimensions = self::rotatedDimensions($imageDimensions['width'], $imageDimensions['height'], $rotation);
$cropDimensions = [
'width' => (int)round($cropData['width']),
'height' => (int)round($cropData['height']),
src/imagetransforms/ImageEditor.php:95
focalPoint()assumes the incoming$focalPointpayload hasoffsetX/offsetY/imageDimensions[...]and that the derived crop width/height are non-zero. With malformed input, this can raise undefined-index notices or DivisionByZeroError, and the computed focal point can fall outside the expected 0..1 range. Validate required keys/positive dimensions and clamp the final x/y to [0,1] before saving.
$adjustmentRatio = min(
$asset->width / $focalPoint['imageDimensions']['width'],
$asset->height / $focalPoint['imageDimensions']['height'],
);
src/imagetransforms/ImageEditor.php:117
crop()divides by$imageDimensions['width'/'height']and multiplies by$zoomwithout validating that dimensions are present and > 0 (and that$zoomis > 0). Since these values originate from the editor request payload, a bad request can currently trigger division-by-zero or warnings. Add a guard and throwNotSupportedExceptionfor invalid payloads.
$adjustmentRatio = min(
$asset->width / $imageDimensions['width'],
$asset->height / $imageDimensions['height'],
);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/imagetransforms/ImageEditor.php:67
- In
editImage(), the computed focal point is stored in$focal, but the call toImageTransformer::fromImageEditor()passes the original$focalPointinput instead. Even thoughfromImageEditor()doesn’t currently use this argument, this mismatch is confusing and likely to become a bug if focal point support is added there.
$focal = $this->focalPoint($asset, $focalPoint, $viewportRotation, $imageRotation, $cropData, $imageDimensions, $flipData, $zoom);
$transform = ImageTransformer::fromImageEditor($asset, $viewportRotation, $imageRotation, $cropData, $imageDimensions, $flipData, $zoom, $focalPoint);
src/imagetransforms/ImageTransformer.php:39
ImageTransformer::fromImageEditor()declares a$focalPointparameter, but it’s currently unused. This makes the API misleading (and suggests missing functionality). Either remove the parameter + update call sites, or implement focal-point-aware transform behavior.
public static function fromImageEditor(
Asset $asset,
int $viewportRotation,
float $imageRotation,
array $cropData,
array $imageDimensions,
?array $flipData,
float $zoom,
?array $focalPoint = null,
): ?ImageTransform {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/imagetransforms/ImageEditor.php:143
crop()can calculate a zero/negative width/height (after rounding and/or when offsets push the crop fully out of bounds). That can later cause invalid trim values and will also trigger division-by-zero infocalPoint()(which divides by$crop['width']/$crop['height']). It should validate/constrain the crop and reject invalid results.
protected function crop(Asset $asset, int $rotation, array $cropData, array $imageDimensions, float $zoom): array
{
$adjustmentRatio = min(
$asset->width / $imageDimensions['width'],
$asset->height / $imageDimensions['height'],
);
$editedDimensions = $this->rotatedDimensions($asset->width, $asset->height, $rotation);
$width = (int)round($cropData['width'] * $zoom * $adjustmentRatio);
$height = (int)round($cropData['height'] * $zoom * $adjustmentRatio);
return [
'left' => (int)round(($editedDimensions['width'] / 2) + ($cropData['offsetX'] * $zoom * $adjustmentRatio) - ($width / 2)),
'top' => (int)round(($editedDimensions['height'] / 2) + ($cropData['offsetY'] * $zoom * $adjustmentRatio) - ($height / 2)),
'width' => $width,
'height' => $height,
];
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/imagetransforms/ImageEditor.php:146
crop()can return negative/out-of-bounds rectangles and can also round down to a 0 width/height (e.g. fractionalcropData['width']/height), which would lead to incorrect focal-point mapping and potential division-by-zero infocalPoint(). Consider clamping the crop rectangle to the edited image bounds and rejecting non-positive dimensions, similar to the constraining logic used for transforms inImageTransformer::fromImageEditor().
protected function crop(Asset $asset, int $rotation, array $cropData, array $imageDimensions, float $zoom): array
{
$adjustmentRatio = min(
$asset->width / $imageDimensions['width'],
$asset->height / $imageDimensions['height'],
);
$editedDimensions = $this->rotatedDimensions($asset->width, $asset->height, $rotation);
$width = (int)round($cropData['width'] * $zoom * $adjustmentRatio);
$height = (int)round($cropData['height'] * $zoom * $adjustmentRatio);
return [
'left' => (int)round(($editedDimensions['width'] / 2) + ($cropData['offsetX'] * $zoom * $adjustmentRatio) - ($width / 2)),
'top' => (int)round(($editedDimensions['height'] / 2) + ($cropData['offsetY'] * $zoom * $adjustmentRatio) - ($height / 2)),
'width' => $width,
'height' => $height,
];
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a023294ede
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1601a9698
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/imagetransforms/ImageEditor.php:49
- The RequestException handler uses the upstream HTTP response body as the client-facing BadRequest message. That can leak internal details (and potentially signed transform URLs) and may return large/HTML responses to the CP UI. Prefer a fixed, safe message here (and optionally log details separately).
} catch (RequestException $e) {
$message = trim((string)$e->getResponse()?->getBody()) ?: 'Could not save the edited image.';
throw new BadRequestHttpException($message, 0, $e);
src/imagetransforms/ImageEditor.php:161
- This class duplicates the image-editor math/validation already implemented in ImageTransformer (crop/rotation/right-angle checks/rotatedDimensions/validDimensions). Keeping these in sync will be error-prone over time; consider extracting a shared internal helper (or reusing ImageTransformer’s methods where possible) so the transform + focal-point paths can’t drift.
protected function crop(Asset $asset, int $rotation, array $cropData, array $imageDimensions, float $zoom): array
{
$adjustmentRatio = min(
$asset->width / $imageDimensions['width'],
$asset->height / $imageDimensions['height'],
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/imagetransforms/ImageEditor.php:49
- The RequestException handler uses the upstream response body as the BadRequest message. That body can contain sensitive details (including signed transform URLs or internal error context) and would be returned to the client. It’s safer to always return a generic message here and rely on server-side logs for diagnostics.
throw new BadRequestHttpException($e->getMessage(), 0, $e);
} catch (TransferException $e) {
$message = $e instanceof RequestException
? trim((string)$e->getResponse()?->getBody())
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e42ad3991c
ℹ️ 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".
| "bref/bref": "^3", | ||
| "bref/extra-php-extensions": "^3", | ||
| "craftcms/cms": "^4.6 || ^5", | ||
| "craftcms/cms": "^4.18.2 || ^5.10.6", |
There was a problem hiding this comment.
With this constraint, the Craft 4.6–4.18.1 projects advertised as supported in README.md:18 can no longer resolve or install this extension. Update the installation/support documentation to state the new 4.18.2 minimum so users do not encounter an unexpected Composer conflict.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/imagetransforms/ImageEditor.php:54
handleSaveImage()uses the upstream HTTP response body as the exception message. That can leak implementation details (and potentially sensitive info) back to the client and also produce very large/unstructured error messages. It’s safer to return a consistent generic message here (and optionally log details server-side).
} catch (TransferException $e) {
$message = $e instanceof RequestException
? trim((string)$e->getResponse()?->getBody())
: '';
$message = $message ?: 'Could not save the edited image.';
throw new BadRequestHttpException($message, 0, $e);
}
src/imagetransforms/ImageEditor.php:395
createAsset()unconditionally setssanitizeOnUpload = false, which can skip SVG sanitization when the source asset is an SVG. Consider enabling sanitization forimage/svg+xmlto avoid storing potentially unsafe SVG content.
$newAsset->avoidFilenameConflicts = true;
$newAsset->setScenario(Asset::SCENARIO_CREATE);
$newAsset->sanitizeOnUpload = false;
$newAsset->setFilename($asset->getFilename());
| if ($transform !== null) { | ||
| $tempPath = $this->downloadEditedImage($asset, $transform); | ||
| $asset->sanitizeOnUpload = false; | ||
| Craft::$app->getAssets()->replaceAssetFile($asset, $tempPath, $asset->getFilename()); | ||
| return; | ||
| } |
Description
Adds a Cloud image-editor save handler that maps Craft editor state to Cloudflare image transform options, then saves or replaces the asset without PHP image decoding.
Requires Craft CMS versions that expose the image-editor save event and expands Cloudflare option support for the editor transform path.