Media: Fix wp_show_heic_upload_error() assigning to an undeclared variable. - #12830
Media: Fix wp_show_heic_upload_error() assigning to an undeclared variable.#12830softglazee wants to merge 4 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. You can also manually ask me to refresh this list by adding the 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. |
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. |
…iable. The function assigned the error flag to an undeclared $plupload_init variable instead of the $plupload_settings parameter, preventing the flag from reaching the returned array. This behavior was introduced in [58849] and shipped in WordPress 6.7.0. Adds unit tests covering both the supported and unsupported cases. See #65802.
a53c8ee to
13a5841
Compare
|
Thanks @irozum for checking out the branch and running the red-green proof independently and appreciated. |
| // Check if HEIC images can be edited. | ||
| if ( ! wp_image_editor_supports( array( 'mime_type' => 'image/heic' ) ) ) { | ||
| $plupload_init['heic_upload_error'] = true; | ||
| $plupload_settings['heic_upload_error'] = true; |
There was a problem hiding this comment.
I'm quite surprised that PHPStan didn't flag this as an error. It seems that it will be implemented at some point. See phpstan/phpstan#8192.
|
Since this was dead code, I was surprised that it hadn't been noticed before. Here's a test plugin that Claude Code (Opus 5) made so I can reproduce the issue by disabling HEIC: heic-upload-error-repro.zip When attempting to upload an HEIC file from the Media > Add Media File screen in the admn. When I upload the HEIC test file included in the PHPUnit test data:
The changes here successfully cause the error to show up. |
|
🤖 Comment from Claude Opus 5: On the surprise that this went unnoticed: the changeset that broke this path also made the other path self-sufficient in the same commit, so nothing observable regressed on the screens most people use. Timeliner48288 (5.5) introduced r58849 (6.7.0) then made two changes at once. The callback was wrapped in a // Check if HEIC images can be edited.
if ( ! wp_image_editor_supports( array( 'mime_type' => 'image/heic' ) ) ) {
$defaults['heic_upload_error'] = true;
}
/**
* Filters the Plupload default settings.
* ...
*/
$defaults = apply_filters( 'plupload_default_settings', $defaults );Because that direct assignment runs first, the Where the error was still being shown
That split was verified by inspecting the rendered admin markup with HEIC editing unavailable on the server:
A further reason the regression stayed quiet: HEIC is treated differently from WebP and AVIF on the client. WebP and AVIF call Two possible follow-ups, both out of scope here
|
|
@westonruter That issue is the control flow graph work, and detecting a variable that's written but never read is one of the items listed on it. Sounds like the readonly property false positives come first, so this class of bug isn't catchable yet. |
The `wp_show_heic_upload_error()` function assigned the `heic_upload_error` flag to an undeclared `$plupload_init` variable rather than to its `$plupload_settings` parameter, so the callback returned the settings untouched and the flag never reached Plupload. Only the `plupload_init` path was affected: the `wp_plupload_default_settings()` function sets the same flag inline before applying `plupload_default_settings`, so screens fed by `_wpPluploadSettings` kept warning correctly. The regression was confined to `media-new.php` and the `media-upload.php` iframe, where `media_upload_form()` relies on this callback alone for HEIC. Also correct the documented types for the settings array from `array[]` to `array<string, mixed>`, both on the function and on the `plupload_init` hook, and add regression tests for the callback. Developed in #12830. Follow-up to r48288, r58849. Props softglaze, westonruter, khokansardar, irozum. See #53645. Fixes #65802. git-svn-id: https://develop.svn.wordpress.org/trunk@63338 602fd350-edb4-49c9-b593-d223f7449a82
The `wp_show_heic_upload_error()` function assigned the `heic_upload_error` flag to an undeclared `$plupload_init` variable rather than to its `$plupload_settings` parameter, so the callback returned the settings untouched and the flag never reached Plupload. Only the `plupload_init` path was affected: the `wp_plupload_default_settings()` function sets the same flag inline before applying `plupload_default_settings`, so screens fed by `_wpPluploadSettings` kept warning correctly. The regression was confined to `media-new.php` and the `media-upload.php` iframe, where `media_upload_form()` relies on this callback alone for HEIC. Also correct the documented types for the settings array from `array[]` to `array<string, mixed>`, both on the function and on the `plupload_init` hook, and add regression tests for the callback. Developed in WordPress/wordpress-develop#12830. Follow-up to r48288, r58849. Props softglaze, westonruter, khokansardar, irozum. See #53645. Fixes #65802. Built from https://develop.svn.wordpress.org/trunk@63338 git-svn-id: http://core.svn.wordpress.org/trunk@62531 1a063a9b-81f0-0310-95a4-ce76da25c4cd


The
wp_show_heic_upload_error()function assigned the HEIC upload error flag to an undeclared$plupload_initvariable instead of the$plupload_settingsparameter. As a result, the flag never reached the returned array. This behavior was introduced in [58849] and shipped in WordPress 6.7.0.This patch updates the variable name to
$plupload_settingsso the flag is properly appended, and adds unit tests covering both the supported and unsupported HEIC editor cases.Testing Instructions
These tests were run on trunk
8c3c976c25using PHP 8.3.32 and PHPUnit 9.6.35.npm run test:php -- --filter test_wp_show_heic_upload_errorOK (2 tests, 7 assertions)src/wp-includes/media.phpto test the baseline failure:git stash push src/wp-includes/media.phpnpm run test:php -- --filter test_wp_show_heic_upload_error1) Tests_Media::test_wp_show_heic_upload_error_adds_flag_when_not_supportedFailed asserting that an array has the key 'heic_upload_error'.git stash popTrac ticket: https://core.trac.wordpress.org/ticket/65802
Use of AI Tools
Used for: Investigating the underlying code paths, structuring the PHPUnit test scaffolding, and generating shell commands for the environment. The initial finding, manual measurements, Trac ticket description, and commit message are my own work.