Introduce webp_uploads_get_content_image_mimes() to get content image MIME replacement rules#420
Conversation
eugene-manuilov
added
[Type] Enhancement
eugene-manuilov
requested review from
adamsilverstein,
felixarntz and
mitogh
as code owners
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
I have added some feedback regarding the doc block and code changes. PR 418 only allows a smaller version of an additional mime type image when this PR is merged. After that, this PR needs to be updated according to it.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@eugene-manuilov I don't think we should address this in the plugin, as it isn't really an appropriate solution, since in core we should do this primarily based on a parameter - and not just replace the images but use the right images from the start. See #414 (comment) for more details.
With that said, you already put some work into this and I think breaking out the filter into a separate function and moving the unnecessary function call below the foreach loop are nice little improvements. So I'd say we can still merge this if we remove the new integrations around wp_get_image_attachment_src.
modules/images/webp-uploads/load.php
Outdated
|
|
||
| return $image; | ||
| } | ||
| add_filter( 'wp_get_attachment_image_src', 'webp_uploads_update_attachment_image_src', 10, 3 ); |
There was a problem hiding this comment.
I think this is out of scope for the plugin, as functions controlled by developers should leave that control to developers. See #414 (comment)
There was a problem hiding this comment.
Sounds good to me. Reverted it.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
Thanks @eugene-manuilov! Will remove this PR from the issue since it doesn't really address it anymore, but rather is a nice little code enhancement.
felixarntz
removed
the
no milestone
|
@eugene-manuilov There are some merge conflicts here, can you look into those? This is not critical for the release since it's just a little code enhancement, so I'm going to move it to the next milestone. |
@felixarntz, merge conflicts are fixed. |
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
@eugene-manuilov Just needs to correct the merge conflict.
felixarntz
changed the title
webp_uploads_get_content_image_mimes() to get content image MIME replacement rules
4 participants
Summary
Originally intended as a fix to #414, but that issue isn't feasible to fix in the plugin, so now it's just a little API enhancement.
Relevant technical choices
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.