Skip to content
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

Fix/1144 webp support #1269

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Fix/1144 webp support #1269

wants to merge 2 commits into from

Conversation

benlk
Copy link

@benlk benlk commented Sep 8, 2024

Description of the Change

This PR:

  • adds WebP support to Distributor, for parity with WordPress 5.8
  • adds HEIC support to Distributor, for parity with WordPress 6.5
  • adds AVIF support to Distributor, for parity with WordPress 6.7
  • adds tests:
    • WebP
    • HEIC
    • AVIF

Closes #810

How to test the Change

Upload test AVIF, HEIC, and WebP files to a Distributor source site. Ensure taht they are synchronized to a downstream Distributor site.

Changelog Entry

Added - Support for WebP, paralleling WordPress 5.8
Added - Support for HEIC, paralleling WordPress 6.5
Added - Support for AVIF, paralleling WordPress 6.7

Credits

Props @username, @username2, ...

Checklist:

@benlk benlk added the needs:tests This requires tests. label Sep 8, 2024
@benlk benlk self-assigned this Sep 8, 2024
@github-actions github-actions bot added this to the 2.1.0 milestone Sep 8, 2024
@benlk benlk marked this pull request as ready for review September 8, 2024 22:13
@benlk benlk requested a review from a team as a code owner September 8, 2024 22:13
@benlk benlk requested review from iamdharmesh and removed request for a team September 8, 2024 22:13
@github-actions github-actions bot added the needs:code-review This requires code review. label Sep 8, 2024
@benlk benlk marked this pull request as draft September 8, 2024 22:15
@github-actions github-actions bot removed the needs:code-review This requires code review. label Sep 8, 2024
@benlk
Copy link
Author

benlk commented Sep 8, 2024

Before this PR is merged, we should:

  • add tests covering support for WebP, HEIC, and AVIF — There are specific image tests in https://github.com/10up/distributor/blob/develop/tests/php/DistributorPostTest.php but it's not clear to me where the example images come from. The domain names given are fictional or reserved, and the images referenced are not included in this plugin's filesystem. Can support be added simply with mock data in the test case, or do the files need to also exist somewhere?
  • add a test covering a fictional image type, to ensure that we reject disallowed media types
  • discuss how to handle differences in support between Distributor instances running on different versions of WordPress — Should the receiver communicate to the sender which MIME types and other supported things are allowed? Or is it entirely upon the receiving Distributor instance to filter out that which it does not support?

@peterwilsoncc
Copy link
Collaborator

Can support be added simply with mock data in the test case, or do the files need to also exist somewhere?

Yes, tests can be added by mocking the image names. As the tests use https://github.com/10up/wp_mock/ the files don't need to exist in the repo.

This should work for the fictional file type too.

Should the receiver communicate to the sender which MIME types and other supported things are allowed

As some of the supported media types depend on the version of underlying libraries installed (GD, ImageMagick) then I think it would be good for the pushing instance to know what the receiving instance supports.

There's a few REST API endpoints that Distributor registers that the system could make use of. I think we'd only want to make the data available if the user is logged in to the endpoint but we'd need to check for the data during a push, which I don't think is something that's done at the moment.

Maybe add the data to check_post_types_permissions for logged in users and if the data isn't recorded in the meta data prior to pushing, then we can recheck the remote end point.

@benlk
Copy link
Author

benlk commented Oct 4, 2024

Flagging that I'm not sure when I'm going to have time to work on this PR again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:tests This requires tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WEBP image formats
2 participants