Opened 3 weeks ago
Last modified 4 days ago
#66113 new defect (bug)
Use imageflip() in WP_Image_Editor_GD::flip()
| Reported by: | arshidkv12 | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.2 |
| Component: | General | Version: | |
| Severity: | normal | Keywords: | has-patch has-unit-tests |
| Cc: | Focuses: |
Description
WP_Image_Editor_GD::flip() uses negative dimensions with imagecopyresampled().
With PHP 8.6's bundled GD 2.4.0, this causes Tests_Image_Editor_GD::test_flip to fail:
Failed asserting that 0 is identical to 8434407
The same operation works with PHP 8.5/GD 2.3.3. imageflip() produces the expected result on both versions.
This patch replaces the imagecopyresampled() implementation with imageflip() while preserving the existing flip behavior.
Change History (6)
This ticket was mentioned in PR #13549 on WordPress/wordpress-develop by @arshidkv12.
3 weeks ago
#1
- Keywords has-patch added
This ticket was mentioned in PR #13920 on WordPress/wordpress-develop by sanjithsarkar.
4 days ago
#3
- Keywords has-unit-tests added
Summary
WP_Image_Editor_GD::flip() mirrored images by passing negative source dimensions to imagecopyresampled(). That only works if the GD build handles negative sizes the way the old code assumed, and it allocated a second full-size image just to flip the first. It now calls imageflip(), which flips in place and doesn't depend on that behavior. _flip_image_resource() (the deprecated GD-resource twin) gets the same change.
Trac ticket: https://core.trac.wordpress.org/ticket/66113
What Changed
| File | Change |
|---|---|
src/wp-includes/class-wp-image-editor-gd.php:441 | flip(): wp_imagecreatetruecolor() + imagecopyresampled() with negative $sw/$sh → imageflip() with IMG_FLIP_VERTICAL / IMG_FLIP_HORIZONTAL / IMG_FLIP_BOTH. Non-GD $this->image now returns WP_Error up front.
|
src/wp-admin/includes/image-edit.php:578 | _flip_image_resource(): same swap. No-op when neither axis is set; skips imageflip() if $img isn't a GD image.
|
tests/phpunit/tests/image/editorGd.php:602 | test_flip_moves_every_pixel(): 3×2 image, distinct color per pixel, all 4 flag combinations, asserts every pixel lands where expected.
|
tests/phpunit/tests/image/editorGd.php:669 | test_flip_returns_error_for_invalid_image(): flip() returns WP_Error when $this->image isn't a GD image.
|
tests/phpunit/tests/image/functions.php:1208 | test_flip_image_resource(): same pixel check for _flip_image_resource().
|
Testing
- [ ]
vendor/bin/phpunit --filter Tests_Image_Editor_GD→ passes (includes the new flip tests). - [ ]
vendor/bin/phpunit --filter test_flip_image_resource tests/phpunit/tests/image/functions.php→ passes. - [ ] Flip an image vertically in the Edit Image screen and save → top and bottom swap, left and right untouched.
- [ ] Flip an image horizontally → left and right swap, top and bottom untouched.
- [ ] Flip both axes → same result as a 180° rotation.
- [ ] Upload a JPEG with EXIF orientation 2, 3 or 4 (
maybe_exif_rotate()callsflip()) → the image comes out upright. - [ ]
flip( false, false )on a loaded editor → returnstrue, image untouched. - [ ]
flip()with no valid GD image loaded → returnsWP_Errorimage_flip_error. - [ ] Flip a PNG with transparency → alpha is preserved.
- [ ]
vendor/bin/phpcson the four changed files → clean.
Implementation Notes
- Axis mapping:
$horzflips along the horizontal axis, which mirrors top↔bottom, so it maps toIMG_FLIP_VERTICAL.$vertmaps toIMG_FLIP_HORIZONTAL. This matches the old$sy/$sxlogic exactly. The tests compare pixels, so a swap would fail them. - In place:
imageflip()mutates the existing image, so$this->imageis the same object before and after. The old code built a new image and destroyed the old one on PHP < 8. Peak memory for large images drops because only one full-size bitmap is held. - Validation first in
flip(): Theis_gd_image()check sits before the "no axes requested" early return. Otherwiseflip( false, false )on an unloaded editor would returntruewhere every other path returnsWP_Error. - Silent no-op in
_flip_image_resource(): It can't returnWP_Error. Its contract is "returns an image resource", and the caller (image_edit_apply_changes()) assigns the result to$imageand keeps using it. Returning the input unchanged matches the old failure behavior and the "(maybe) flipped" wording in its docblock. - No
function_exists( 'imageflip' )guard: It's been in PHP since 5.5 and core requires 7.4+. - Palette images: They now stay palette images after a flip. The old code converted them to truecolor. Nothing downstream relies on that, since resizing and saving go through their own conversion.
Anything the Reviewer Should Know
- On PHP 8.3 with bundled GD 2.0.35, the old code produced pixel-identical output to the new code, so the new pixel tests also pass against the old implementation here. They guard against regressions but don't prove the fix. I'd appreciate a check on the GD build from the ticket.
- Unrelated failure in the image group.
Tests_Image_Editor_Imagick::test_remove_pdf_alpha_channel_should_remove_the_alpha_channel_in_previewfails locally because Ghostscript (gs) isn't installed. This diff doesn't touch Imagick. - In-place mutation is a behavior change for direct callers. Core only does
$image = _flip_image_resource( … ), so it's fine there. A third-party caller that kept the original resource and expected it unchanged would now see it flipped. I judged that acceptable for a function deprecated since 3.5.0. - Read order:
class-wp-image-editor-gd.phpfirst (the real change), thenimage-edit.php(a mechanical mirror), then the tests.
This ticket was mentioned in PR #13925 on WordPress/wordpress-develop by @algorithmsunlocks.
4 days ago
#4
Builds on #13549 by @arshidkv12 (that change is kept as its own commit) and addresses its open review comment from @dhruvang21.
Problem
WP_Image_Editor_GD::flip() flips images with imagecopyresampled() and negative source dimensions. With PHP 8.6's bundled GD 2.4.0 this produces a blank (all-black) image, and Tests_Image_Editor_GD::test_flip fails:
Failed asserting that 0 is identical to 8434407.
Solution
- From #13549: use
imageflip(), the dedicated GD API (available since PHP 5.5). It produces the same result as before on older GD and the correct result on GD 2.4.0. - This follow-up: keep the documented
true|WP_Errorreturn contract. #13549 returnedimageflip()'sbooldirectly, soflip()could returnfalse, unlikeWP_Image_Editor::flip()and the Imagick editor. A failure now returns the existingimage_flip_errorWP_Error. - Tests:
test_flip_moves_pixelschecks every pixel of a small image with distinct colors for horizontal, vertical, both, and no flip.test_flip_returns_wp_error_on_failurecovers the error path.
Test results
| PHP 8.3.35 (GD 2.1) | PHP 8.6.0RC2 (GD 2.4.0) | |
|---|---|---|
| trunk | 28 pass, 1 error (new failure test: TypeError) | 4 failures + 1 error (test_flip, 3 new flip cases, failure test)
|
| this PR | 29/29 pass | 29/29 pass |
- The
imagetest group passes on both versions. - On trunk, the new failure test hits
TypeError: imagecopyresampled(): Argument #2 ($src_image) must be of type GdImage, false given.
How to test
LOCAL_PHP=8.6-fpm npm run env:start(PHP 8.6 with GD 2.4.0).npm run test:php -- --filter Tests_Image_Editor_GD. On trunktest_flipfails; with this PR all tests pass.- Repeat on the default PHP version (
npm run env:start) to confirm nothing changes on older GD. - Optional, in the UI: Media > Library, edit an image, use "Flip vertical" / "Flip horizontal", and save. The result should be mirrored correctly, not black.
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5.5
Used for: Reproducing the failure on PHP 8.6, root-cause investigation, writing the follow-up change and tests, and drafting this description. I reviewed and verified everything. The imageflip() change itself is the original PR author's work.
#5
@
4 days ago
Reproduction Report
Reproduced on trunk with the local Docker environment on PHP 8.6.0RC2 (bundled GD "2.4.0 compatible"). Tests_Image_Editor_GD::test_flip fails with Failed asserting that 0 is identical to 8434407. It passes on PHP 8.3.35 (bundled GD "2.1.0 compatible").
A direct comparison on a 3x2 image with distinct pixel colors shows that the current imagecopyresampled() approach with negative dimensions returns an all-black image on GD 2.4.0. imageflip() gives the correct result on both versions, and matches the old output on GD 2.1.
Patch
PR: https://github.com/WordPress/wordpress-develop/pull/13925
This builds on https://github.com/WordPress/wordpress-develop/pull/13549 (kept as its own commit) and addresses the open review comment there. Thanks to https://github.com/WordPress/wordpress-develop/pull/13920 for pointing out the deprecated _flip_image_resource(), which has the same code; it is now covered here too.
flip()keeps its documentedtrue|WP_Errorreturn. A failure returns the existingimage_flip_error, neverfalse, consistent withWP_Image_Editor::flip()and the Imagick editor._flip_image_resource()usesimageflip()as well.- New tests check every pixel for horizontal, vertical, both, and no flip, for both functions, plus the error path.
Results
| PHP 8.3 / GD 2.1 | PHP 8.6 / GD 2.4.0 | |
|---|---|---|
| trunk | 32 pass, 1 error (TypeError in the new failure test) | 7 failures + 1 error |
| patch | 33/33 pass | 33/33 pass |
The image group passes on both versions.
There are now three PRs here, and they take nearly the same approach. I'm happy to close mine in favour of whichever one committers prefer, or to have the tests cherry-picked into it (@arshidkv12, @sanjithsarkar).
AI assistance: Yes. Claude Code (Claude Opus 5.5) was used for reproduction, investigation, and writing the follow-up change and tests. I reviewed and verified everything.
@algorithmsunlocks commented on PR #13925:
4 days ago
#6
The two failing "Upgrade Develop Version Tests" jobs (Upgrade from 5.3 / PHP 8.4 with MySQL 5.7 multisite, and Upgrade from 7.1 / PHP 7.4 with MySQL 5.7) are unrelated to this change: setup-php failed to install WP-CLI ("Could not setup wp-cli"), so wp core download exits with 127. All PHPUnit jobs (PHP 7.4–8.6), coding standards, and E2E pass. Could someone re-run the failed jobs?
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
Use
imageflip()inWP_Image_Editor_GD::flip()instead ofimagecopyresampled()with negative dimensions.This fixes image flipping with PHP 8.6 / GD 2.4.0, where the existing implementation can produce incorrect pixel data.
[Trac ticket: https://core.trac.wordpress.org/ticket/66113
](https://core.trac.wordpress.org/ticket/66113)
Use of AI Tools
AI assistance: Yes
Tool(s): ChatGPT
Model(s): GPT-5.6 Luna
Used for: Debugging and test suggestions;