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

Revert changes done in #216 #238

Merged
merged 3 commits into from
Dec 5, 2024
Merged

Revert changes done in #216 #238

merged 3 commits into from
Dec 5, 2024

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Dec 3, 2024

Description of the Change

As reported in #237, the changes made in #216 are causing issues for anyone using the wp_get_attachment_image function to output SVGs. As described in that Issue, we need to be smarter in how we determine the height and width attributes to use instead of just relying on the dimensions for each registered size (since SVGs don't get cropped the same as normal images by WordPress).

This PR reverts the changes introduced in #216 and also adds some additional E2E tests to help us catch these sorts of issues in the future.

Note there is additional work we could look to do here, both fixing what we were attempting to do in #216 and being smarter with how we determine the SVG sizes, as described in this comment.

Closes #237

How to test the Change

  1. Ensure general functionality of the plugin still works as expected (you can upload svgs, you can add those into content, etc)
  2. Also modify your theme to directly output svgs using wp_get_attachment_image and ensure that no matter what image size you pass in, the full SVG dimensions are used instead (which is previous behavior)

Changelog Entry

Fixed - Revert changes made to how we determine custom dimensions for svgs

Credits

Props @dkotter, @martinpl, @subfighter3, @smerriman, @gigatyrant

Checklist:

@dkotter dkotter self-assigned this Dec 3, 2024
@dkotter dkotter requested a review from jeffpaul as a code owner December 3, 2024 18:06
@dkotter dkotter added this to the 2.3.1 milestone Dec 3, 2024
@github-actions github-actions bot added the needs:code-review This requires code review. label Dec 3, 2024
jeffpaul
jeffpaul previously approved these changes Dec 3, 2024
@dkotter
Copy link
Collaborator Author

dkotter commented Dec 3, 2024

This reverts the changes made in #216 and brings back the functionality for wp_get_attachment_image. In testing with get_image_tag though, that doesn't seem to be working as expected anymore, though seems that's a WordPress core issue and not something we changed (I tested Safe SVG back to 2.0.0 and got the same results).

Basically that function is using the dimensions of the image size passed in instead of the SVG dimensions (basically functions the way wp_get_attachment_image did after the changes in #216, which we're reverting here). Don't need to hold up this PR for that but we should open a new issue to investigate what needs to change to ensure get_image_tag outputs SVGs as expected. Seems the problem lies in our code that looks for height and width set to 1 and replaces those with the custom size. WordPress isn't setting those to 1 anymore so that replacement doesn't work.

I've added E2E tests in this PR for both wp_get_attachment_image and get_image_tag but the get_image_tag ones, while they pass, aren't getting the height and width attributes I'd expect, so once that issue gets fixed, those tests should fail (which is good in this case) and we can fix the tests at that point.

Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good to me and it tests well. Thanks @dkotter

@dkotter dkotter merged commit 13777ce into develop Dec 5, 2024
14 checks passed
@dkotter dkotter deleted the fix/revert-216 branch December 5, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaking behavior in inheriting width, height attributes from 2.3.0
3 participants