Skip to content

Instant Search: don't photon-ize SVG images#19927

Merged
bluefuton merged 5 commits into
masterfrom
fix/jetpack-search-photon-svg
May 27, 2021
Merged

Instant Search: don't photon-ize SVG images#19927
bluefuton merged 5 commits into
masterfrom
fix/jetpack-search-photon-svg

Conversation

@bluefuton
Copy link
Copy Markdown
Contributor

@bluefuton bluefuton commented May 25, 2021 •
edited
Loading

Fixes #19925.

Changes proposed in this Pull Request:

SVG images aren't supported by Photon. This PR skips Photon if the image is SVG.

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

  1. Make sure Site Accelerator for images is enabled in /wp-admin/admin.php?page=jetpack#/performance and that Jetpack Search has either the 'Expanded' or 'Product' result format selected.
  2. Add a new page and put an SVG image as the first image
  3. Publish page
  4. Navigate to Jetpack Search and search for this new page
  5. See if SVG image is being displayed correctly

Screen Shot 2021-05-25 at 14 28 05

@bluefuton bluefuton self-assigned this May 25, 2021
@github-actions github-actions Bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label May 25, 2021
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 25, 2021 •
edited
Loading

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ⚠️ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

  • Next scheduled release: June 1, 2021.
  • Scheduled code freeze: May 24, 2021.

@bluefuton bluefuton marked this pull request as ready for review May 25, 2021 02:32
@bluefuton bluefuton requested a review from a team as a code owner May 25, 2021 02:32
@bluefuton bluefuton added [Status] Needs Team Review Obsolete. Use Needs Review instead. and removed [Status] In Progress labels May 25, 2021
kangzj
kangzj reviewed May 25, 2021
View reviewed changes
Comment thread projects/plugins/jetpack/modules/search/instant-search/lib/hooks/use-photon.js Outdated
kangzj
kangzj previously approved these changes May 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@kangzj kangzj left a comment

Choose a reason for hiding this comment

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

Tested working and LGTM. Hooray for the test! 👍

@kangzj kangzj added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Team Review Obsolete. Use Needs Review instead. labels May 25, 2021
@jeherve jeherve added this to the jetpack/9.9 milestone May 25, 2021
leogermani
leogermani reviewed May 25, 2021
View reviewed changes
Comment thread projects/plugins/jetpack/modules/search/instant-search/lib/hooks/use-photon.js Outdated
@leogermani leogermani added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels May 25, 2021
@bluefuton bluefuton added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels May 25, 2021
jeherve
jeherve previously approved these changes May 26, 2021
View reviewed changes
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels May 26, 2021
@bluefuton
Copy link
Copy Markdown
Contributor Author

bluefuton commented May 26, 2021

🚢 Shipped in r226345-wpcom.

@bluefuton bluefuton force-pushed the fix/jetpack-search-photon-svg branch from c6cda21 to 67a5a0a Compare May 26, 2021 23:58
@bluefuton
Copy link
Copy Markdown
Contributor Author

bluefuton commented May 27, 2021

@jeherve would you mind pressing the big merge button for me please? I had to rebase and that seems to have dismissed your review unfortunately.

Also - is there any possibility of having this one cherry-picked across to v9.8? It's affecting a Team 51 site (see p4Kr4c-4gB-p2#comment-18288). Thanks :)

jsnmoon
jsnmoon approved these changes May 27, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@jsnmoon jsnmoon left a comment

Choose a reason for hiding this comment

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

Resident Jetpack approver to the rescue!

@bluefuton
Copy link
Copy Markdown
Contributor Author

bluefuton commented May 27, 2021

Thank you @jsnmoon :)

@bluefuton bluefuton merged commit 7ac4a2f into master May 27, 2021
@bluefuton bluefuton deleted the fix/jetpack-search-photon-svg branch May 27, 2021 02:26
@github-actions github-actions Bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 27, 2021
@anomiex anomiex mentioned this pull request May 27, 2021
Closed
20 tasks
@mdrovdahl
Copy link
Copy Markdown

mdrovdahl commented May 31, 2021

@jeherve just echoing the request from above...any chance this can make tomorrow's release?

jeherve pushed a commit that referenced this pull request Jun 1, 2021
@bluefuton @jeherve
[not verified] Instant Search: don't photon-ize SVG images (#19927)
d900255 
* [not verified] Search: don't photon-ize SVG images

* [not verified] Changelog

* [not verified] Specify allowed file types rather than excluding SVG

* [not verified] Add test

* [not verified] Handle .jpeg extension
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Jun 1, 2021

Sorry, I had missed that one for some reason. I cherry-picked this to jetpack/branch-9.8 in d900255

@jeherve jeherve modified the milestones: jetpack/9.9, jetpack/9.8 Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leogermani leogermani leogermani left review comments

@jeherve jeherve jeherve left review comments

@kangzj kangzj kangzj left review comments

+1 more reviewer

@jsnmoon jsnmoon jsnmoon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bluefuton bluefuton

Labels

[Feature] Search For all things related to Search [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files

Projects

None yet

Milestone

jetpack/9.8

Development

Successfully merging this pull request may close these issues.

Search: SVG images being served using Photon URL despite being unsupported and displaying as broken links

7 participants

@bluefuton @mdrovdahl @jeherve @leogermani @kangzj @jsnmoon @matticbot