Add check for local files before processing post images in Photon

[hanifn]

Summary

In Jetpack_Photon::filter_the_content, the call to strip_image_dimensions_maybe is outside the if clause that checks for local uploads. This is problematic because strip_image_dimensions_maybe only checks if the filename has the image dimensions string, e.g.: 300x300, and then calls file_exists on this file after removing the image dimensions and prepending the basedir. If a file is hosted externally, like on a CDN, the call to file_exists will only return false. Additionally, if the site is using a custom Stream wrapper implementation to upload files to the CDN, these file_exists calls results in a lot of requests to the CDN that returns 404 status. This means those requests are uncached and would slow down the site load times.

Changes proposed in this Pull Request:

• Create a dedicated function to check if a file is locally uploaded or not.
• Add the local file check into strip_image_dimensions_maybe.

Testing instructions:

Note: Tested on a site hosted on VIP Go.

• Enable Jetpack Photon for site.
• Create a new post with an image that's hosted externally and with a filename that has ends in something like -300x300.jpg.
• Visit post and make sure there's no external calls to CDN that results in a 404 response.

Proposed changelog entry for your changes:

• Add check for local file upload before processing post images with Photon

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: September 3, 2019.
Scheduled code freeze: August 27, 2019

Generated by 🚫 dangerJS against a84daa0

[jeherve]

It seems to work in my tests. Thank you for adding tests as well. I would have 2 minor remarks.

changes addressed

[zinigor]

I know I'm probably overthinking it, but can we go back to only getting upload dir details once per image? Currently with this code we're doing it three times. And while the code only runs the "lightweight" version that doesn't refresh the upload folder every time, it still runs through filters that may be set on the upload dir location. We will be triggering these filters three times more now with this change.
Can we get rid of the is_local_upload method, store the upload folder data as before and just run strpos inside if conditions?

[hanifn]

@zinigor That makes sense. Considering that we're only using this in only 2 places in the class right now, the new is_local_upload might be unneeded. Still, we would need to do the same check in strip_image_dimensions_maybe so the number of calls to wp_get_upload_dir() would only be reduced by one. What do you think of moving the call to strip_image_dimensions_maybe() inside the first if clause that checks for local file? That should reduce the need to get the upload directories to just once.

[zinigor]

Whoa, sorry for such a delay with the response, @hanifn . Yeah, sure, if we can reduce the number of calls even more, I'm all for that. Can you please make it, as well as rebase the code so that it wouldn't conflict? I'm happy to take a look and test again.

[hanifn]

@zinigor Changes made and rebased on master in a84daa0

[zinigor]

Thanks, sorry it took me so long to review this one!