Opened 5 weeks ago
Closed 3 weeks ago
#65802 closed defect (bug) (fixed)
wp_show_heic_upload_error() assigns to an undeclared variable
| Reported by: | softglaze | Owned by: | westonruter |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.2 |
| Component: | Media | Version: | 6.7 |
| Severity: | normal | Keywords: | has-patch has-unit-tests |
| Cc: | Focuses: |
Description
The wp_show_heic_upload_error() function in src/wp-includes/media.php (line 5879) modifies an undeclared variable at line 5882 instead of the parameter it receives. The function returns the parameter untouched, so the flag is never added.
The function is hooked at admin-filters.php:19 (plupload_init) and default-filters.php:693 (plupload_default_settings).
Here is the exact code block:
function wp_show_heic_upload_error( $plupload_settings ) {
// Check if HEIC images can be edited.
if ( ! wp_image_editor_supports( array( 'mime_type' => 'image/heic' ) ) ) {
$plupload_init['heic_upload_error'] = true;
}
return $plupload_settings;
}
Testing the Forced Condition
Testing was done on trunk 8c3c976c25 (WP 7.1-beta4-62899-src, Docker/nginx, PHP 8.3.32) using a mu-plugin (add_filter( 'wp_image_editors', '__return_empty_array' )) to force the wp_image_editor_supports() check to return false.
Under these conditions, evaluating the function directly via WP-CLI with wp_show_heic_upload_error( array( 'existing' => 'value' ) ) returns the unmodified array: array(1) { ["existing"] => "value" }.
Checking the page source on media-new.php confirms the impact: "webp_upload_error":true and "avif_upload_error":true are successfully appended, but heic_upload_error is entirely absent, even though all three checks use the same wp_image_editor_supports() call in the same block.
Introduced in
This behaviour was introduced in [58849] on 5 Aug 2024 to fix #53645 (shipped in 6.7.0). Prior to that changeset, the function correctly assigned the value to $plupload_settings.
Change History (10)
This ticket was mentioned in PR #12828 on WordPress/wordpress-develop by @khokansardar.
5 weeks ago
#1
- Keywords has-patch has-unit-tests added; needs-patch removed
This ticket was mentioned in PR #12830 on WordPress/wordpress-develop by @softglaze.
5 weeks ago
#2
The wp_show_heic_upload_error() function assigned the HEIC upload error flag to an undeclared $plupload_init variable instead of the $plupload_settings parameter. 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_settings so 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 8c3c976c25 using PHP 8.3.32 and PHPUnit 9.6.35.
- Run the newly added tests:
npm run test:php -- --filter test_wp_show_heic_upload_error - Confirm the tests pass with the fix:
OK (2 tests, 7 assertions) - Revert the change in
src/wp-includes/media.phpto test the baseline failure:git stash push src/wp-includes/media.php - Re-run the tests:
npm run test:php -- --filter test_wp_show_heic_upload_error - Confirm the expected failure occurs without the patch:
1) Tests_Media::test_wp_show_heic_upload_error_adds_flag_when_not_supportedFailed asserting that an array has the key 'heic_upload_error'. - Restore the change:
git stash pop
Trac 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.
#3
@
5 weeks ago
I measured how the bug's effect differs between the two filters wp_show_heic_upload_error() is hooked on, verifying the scoping points described in PR 12828 on current trunk.
Environment
Trunk 8c3c976c25, WP Core version 7.1-beta4-62899-src, local Docker environment.
Method
I forced the condition using a mu-plugin:
add_filter( 'wp_image_editors', '__return_empty_array', 99 );
I verified the force landed by checking wp_image_editor_supports( array( 'mime_type' => 'image/heic' ) ), which successfully flipped from bool(true) to bool(false). After testing, I removed the mu-plugin and confirmed the control check returned to bool(true).
Finding 1: The plupload_default_settings path
In the page source of post-new.php, _wpPluploadSettings is present, and its defaults contain:
"webp_upload_error":true,"avif_upload_error":true,"heic_upload_error":true
The heic_upload_error flag is present here despite the forced condition because wp_plupload_default_settings() sets it inline (src/wp-includes/media.php:4553) before wp_show_heic_upload_error() runs on that filter (default-filters.php:693). Conclusion: on this path, the bug has no user-visible effect.
Finding 2: The plupload_init path
In the page source of both media-new.php and media-upload.php?type=image, the uploader settings contain:
"webp_upload_error":true,"avif_upload_error":true
The heic_upload_error flag is absent, and _wpPluploadSettings is not present on either page. The only thing setting the HEIC error flag on this path is the function (admin-filters.php:19), so its failure to execute is user-visible here. Conclusion: the user-visible impact of this bug is isolated to media-new.php and the media-upload.php iframe.
@westonruter commented on PR #12828:
5 weeks ago
#4
Closing in favor of the reporter's own PR: https://github.com/WordPress/wordpress-develop/pull/12830
@softglaze commented on PR #12830:
4 weeks ago
#6
Thanks @irozum for checking out the branch and running the red-green proof independently and appreciated.
@westonruter commented on PR #12830:
3 weeks ago
#7
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:
Before | After
The changes here successfully cause the error to show up.
@westonruter commented on PR #12830:
3 weeks ago
#8
🤖 _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.
### Timeline
r48288 (5.5) introduced wp_show_heic_upload_error() and hooked it to two filters — plupload_init in wp-admin/includes/admin-filters.php and plupload_default_settings in wp-includes/default-filters.php. The assignment was unconditional and correct, so both paths carried the flag.
r58849 (6.7.0) then made two changes at once. The callback was wrapped in a wp_image_editor_supports() check and its assignment target was mistyped as $plupload_init — the bug fixed here. In the same changeset, an equivalent assignment was added directly to wp_plupload_default_settings(), immediately *before* the filter it feeds:
// 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 plupload_default_settings hookup has contributed nothing since 6.7.0. Losing the callback's output was therefore invisible on every screen fed by _wpPluploadSettings, and only the plupload_init consumer actually regressed.
### Where the error was still being shown
| Consumer | Settings global | Client-side check | Flag supplied by | 6.7 → now |
|---|---|---|---|---|
wp_enqueue_media() — media library grid, the media modal in the block and classic editors, Customizer media controls | _wpPluploadSettings.defaults | wp-includes/js/plupload/wp-plupload.js | wp_plupload_default_settings(), directly | works |
media_upload_form() — media-new.php, and the legacy media-upload.php iframe via media_upload_type_form() | wpUploaderInit | wp-includes/js/plupload/handlers.js | wp_show_heic_upload_error() via plupload_init, and nothing else | broken |
That split was verified by inspecting the rendered admin markup with HEIC editing unavailable on the server:
| State | Screen | Emitted settings |
|---|---|---|
| Before | media-new.php | heic_upload_error absent from wpUploaderInit
|
| Before | upload.php | "heic_upload_error":true present in _wpPluploadSettings
|
| After | media-new.php | "heic_upload_error":true present in wpUploaderInit
|
A further reason the regression stayed quiet: HEIC is treated differently from WebP and AVIF on the client. WebP and AVIF call up.removeFile() and abort the upload, whereas HEIC only queues a warning and lets the upload proceed. Without the flag, media-new.php produced a silent *success* rather than a visibly failed upload, so there was nothing for a user to report beyond a missing advisory.
### Two possible follow-ups, both out of scope here
media_upload_form()setswebp_upload_errorandavif_upload_errorinline, gated behindwp_prevent_unsupported_mime_type_uploads, but leaves HEIC entirely to the filter and ungated. Setting HEIC inline alongside its siblings would remove both the asymmetry and the single point of failure that this bug depended on.- Given the direct assignment in
wp_plupload_default_settings(),add_filter( 'plupload_default_settings', 'wp_show_heic_upload_error' )is now dead weight. Removing it would be behaviour-neutral, though it does remain a documented extension point that plugins may callremove_filter()against.
@softglaze commented on PR #12830:
3 weeks ago
#9
@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.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
wp_show_heic_upload_error()assigned theheic_upload_errorflag to an undeclared$plupload_initvariable instead of the$plupload_settingsparameter it receives, so the callback returned the settings untouched and the flag was never added.What the problem was:
$plupload_settingscorrectly.plupload_default_settingspath is unaffected in practice, becausewp_plupload_default_settings()already sets the flag inline.plupload_initpath is broken:media_upload_form()sets the WebP and AVIF flags inline but relies on this callback for HEIC, so no error was shown onmedia-new.phpor in themedia-upload.phpiframe when the server could not edit HEIC images.What the fix does:
$plupload_settings, restoring the behaviour the function has documented since 5.5.0.Approach and why:
heic_upload_errorflag is a non-blocking notice inhandlers.js; unlike WebP and AVIF it does not remove the file from the queue, so restoring it warns the user without preventing the upload.wp_plupload_default_settings()was deliberately left alone: it is harmless (the callback assigns the sametrue) and removing it would change behaviour for anyone who has unhooked the callback.Trac ticket: https://core.trac.wordpress.org/ticket/65802
## Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Generate PR.