Comments: Skip Photon resizing for Facebook avatars#6872
Conversation
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Instead of creating a new function, I wonder if we could add the graph.facebook.com domain to the list of banned domains in Photon, here:
https://github.com/Automattic/jetpack/blob/master/functions.photon.php#L303
That would solve the issue everywhere where such URLs are being used, not just in comments.
That said, it would be interesting to find out why the domain was whitelisted in the first place:
https://github.com/Automattic/jetpack/blame/master/functions.photon.php#L275
It seems it was done here:
https://code.trac.wordpress.org/ticket/17
cc @mdawaffe
There was a problem hiding this comment.
@mdawaffe is out for awhile, so it'll be best to add the domain to the banned list and circle back later if needed.
Agree with adding any domains that fail to the banned list instead of removing the functionality outright.
There was a problem hiding this comment.
Banned domain seems like a more direct and safer approach, sure. Sorta begs the question why we call Photon functions with a banned domain in the first place (at least assuming we know it would be one, as is the case here).
The explicit whitelisting is interesting though... because the filter it's attached to is related to query arguments being preserved, not unlike the issue we're seeking to resolve here. But the case we're encountering is not that the URL passed through jetpack_photon_url needs query arguments but rather the URL it redirects to (only determined Photon server-side). It may be the case that this redirect had not always existed?
jeherve
left a comment
There was a problem hiding this comment.
While the changes appear to work, I still see some issues in the WordPress.com comment iFrames; Photon still seems to be applied there:
The PR also didn't change the behaviour for Twitter comments, although Twitter is mentioned in the PR title. Should we exclude Twitter URLs as well?
I imagine we'll need to sync equivalent changes to WordPress.com for this to be resolved.
I'm not aware of any issues with Photon being applied to Twitter images. Excluding Twitter in the original changes was likely more a point of consistency in how we'd treated "foreign avatars" in the comment's |
That makes sense. Should we then exclude the Twitter domain from Photon as well? |
I don't think we need to. |
jeherve
left a comment
There was a problem hiding this comment.
It should work now! Worth noting that we'll need to fix this on WordPress.com as well.
|
It might be worth including Facebook's
Reported in 3221931-t and in this thread. |
jeherve
left a comment
There was a problem hiding this comment.
These last changes work for me! 🚢
* Changelog: first pass at a changelog for 5.0 * Changelog: delete 4.9 testing list. * Changelog: update minimum WP version to match ver. in jetpack.php Fixes #7158 * Changelog: add #6051 * Changelog: add #6753 * Changelog: add #6928 * Changelog: add #6964 * Changelog: add #7014 * Changelog: add #7057 * Changelog: add #7060 * Changelog: add #7068 * Changelog: add #7070 * Changelog: add #7072 * Changelog: add #7071 * Changelog: add release date and post shortlink. * Changelog: add #7094 * Changelog: add #7100 * Changelog: add #7108 * Changelog: add #7113 * Changelog: add #7123 * Changelog: add #7135 * Changelog: add #7143 * Changelog: add #7151 * Changelog: add #6996 * Changelog: add #7105 * Changelog: add #7132 * Changelog: add #7166 * Changelog: fix typo in 4.9 changelog. * Changelog: remove older releases' changelogs. @see p1HpG7-42e-p2 * Changelog: add #7090 * Changelog: add #7095 * Changelog: add #7112 * Changelog: add #7115 * Changelog: add #7122 * Changelog: add #7137 * Changelog: add #7138 * Changelog: add #7140 * Changelog: add #7154 * Changelog: add ##7155 * Changelog: add #7163 * Changelog: add #7167 * Changelog: add #7171 * Changelog: add #7180 * Changelog: add #7181 * Changelog: add #7183 * Changelog: add #7184 * Changelog: add #7189 * Changelog: add #7191 * Changelog: add #7193 * Changelog: add #7198 * Changelog: add #7200 * Changelog: add #7209 * Changelog: add #7212 * Testing list: add instructions for #7115 * Changelog: add #7188 * Changelog: add #7205 * Changelog: add #7225 * Changelog: add #6872 * Changelog: add #7107 * Changelog: add #7118 * Changelog: add #7142 * Changelog: add #7170 * Changelog: add #7210 * Changelog: add #7218 * Changelog: add #7232 * Changelog: add #7211 * Changelog: add #7213 * Changelog: add #7229 * Changelog: add #7230 * Changelog: add #7214 * Draft changelog for 5.0 * Changelog updates: 2nd pass at a clearer changelog. - Fix typos. - Use consistent tense and tone across all changelog. - Remove unclear items. * Changelog: add #7026 * Changelog: add #7058 * Changelog: add #7125 * Changelog: add #7249 * Changelog: add #7185 * add mentions of image widget migration * Changelog: add info about new output for CLI command. * Changelog: add WP version number matching the new Image Widget.
|
I'm still seeing broken Twitter avatars in my Jetpack comments. Is this the same problem? |
|
@billbennettnz we can't be sure, can you provide a link to the page where you're seeing broken Twitter avatars please? Or at least copy the URL of the broken image and paste it here in a comment? Thanks! |
|
Hi Igor
I replied earlier to your colleague Anne MaCarthy can you see that or shall I resend to you?
…Sent from my iPad
On 7/06/2017, at 8:44 PM, Igor Zinovyev ***@***.***> wrote:
@billbennettnz we can't be sure, can you provide a link to the page where you're seeing broken Twitter avatars please? Or at least copy the URL of the broken image and paste it here in a comment? Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@billbennettnz yes, thank you, Anne has pointed me in the right direction, let's continue there. |
|
@annezazu @zinigor I have another user for whom this is still happening in 3136104-t. Do y'all have a link to the ticket I can check out? Or maybe @billbennettnz can chime in if the issue was fixed? |
|
3250576-t @aheckler |
|
Just checked, it still happens. See the bottom comment on this page: https://billbennett.co.nz/2016/04/25/doing-one-thing-at-a-time-works-wonders/ |
Fixes Automattic/io#668
Fixes #6992
Changes proposed in this Pull Request:
Note query parameters after following redirect:
http://graph.facebook.com/600634677/picture
Testing instructions:
cc @KokkieH @kraftbj @chaitanyamsv