Unit tests that confirm that the original image is properly converted to additional mime type #7733

mukeshpanchal27 · 2024-11-06T07:40:08Z

Track ticket: https://core.trac.wordpress.org/ticket/62305

github-actions · 2024-11-06T07:54:40Z

Test using WordPress Playground

The 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

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

mukeshpanchal27 · 2024-11-06T07:59:22Z

The [59317] changes make some changes in attachment metadata.

In 6.7 branch

<prE>Array
(
    [width] => 1920
    [height] => 1080
    [file] => /tmp//33772.jpg
    [filesize] => 176615
    [sizes] => Array
        (
            [medium] => Array
                (
                    [file] => 33772-300x169.webp
                    [width] => 300
                    [height] => 169
                    [mime-type] => image/webp
                    [filesize] => 55988
                )

            [large] => Array
                (
                    [file] => 33772-1024x576.webp
                    [width] => 1024
                    [height] => 576
                    [mime-type] => image/webp
                    [filesize] => 446208
                )

            [thumbnail] => Array
                (
                    [file] => 33772-150x150.webp
                    [width] => 150
                    [height] => 150
                    [mime-type] => image/webp
                    [filesize] => 31268
                )

            [medium_large] => Array
                (
                    [file] => 33772-768x432.webp
                    [width] => 768
                    [height] => 432
                    [mime-type] => image/webp
                    [filesize] => 279734
                )

        )

    [image_meta] => Array
        (
            [aperture] => 8
            [credit] => Photoshop Author
            [camera] => DMC-LX2
            [caption] => Photoshop Description
            [created_timestamp] => 1306315327
            [copyright] => Photoshop Copyrright Notice
            [focal_length] => 6.3
            [iso] => 100
            [shutter_speed] => 0.0025
            [title] => Photoshop Document Ttitle
            [orientation] => 1
            [keywords] => Array
                (
                    [0] => beach
                    [1] => baywatch
                    [2] => LA
                    [3] => sunset
                )
        )
)

In current trunk

<prE>Array
(
    [width] => 1920
    [height] => 1080
    [file] => /tmp/33772-1920x1080.webp
    [filesize] => 865924
    [sizes] => Array
        (
            [medium] => Array
                (
                    [file] => 33772-300x169.webp
                    [width] => 300
                    [height] => 169
                    [mime-type] => image/webp
                    [filesize] => 55988
                )

            [large] => Array
                (
                    [file] => 33772-1024x576.webp
                    [width] => 1024
                    [height] => 576
                    [mime-type] => image/webp
                    [filesize] => 446208
                )

            [thumbnail] => Array
                (
                    [file] => 33772-150x150.webp
                    [width] => 150
                    [height] => 150
                    [mime-type] => image/webp
                    [filesize] => 31268
                )

            [medium_large] => Array
                (
                    [file] => 33772-768x432.webp
                    [width] => 768
                    [height] => 432
                    [mime-type] => image/webp
                    [filesize] => 279734
                )

        )

    [image_meta] => Array
        (
            [aperture] => 8
            [credit] => Photoshop Author
            [camera] => DMC-LX2
            [caption] => Photoshop Description
            [created_timestamp] => 1306315327
            [copyright] => Photoshop Copyrright Notice
            [focal_length] => 6.3
            [iso] => 100
            [shutter_speed] => 0.0025
            [title] => Photoshop Document Ttitle
            [orientation] => 1
            [keywords] => Array
                (
                    [0] => beach
                    [1] => baywatch
                    [2] => LA
                    [3] => sunset
                )
        )
    [original_image] => 33772.jpg
)

Check the file key in array. It use alternate mime type image and add the image sizes in name. Before it was /tmp//33772.jpg now it was /tmp/33772-1920x1080.webp. It cause the issues in plugins that use the file and update the metadata. See WordPress/performance#1634

github-actions · 2024-11-06T15:14:06Z

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props mukesh27, peterwilsoncc, flixos90, azaozz.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

peterwilsoncc · 2024-11-06T22:20:23Z

tests/phpunit/tests/media.php

+		$webp_sizes = wp_generate_attachment_metadata( $attachment_id, $file );
+		remove_filter( 'image_editor_output_format', array( $this, 'image_editor_output_webp' ) );
+
+		$this->assertSame( $file, $webp_sizes['file'], 'The original file name should same.' );


I think this should be original_image, as file is intended to be the full size version of the converted image.

I agree, this comparison doesn't look right.

That said, I'm unclear what this test is covering. Per https://core.trac.wordpress.org/ticket/62305#comment:34, the Core change itself is not a bug. It does change the attachment metadata, but that's because of what it fixes (consistently convert the image if it requires conversion, regardless of whether it needs to also be scaled down or not).

I think this should be original_image, as file is intended to be the full size version of the converted image.
..
It does change the attachment metadata, but that's because of what it fixes

Right. This is the actual bugfix from https://core.trac.wordpress.org/ticket/62305. Before https://core.trac.wordpress.org/changeset/59317 the uploaded image (i.e. the file from the image meta array) was not converted when conversion was required. After the fix it is converted.

This bug has existed for a while but became more visible/severe when converting .heic images as the originally uploaded image cannot be used in most browsers but WordPress was using it anyway.

Thinking this test can perhaps be changed to confirm that the original image is converted (when smaller than the big image threshold).

Got it. Thanks for clarification 🙌

My question is why the full size image generated with 33772-1920x1080.webp not 33772.webp? It's just observation not a concern. We should return 33772.webp for full size image.

When i upload https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/data/images/33772.jpg image it generates below images:

33772-150x150.jpg

33772-300x169.jpg

33772-768x432.jpg

33772-1024x576.jpg

33772-1536x864.jpg

33772.jpg

But when i convert JPEG to WEBP it generates below images:

33772-150x150.webp

33772-300x169.webp

33772-768x432.webp

33772-1024x576.webp

33772-1536x864.webp

33772-1920x1080.webp

33772.jpg

@peterwilsoncc Something like -converted would be okay for me, though I would prefer to consider changing that internal logic - as in: If the file extension is different from the input image, it should be possible to keep using the same name (except the extension of course).

I agree changing the logic would be optimal but I think other falsey values can end up being used, eg ::get_suffix() can return false. So ::generate_filename( ::get_suffix() ) requires the loose check (I briefly considered using false to indicate no suffix)

Using -converted is the smaller fix but WP would be stuck with it forever more, so I'm tempted to put in the snowflake code for now to allow for any niceties later. Either way I am happier than using the dimensions.

Yes, for a moment I was considering to change ::generate_filename() to accept an empty string for no suffix. Didn't because of RC, but will open a ticket for 6.8.

Hm, another option here may be to include the old file extension as a suffix? For example: converting image.heic to JPEG would produce image-heic.jpg?

Hm, another option here may be to include the old file extension as a suffix? For example: converting image.heic to JPEG would produce image-heic.jpg?

This is what we did in Modern Image Formats.

tests/phpunit/tests/media.php

Co-authored-by: Peter Wilson <[email protected]>

tests/phpunit/tests/media.php

peterwilsoncc · 2024-11-08T04:13:36Z

I've incorproated these tests in to #7748 with a few modifications to use HEIC testing to allow it to rely on the default core features.

mukeshpanchal27 · 2024-11-08T06:29:19Z

There was 1 failure:

1) Tests_Media::test_image_converted_to_other_format_has_correct_filename with data set "do not scale image" (false)
The file name is expected to be 33772.webp
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'33772.webp'
+'33772-1920x1080.webp'

mukeshpanchal27 · 2024-11-08T07:06:56Z

@peterwilsoncc In 76a5c39 i incorporate changes from PR #7748 but now it return original image instead WebP mime type.

There was 1 failure:

1) Tests_Media::test_image_converted_to_other_format_has_correct_filename with data set "do not scale image" (false)
The file extension is expected to change.
Failed asserting that '/tmp/33772.jpg' ends not with ".jpg".

peterwilsoncc · 2024-11-08T21:41:47Z

@mukeshpanchal27 That looks like it was due to the regex error @felixarntz picked up in #7748 (comment)

mukeshpanchal27 · 2024-11-10T16:17:46Z

@peterwilsoncc Do we need these PR or can we close it?

peterwilsoncc · 2024-11-10T21:43:08Z

@mukeshpanchal27 This was included in the follow up PR.

Check attachment meta changes

ea884b5

mukeshpanchal27 requested review from peterwilsoncc, azaozz, joemcgill, adamsilverstein and felixarntz November 6, 2024 08:43

mukeshpanchal27 mentioned this pull request Nov 6, 2024

Fix bug that would prevent uploaded images from being converted to the intended output format when having fallback formats enabled WordPress/performance#1635

Merged

mukeshpanchal27 requested a review from SergeyBiryukov November 6, 2024 12:04

mukeshpanchal27 marked this pull request as ready for review November 6, 2024 15:13

Incorporate additional changes

5e4c9e8

peterwilsoncc requested changes Nov 6, 2024

View reviewed changes

mukeshpanchal27 and others added 3 commits November 7, 2024 10:05

Merge branch 'WordPress:trunk' into test/attachment-metadata

de53b22

Apply suggestions from code review

0fba5c6

Co-authored-by: Peter Wilson <[email protected]>

Update test that cover new changes

7c0e5e7

mukeshpanchal27 requested a review from peterwilsoncc November 7, 2024 10:41

mukeshpanchal27 changed the title ~~Trunk branch: Test attachment metadata~~ Unit tests that confirm that the original image is properly converted to additional mime type Nov 7, 2024

felixarntz reviewed Nov 7, 2024

View reviewed changes

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

Update test

6106ef3

peterwilsoncc reviewed Nov 7, 2024

View reviewed changes

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

peterwilsoncc mentioned this pull request Nov 8, 2024

Remove suffix from converted full size images. #7748

Closed

Update function name and asserts

ad0a3ad

Incorporate changes from PR 7748

76a5c39

Update regex

c7a4c64

mukeshpanchal27 closed this Nov 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests that confirm that the original image is properly converted to additional mime type #7733

Unit tests that confirm that the original image is properly converted to additional mime type #7733

mukeshpanchal27 commented Nov 6, 2024 •

edited

Loading

github-actions bot commented Nov 6, 2024

mukeshpanchal27 commented Nov 6, 2024

github-actions bot commented Nov 6, 2024 •

edited

Loading

peterwilsoncc Nov 6, 2024

felixarntz Nov 6, 2024

azaozz Nov 6, 2024

mukeshpanchal27 Nov 7, 2024

mukeshpanchal27 Nov 7, 2024 •

edited

Loading

felixarntz Nov 7, 2024

peterwilsoncc Nov 7, 2024

azaozz Nov 8, 2024 •

edited

Loading

azaozz Nov 8, 2024

mukeshpanchal27 Nov 8, 2024

peterwilsoncc commented Nov 8, 2024

mukeshpanchal27 commented Nov 8, 2024

mukeshpanchal27 commented Nov 8, 2024

peterwilsoncc commented Nov 8, 2024

mukeshpanchal27 commented Nov 10, 2024

peterwilsoncc commented Nov 10, 2024

Unit tests that confirm that the original image is properly converted to additional mime type #7733

Unit tests that confirm that the original image is properly converted to additional mime type #7733

Conversation

mukeshpanchal27 commented Nov 6, 2024 • edited Loading

github-actions bot commented Nov 6, 2024

Test using WordPress Playground

Some things to be aware of

mukeshpanchal27 commented Nov 6, 2024

In 6.7 branch

In current trunk

github-actions bot commented Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukeshpanchal27 Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwilsoncc commented Nov 8, 2024

mukeshpanchal27 commented Nov 8, 2024

mukeshpanchal27 commented Nov 8, 2024

peterwilsoncc commented Nov 8, 2024

mukeshpanchal27 commented Nov 10, 2024

peterwilsoncc commented Nov 10, 2024

mukeshpanchal27 commented Nov 6, 2024 •

edited

Loading

github-actions bot commented Nov 6, 2024 •

edited

Loading

mukeshpanchal27 Nov 7, 2024 •

edited

Loading

azaozz Nov 8, 2024 •

edited

Loading