-
Notifications
You must be signed in to change notification settings - Fork 159
Fix/1144 webp support #1269
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
base: develop
Are you sure you want to change the base?
Fix/1144 webp support #1269
Conversation
|
Before this PR is merged, we should:
|
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.
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 |
|
Flagging that I'm not sure when I'm going to have time to work on this PR again. |
|
@peterwilsoncc I added a few test cases for WebP image types in b83212b, Can you let me know if that's the right direction to proceed with? If so, I can proceed with HEIC/AVIF image types test cases. Also, I tested HEIC & AVIF image types, with different versions, and every time it passed to the receiver site whether the receiver site supports that type or not. I'm not sure what I'm doing wrong or WP is acting weird for me. 🤷🏻♂️ Can you try as well? Just in case I'm missing anything! |
Yes, these tests look good to me. Thank you!
I'm unsure what you mean here:
We can limit the files in $wp_allowed_image_mimes = array_filter( get_allowed_mime_types(), function( $mime ) {
return str_starts_with( $mime, 'image/' );
} );
$wp_allowed_image_extensions = array();
foreach ( $wp_allowed_image_mimes as $ext => $mime ) {
$wp_allowed_image_extensions = array_merge( $wp_allowed_image_extensions, explode( '|', $ext ) );
}
$dt_allowed_image_extensions = array_intersect( $wp_allowed_image_extensions, array(
'jpg',
'jpeg',
'jpe',
'gif',
'png',
'webp',
'heic',
'avif',
) ); |
What II mean by this is, I tried the latest version of WordPress with uploading HEIC & AVIF image types, and then distribute those media files to the receiver site where neither of these image types are supported, but still that receiver site was showing these images just fine. |
|
@sanketio Got you. WordPress converts HEIC images to jpegs on uploads, so that might be why it was working. AVIF images aren't converted so I'm not sure what was happening in that instance. It might have been a reference to the image on the original site, depending on when you last merged in the |
@peterwilsoncc I tested this again with fresh install, merging latest code from develop branch to the feature branch. But I think I found the reason. I was testing both the sites with the feature branch, as we are already supporting AVIF in this branch, it will work fine, but if we change the receiver site branch to develop, then it is referencing to the source site image. 🤦🏻♂️ Now, as we will be allowing AVIF image type when this branch is merged, that shouldn't be a problem, so I'm thinking this is fine 🤔 |
Description of the Change
This PR:
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
Credits
Props @username, @username2, ...Checklist: