Fix | Slow load times due to svg_dimensions #154
Conversation
|
@sksaju the PR is in draft, is this ready for review? |
Not yet. I just need to fix the unit test issue, which I can do early next week :) |
|
|
||
| /** | ||
| * Calculate SVG dimensions and orientation. | ||
| * | ||
| * @param array $dimensions An array containing width, height, and orientation. | ||
| * @param string $svg The file path to the SVG. | ||
| * | ||
| * @return array An array of SVG dimensions and orientation. | ||
| */ | ||
| return apply_filters( 'safe_svg_dimensions', $dimensions, $svg ); |
There was a problem hiding this comment.
@sksaju @10up/open-source-practice I like this filter, but what are your thoughts on introducing a safe_svg_pre_dimensions filter also?
My thinking is, if someone wants to override this value, forcing them through the overhead of loading and sizing the SVG seems a little odd.
Something like the following at the top of this method to allow the short-circuit might be useful:
/**
* Calculate SVG dimensions and orientation.
*
* This filter allows you to implement your own sizing. By returning a non-false
* value, it will short-circuit this function and return your set value.
*
* @param array $attachment_id The attachment ID of the SVG being processed.
*
* @return array|false An array of SVG dimensions and orientation or false.
*/
$short_circuit = apply_filters( 'safe_svg_pre_dimensions', '__return_false' );
if ( false !== $short_circuit ) {
return $short_circuit;
}There was a problem hiding this comment.
Thanks @darylldoyle I've updated the PR, please take a look again and let me know your feedback. could you also let me know how to fix this https://github.com/10up/safe-svg/actions/runs/7004501147/job/19052370329?pr=154 UnitTest issue? Thanks
|
@darylldoyle back to you for review |
There was a problem hiding this comment.
This looks great @sksaju!
The PHPUnit failure looks like it's because you haven't mocked wp_get_attachment_metadata() within the test. I didn't think it would be needed, but worth a try. See: https://github.com/10up/safe-svg/blob/develop/tests/unit/test-safe-svg.php#L191-L209
Once those tests are passing I'm good with merging this.
CC @jeffpaul
| /** | ||
| * Calculate SVG dimensions and orientation. | ||
| * | ||
| * This filter allows you to implement your own sizing. By returning a non-false | ||
| * value, it will short-circuit this function and return your set value. | ||
| * | ||
| * @param boolean Default value of the filter. | ||
| * @param integer $attachment_id The attachment ID of the SVG being processed. | ||
| * | ||
| * @return array|false An array of SVG dimensions and orientation or false. | ||
| */ | ||
| $short_circuit = apply_filters( 'safe_svg_pre_dimensions', false, $attachment_id ); | ||
|
|
||
| if ( false !== $short_circuit ) { | ||
| return $short_circuit; | ||
| } |
There was a problem hiding this comment.
praise (non-blocking): Nice work here!
Thanks @darylldoyle I already fixed the E@E issue here d0b5b8a :) |
| $width = 0; | ||
| $height = 0; | ||
|
|
||
| if ( $svg && ! empty( $metadata['width'] ) && empty( $metadata['height'] ) ) { |
There was a problem hiding this comment.
@sksaju Sorry, I know this is late and this PR is already merged but in looking at this prior to prepping the next release, on the surface it seems like this conditional is not correct. It checks $svg isn't null and then checks if the width isn't empty and the height is empty. Unless I'm thinking of this wrong, shouldn't it be checking if both width and height aren't empty, so like this:
if ( $svg && ! empty( $metadata['width'] ) && ! empty( $metadata['height'] ) ) {
Description of the Change
We have refactored the
svg_dimensionsfunction to calculate dimensions using the existing attachment meta.Closes #75 and #151
How to test the Change
Changelog Entry
Credits
Props @username, @username2, ...
Checklist: