Make WordPress Core

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

wp_show_heic_upload_error() assigned the heic_upload_error flag to an undeclared $plupload_init variable instead of the $plupload_settings parameter it receives, so the callback returned the settings untouched and the flag was never added.

What the problem was:

  • The typo was introduced in [58849] (#53645, shipped in 6.7.0); before that changeset the function assigned to $plupload_settings correctly.
  • The plupload_default_settings path is unaffected in practice, because wp_plupload_default_settings() already sets the flag inline.
  • The plupload_init path is broken: media_upload_form() sets the WebP and AVIF flags inline but relies on this callback for HEIC, so no error was shown on media-new.php or in the media-upload.php iframe when the server could not edit HEIC images.

What the fix does:

  • Assigns to $plupload_settings, restoring the behaviour the function has documented since 5.5.0.
  • Adds a regression test for the callback.

Approach and why:

  • One-character-scope fix on the assignment target — nothing else in the function or its two callers needs to change.
  • The heic_upload_error flag is a non-blocking notice in handlers.js; unlike WebP and AVIF it does not remove the file from the queue, so restoring it warns the user without preventing the upload.
  • The redundant inline HEIC check in wp_plupload_default_settings() was deliberately left alone: it is harmless (the callback assigns the same true) 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.

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.

  1. Run the newly added tests: npm run test:php -- --filter test_wp_show_heic_upload_error
  2. Confirm the tests pass with the fix: OK (2 tests, 7 assertions)
  3. Revert the change in src/wp-includes/media.php to test the baseline failure: git stash push src/wp-includes/media.php
  4. Re-run the tests: npm run test:php -- --filter test_wp_show_heic_upload_error
  5. Confirm the expected failure occurs without the patch: 1) Tests_Media::test_wp_show_heic_upload_error_adds_flag_when_not_supported Failed asserting that an array has the key 'heic_upload_error'.
  6. 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 @softglaze
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

#5 @westonruter
5 weeks ago

  • Milestone Awaiting Review7.2
  • Owner set to westonruter
  • Status newreviewing

@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

--

https://github.com/user-attachments/assets/221281e4-0fed-463b-a930-ebafcf5b880a | https://github.com/user-attachments/assets/8122e1fb-05b3-46e7-8932-6ea9d061fbe0

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

  1. media_upload_form() sets webp_upload_error and avif_upload_error inline, gated behind wp_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.
  2. 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 call remove_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.

#10 @westonruter
3 weeks ago

  • Resolutionfixed
  • Status reviewingclosed

In 63338:

Media: Restore the HEIC upload error on the media upload form.

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 https://github.com/WordPress/wordpress-develop/pull/12830.
Follow-up to r48288, r58849.

Props softglaze, westonruter, khokansardar, irozum.
See #53645.
Fixes #65802.

Note: See TracTickets for help on using tickets.