Skip to content

Fixes palette-based PNG uploads failing original full-size AVIF/WebP conversion under GD #2024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions plugins/webp-uploads/hooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -803,3 +803,62 @@
return $allowed_sizes;
}
add_filter( 'webp_uploads_image_sizes_with_additional_mime_type_support', 'webp_uploads_enable_additional_mime_type_support_for_all_sizes' );

/**
* Converts palette PNG images to true color PNG images.
*
* GD cannot convert palette-based PNG to WebP/AVIF formats, causing conversion failures.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this fix belongs in core itself sice we inherently support this type of conversion with a simple filter. If we don't fix automatically, at the very least, we should inform the user why the upload failed and how to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I located the Trac ticket: #59339 "Conversion to webp causes fatal error when original image is a palette image (as opposed to truecolor)"

Should we post an update there and start the discussion on where to go next?

* This function detects and converts palette PNG to truecolor during upload.
*
* @since n.e.x.t
*
* @param array<string, mixed> $file The uploaded file data.
* @return array<string, mixed> The modified file data.
*/
function webp_uploads_convert_palette_png_to_truecolor( array $file ): array {
if ( ! isset( $file['tmp_name'], $file['name'] ) ) {
return $file;

Check warning on line 820 in plugins/webp-uploads/hooks.php

View check run for this annotation

Codecov / codecov/patch

plugins/webp-uploads/hooks.php#L820

Added line #L820 was not covered by tests
}

if ( 'png' !== strtolower( pathinfo( $file['name'], PATHINFO_EXTENSION ) ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this look at the attachment metadata instead to see if the MIME type is image/png?

Copy link
Contributor Author

@b1ink0 b1ink0 Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can just check if $file['type'] is image/png.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7443e68.

return $file;
}

$editor = wp_get_image_editor( $file['tmp_name'] );

if ( is_wp_error( $editor ) || ! $editor instanceof WP_Image_Editor_GD ) {
return $file;

Check warning on line 830 in plugins/webp-uploads/hooks.php

View check run for this annotation

Codecov / codecov/patch

plugins/webp-uploads/hooks.php#L830

Added line #L830 was not covered by tests
}

// Check if required GD functions exist.
if (
! function_exists( 'imagecreatefrompng' ) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to defensive programming; however all of these functions have been around since PHP4/5 as as far as I know should be available to all WordPress installs with GD. So maybe we can remove this check?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would address the code coverage issue as well, if it is impossible to create a scenario where any of these functions don't exist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, but actually will these functions be available if GD is not installed? If PHP is not compiled with --with-gd then I don't think they will.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If PHP is not compiled with GD, then these functions will not be available. I compiled PHP with and without GD and tested it; the version without GD did not have these functions.
Screenshot 2025-06-10 at 8 33 05 PM

! function_exists( 'imageistruecolor' ) ||
! function_exists( 'imagealphablending' ) ||
! function_exists( 'imagesavealpha' ) ||
! function_exists( 'imagepalettetotruecolor' ) ||
! function_exists( 'imagepng' ) ||
! function_exists( 'imagedestroy' )
) {
return $file;

Check warning on line 843 in plugins/webp-uploads/hooks.php

View check run for this annotation

Codecov / codecov/patch

plugins/webp-uploads/hooks.php#L843

Added line #L843 was not covered by tests
}

$image = imagecreatefrompng( $file['tmp_name'] );
if ( false === $image || imageistruecolor( $image ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this check. If $image is false, the conversion failed. In that case, don't we want to continue to the processing below?

Copy link
Contributor Author

@b1ink0 b1ink0 Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When imagecreatefrompng() returns false, it means the creation of the PNG failed. In that case, we can't convert it to truecolor because we need a valid GDImage resource for imagepalettetotruecolor() to work properly. Returning early in this scenario is the best choice since there's no valid image resource to operate on.

I also combined two different checks in that condition, which I realize makes the logic less clear. Separate those out in 7f757c7 to improve readability.

return $file;
}

// Preserve transparency.
imagealphablending( $image, false );
imagesavealpha( $image, true );

// Convert the palette to truecolor.
if ( imagepalettetotruecolor( $image ) ) {
// Overwrite the upload with the new truecolor PNG.
imagepng( $image, $file['tmp_name'] );
}
imagedestroy( $image );

return $file;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set $file['name'] to the new file name before returning $file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're asking whether the conversion has changed the name of the file, it has not - we simply overwrote the same file with the converted version.

However, if you're suggesting that users should see a different filename when an image is converted to truecolor, we could add a suffix like -truecolor to the filename. But I'm not sure whether we should do that, since this conversion is meant to be a transparent internal optimization rather than a user-visible change.

}
add_filter( 'wp_handle_upload_prefilter', 'webp_uploads_convert_palette_png_to_truecolor' );

Check warning on line 864 in plugins/webp-uploads/hooks.php

View check run for this annotation

Codecov / codecov/patch

plugins/webp-uploads/hooks.php#L864

Added line #L864 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're already using this in the test, but wouldn't it make sense to also add the same function as a hook callback to wp_handle_sideload_prefilter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it makes sense. At first, I thought wp_handle_sideload and media_handle_sideload, which trigger the wp_handle_sideload_prefilter, were not used in core, but after some digging, I found that core does trigger this filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 8ad675c

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
126 changes: 126 additions & 0 deletions plugins/webp-uploads/tests/test-load.php
Original file line number Diff line number Diff line change
Expand Up @@ -1116,4 +1116,130 @@ public function test_that_it_should_convert_webp_to_avif_on_upload(): void {
}
wp_delete_attachment( $attachment_id );
}

/**
* Tests converting a palette PNG to a truecolor PNG.
*
* @dataProvider data_to_test_webp_uploads_convert_palette_png_to_truecolor
*
* @covers ::webp_uploads_convert_palette_png_to_truecolor
*
* @param string|null $image_path The path to the image file to test.
* @param bool $expect_changed Whether the png should be converted to truecolor.
* @param callable|null $set_up Optional setup function to run before the test.
* @param callable|null $tear_down Optional teardown function to run after the test.
*/
public function test_webp_uploads_convert_palette_png_to_truecolor( ?string $image_path, bool $expect_changed, ?callable $set_up, ?callable $tear_down ): void {
if ( null !== $set_up ) {
$set_up();
}

// Temp file will be copied and unlinked by WordPress core during sideload processing.
$tmp_file = wp_tempnam();
copy( $image_path, $tmp_file );
$file = array(
'name' => basename( $image_path ),
'tmp_name' => $tmp_file,
'type' => wp_check_filetype( $image_path )['type'],
'size' => filesize( $tmp_file ),
'error' => UPLOAD_ERR_OK,
);

// Store the original file hash to compare later.
$original_file_hash = isset( $file['tmp_name'] ) ? md5_file( $file['tmp_name'] ) : '';

// Need to use wp_handle_sideload_prefilter for simulated upload as wp_handle_upload_prefilter is not called in this case.
add_filter( 'wp_handle_sideload_prefilter', 'webp_uploads_convert_palette_png_to_truecolor' );
$attachment_id = media_handle_sideload( $file );
remove_filter( 'wp_handle_sideload_prefilter', 'webp_uploads_convert_palette_png_to_truecolor' );

try {
$this->assertIsNumeric( $attachment_id );

// For getting a original image path for computation of the file hash.
$meta = wp_get_attachment_metadata( $attachment_id );
$upload_dir = wp_get_upload_dir();
$path = null;
if ( isset( $meta['original_image'], $meta['file'] ) ) {
$path = path_join(
$upload_dir['basedir'],
dirname( $meta['file'] ) . '/' . $meta['original_image']
);
}
$this->assertNotNull( $path );
$this->assertFileExists( $path );

// Hash will be modified if the image was converted to truecolor.
$modified_file_hash = md5_file( $path );

if ( ! $expect_changed ) {
$this->assertSame( $original_file_hash, $modified_file_hash );
} else {
$this->assertNotSame( $original_file_hash, $modified_file_hash );
$img = imagecreatefrompng( $path );
$this->assertTrue( imageistruecolor( $img ) );
imagedestroy( $img );

// Make sure the image converted to modern image format is not 0 bytes.
$modern_image_format_path = get_attached_file( $attachment_id );
$this->assertNotFalse( $modern_image_format_path );
$this->assertFileExists( $modern_image_format_path );
$this->assertGreaterThan( 0, (int) filesize( $modern_image_format_path ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add an assertion that the new image filesize isn't larger than the original image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 56e8dac.

}
} finally {
if ( null !== $tear_down ) {
$tear_down();
}
wp_delete_attachment( $attachment_id );
}
}

/**
* Data provider for `test_webp_uploads_convert_palette_png_to_truecolor`.
*
* @return array<string, mixed> Returns an array of test cases.
*/
public function data_to_test_webp_uploads_convert_palette_png_to_truecolor(): array {
$non_palette_png = TESTS_PLUGIN_DIR . '/tests/data/images/dice.png';
$palette_png = TESTS_PLUGIN_DIR . '/tests/data/images/dice-palette.png';
$test_jpg = TESTS_PLUGIN_DIR . '/tests/data/images/leaves.jpg';

return array(
'wrong_extension' => array(
'image_path' => $test_jpg,
'expected_changed' => false,
'set_up' => null,
'tear_down' => null,
),
'non_palette_png' => array(
'image_path' => $non_palette_png,
'expected_changed' => false,
'set_up' => function (): void {
add_filter( 'wp_image_editors', array( $this, 'use_gd_image_editor' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can the test just add this filter always, without needing a "set_up" attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e646941.

},
'tear_down' => function (): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tear_down should not be required as all hooks should be reset between test runs as far as I know. Can you try removing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can remove this. I had added them because I was also testing using the ImageMagick editor, and since filters don't get removed between iterations of the data provider, I had to add setup and teardown.

But I removed the test for ImageMagick as I noticed ImageMagick wasn't working correctly when I ran the tests. When I tried to log Imagick::queryFormats(), it returned an empty array. I'm not sure why - I tried running php -r "print_r(Imagick::queryFormats());" in the wp-env created tests-wordpress container and it gave an array with all supported formats, but when I ran the same command in tests-cli it gave an empty array. I'm not sure - do tests run in the tests-cli container and not the tests-wordpress container?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e646941.

remove_filter( 'wp_image_editors', array( $this, 'use_gd_image_editor' ) );
},
),
'palette_png' => array(
'image_path' => $palette_png,
'expected_changed' => true,
'set_up' => function (): void {
add_filter( 'wp_image_editors', array( $this, 'use_gd_image_editor' ) );
},
'tear_down' => function (): void {
remove_filter( 'wp_image_editors', array( $this, 'use_gd_image_editor' ) );
},
),
);
}

/**
* Use GD image editor.
*
* @return array<string>
*/
public function use_gd_image_editor(): array {
return array( 'WP_Image_Editor_GD' );
}
}
Loading