Make WordPress Core

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

Use imageflip() in WP_Image_Editor_GD::flip() instead of imagecopyresampled() 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;

#2 @SergeyBiryukov
3 weeks ago

  • Milestone Awaiting Review → 7.2

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() calls flip()) → the image comes out upright.
  • [ ] flip( false, false ) on a loaded editor → returns true, image untouched.
  • [ ] flip() with no valid GD image loaded → returns WP_Error image_flip_error.
  • [ ] Flip a PNG with transparency → alpha is preserved.
  • [ ] vendor/bin/phpcs on the four changed files → clean.

Implementation Notes

  • Axis mapping: $horz flips along the horizontal axis, which mirrors top↔bottom, so it maps to IMG_FLIP_VERTICAL. $vert maps to IMG_FLIP_HORIZONTAL. This matches the old $sy/$sx logic exactly. The tests compare pixels, so a swap would fail them.
  • In place: imageflip() mutates the existing image, so $this->image is 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(): The is_gd_image() check sits before the "no axes requested" early return. Otherwise flip( false, false ) on an unloaded editor would return true where every other path returns WP_Error.
  • Silent no-op in _flip_image_resource(): It can't return WP_Error. Its contract is "returns an image resource", and the caller (image_edit_apply_changes()) assigns the result to $image and 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_preview fails 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.php first (the real change), then image-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_Error return contract. #13549 returned imageflip()'s bool directly, so flip() could return false, unlike WP_Image_Editor::flip() and the Imagick editor. A failure now returns the existing image_flip_error WP_Error.
  • Tests:
    • test_flip_moves_pixels checks every pixel of a small image with distinct colors for horizontal, vertical, both, and no flip.
    • test_flip_returns_wp_error_on_failure covers 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 image test 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

  1. LOCAL_PHP=8.6-fpm npm run env:start (PHP 8.6 with GD 2.4.0).
  2. npm run test:php -- --filter Tests_Image_Editor_GD. On trunk test_flip fails; with this PR all tests pass.
  3. Repeat on the default PHP version (npm run env:start) to confirm nothing changes on older GD.
  4. 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 @algorithmsunlocks
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 documented true|WP_Error return. A failure returns the existing image_flip_error, never false, consistent with WP_Image_Editor::flip() and the Imagick editor.
  • _flip_image_resource() uses imageflip() 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?

Note: See TracTickets for help on using tickets.