-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add image replacement feature #230
Add image replacement feature #230
Conversation
@rickalee looking to you / your team for review / approval here |
@hugosolar Approved. Looks great. Any considerations with Cache and configuration we need to document for other users of public plugin? |
@rickalee I've addressed cache issue by setting the blob properties cache to 30 segs |
@jeffpaul the e2e error seems to come directly from
have you seen this before? |
@hugosolar Yes, updating |
$filename_path = $source_sizes['meta_data']['file']; | ||
$file_path = dirname( $filename_path ); | ||
|
||
$target_filename = $target_sizes['meta_data']['file']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we need to be more defensive here? For both $source_sizes
and $target_sizes
, will these always be arrays and will the keys we want always exist? Or do we need to check for those before proceeding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkotter since this array is made above, it stores WordPress default _wp_attachment_metadata
so it's always expected to exist if it's a valid attachment. Although I agree with the extra check because we never knows if we're about to handle an invalid attachment or a similar situation.
I've added some extra checks to bail early
Note I'm holding for @rickalee's review before merging here |
@jeffpaul @hugosolar @dkotter Approved after a few updates by Hugo. |
@hugosolar mind checking on the e2e failures before this gets merged in? |
@jeffpaul as far as I can see, it's failing because I'm pushing this PR from my fork |
Description of the Change
Extending our previous feature to replace PDFs in #220
Now I'm adding the feature to replace images.
The criteria to replace images is based on the source image, we're using it as a reference to handle metadata
The process is to loop over the registered sizes for the image and find the image with the nearest size from the replacement and replace the correspondent image size.
This approach seems to be the one used by known plugins like Enable Media replace
The workflow we're following is
_wp_attached_file
and copy those over to replace the location finding the closest size possiblehttps://drive.google.com/file/d/1vPuk_cYUdWrlGBY3bUkjxWTpZeYr-BTG/view
This plugin also fixes #184 ensuring we're processing an array with array_flip otherwise returning a WP_Error object
closes #182 #130
How to test the Change
Changelog Entry
Credits
Props @hugosolar
Checklist: