Repository navigation
Media: Use imageflip() in WP_Image_Editor_GD::flip() and keep its true|WP_Error contract. - #13925
RumanCodes wants to merge 5 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
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: |
Replaces the imagecopyresampled() implementation, which relies on negative source dimensions, with imageflip(). The old approach produces incorrect pixel data with PHP 8.6's bundled GD 2.4.0. See #66113.
…()`. Converts a failed `imageflip()` call, or a missing GD image, into the existing `image_flip_error` WP_Error instead of returning `false`, matching `WP_Image_Editor::flip()` and the Imagick implementation. Adds tests confirming that every pixel moves to the expected position for each flip direction, and that a failure returns a WP_Error. Props ruman. See #66113. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`_flip_image_resource()` used the same `imagecopyresampled()` approach with negative source dimensions as `WP_Image_Editor_GD::flip()`, which produces a blank image with PHP 8.6's bundled GD 2.4.0. It now uses `imageflip()` as well, and returns the image unchanged when it is not a GD image or no axis is given. Props ruman. See #66113. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c9ab51e to
1b65126
Compare
|
@propsbot add dhruvang21 |
|
@propsbot add arshidkv12 |
|
@propsbot add @sanjithsarkar. |
There was a problem hiding this comment.
🟡 Changes recommended
Both flip paths lose the previous truecolor conversion, causing indexed PNG/GIF exports to WebP to produce no image data; transparency-aware conversion must be restored and regression coverage added.
2 open issues
What changed in this PR
This PR replaces GD’s negative-dimension resampling with imageflip() to address incorrect image flipping on GD 2.4.0, while preserving the editor’s documented true|WP_Error return contract.
Changes:
- Uses in-place flipping in the GD editor and deprecated image-resource helper.
- Returns
image_flip_errorwhen the editor cannot perform a requested flip. - Adds pixel-position tests for all axis combinations and an invalid-image failure test.
| File | Description |
|---|---|
tests/phpunit/tests/image/editorGd.php |
Adds pixel-level flip checks and error-contract coverage. |
src/wp-includes/class-wp-image-editor-gd.php |
Replaces resampling with imageflip() and retains error handling. |
src/wp-admin/includes/image-edit.php |
Updates the deprecated flip helper to use imageflip(). |
Review effort: Balanced
| if ( ! is_gd_image( $img ) || ( ! $horz && ! $vert ) ) { | ||
| return $img; | ||
| } |
| if ( ! $horz && ! $vert ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Thanks for this, @RumanCodes!
When you have a chance, could you please link your WordPress.org user account with your GitHub one? You'll find instructions on the Make Core blog. This ensures you'll rightly receive attribution for you contributions in the release.
Since Trac is still the canonical source of truth for this repository (it's a mirror of the SVN repository), conversation often happens there. I added a comment with feedback on Core-66113, but I could not mention you by username because I'm not clear what that is.
In addition to the point raised contextually, could you please add @since tags noting the differences in behavior where appropriate? Also, it would be great if you could include the list of other changes that will result from this switch started on #13920 in the PR description. Those cases will also need unit tests, and a few may require decisions or additional considerations in the code. Copilot was kind enough to flag a few of those for you as well.
| if ( is_gd_image( $this->image ) && imageflip( $this->image, $mode ) ) { | ||
| return true; |
There was a problem hiding this comment.
Since this image check happens after the early return for the dimensions, an invalid image that's passed will never be flagged when dimensions are empty.
In my opinion, it's preferable to flag the more severe issue first. The dimensions do not matter if the image is invalid or empty.
|
@desrosj thanks! I have connected my github now! |
Thank you! It seems that #13920 is a bit further along and more polished after my most recent review, so I'm going to close this one out to avoid splitting efforts and attention. I hope you'll continue helping to test this issue over there, but you'll receive credit regardless for your help so far! |
Trac ticket: https://core.trac.wordpress.org/ticket/66113
Builds on #13549 by @arshidkv12 (that change is kept as its own commit) and addresses its open review comment from @dhruvang21. Thanks to #13920 by @sanjithsarkar for pointing out the deprecated
_flip_image_resource(), which has the same code; it is covered here too.Problem
WP_Image_Editor_GD::flip()flips images withimagecopyresampled()and negative source dimensions. With PHP 8.6's bundled GD 2.4.0 this produces a blank (all-black) image, andTests_Image_Editor_GD::test_flipfails:Solution
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.true|WP_Errorreturn contract. GD: Use imageflip() for flipping images #13549 returnedimageflip()'s bool directly, soflip()could returnfalse, unlikeWP_Image_Editor::flip()and the Imagick editor. A failure now returns the existingimage_flip_errorWP_Error._flip_image_resource()usesimageflip()the same way.tests/phpunit/tests/image/editorGd.php):test_flip_moves_pixelsandtest_flip_image_resource_moves_pixelscheck every pixel of a small image with distinct colors, for horizontal, vertical, both, and no flip (4 data cases each).test_flip_returns_wp_error_on_failurecovers the error path.Test results
On the current branch (rebased on trunk),
--filter Tests_Image_Editor_GD(33 test runs):TypeErrorin the new failure test)On PHP 8.6, trunk fails the existing
test_flip(Failed asserting that 0 is identical to 8434407.) and all six flip cases of the new pixel tests. On GD 2.1 the old code already flips correctly, so the pixel tests pass on trunk there.--group imagepasses with this PR on PHP 8.6 (280 tests, 49 skipped). PHPCS and PHPCompatibility are clean.How to test
LOCAL_PHP=8.6-fpm npm run env:start(PHP 8.6 with GD 2.4.0).LOCAL_PHP=8.6-fpm npm run test:php -- --filter Tests_Image_Editor_GD. On trunktest_flipfails; with this PR all tests pass.npm run env:start) to confirm nothing changes on older GD.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.This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.