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.

Sign up for GitHub

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

Jump to bottom

Reader/PostNorm: Leave https image srcs from non-wpcom hosts alone. #2922

Merged
merged 3 commits into from
Feb 4, 2016
Merged

Reader/PostNorm: Leave https image srcs from non-wpcom hosts alone. #2922

merged 3 commits into from
Feb 4, 2016

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Jan 29, 2016

This fixes images coming from private WPCOM sites and images from places like github and medium which block photon. It also fixes https images that rely on querystring arguments that photon cannot understand.

See #790 for a repro case

@blowery blowery added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. labels Jan 29, 2016
@blowery blowery self-assigned this Jan 29, 2016
@blowery blowery added this to the Reader: UI Nits milestone Jan 29, 2016
@blowery
Copy link
Contributor Author

blowery commented Jan 29, 2016

Fixes #790

@blowery
Copy link
Contributor Author

blowery commented Jan 29, 2016

Before, at https://wordpress.com/read/post/feed/25320833/916202374
screenshot

after, at http://calypso.localhost:3000/read/post/feed/25320833/916202374
screenshot

@blowery
Copy link
Contributor Author

blowery commented Jan 29, 2016

cc @designsimply as the OP

@blowery blowery force-pushed the try/leave-https-images-alone branch from 1aa5cd3 to 370abeb Compare January 29, 2016 20:11
bluefuton
bluefuton reviewed Jan 31, 2016
View reviewed changes
client/lib/post-normalizer/index.js
@@ -101,6 +101,12 @@ function maxWidthPhotonishURL( imageURL, width ) {
return imageURL;
}

if ( ! ( endsWith( parsedURL.host, 'wp.com' ) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could pop these domain endings into an array rather than doing three separate comparisons?

Copy link
Contributor

Choose a reason for hiding this comment

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

&possibly wrapped in a isPhotonableHost method for extra clarity?

@bluefuton bluefuton added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 31, 2016
@blowery
Copy link
Contributor Author

blowery commented Feb 1, 2016

Going to take a different tact with this one. Automattic/photon.js#5 will allow us to fetch the github (and other secure content) images via photon. I'd rather do that than open us up to pulling images from just anywhere.

When that lands, this PR will change to just update the photon dep. No other changes should be necessary.

@blowery blowery force-pushed the try/leave-https-images-alone branch from 370abeb to a9f595e Compare February 2, 2016 20:54
@blowery blowery force-pushed the try/leave-https-images-alone branch from a9f595e to b2b578a Compare February 2, 2016 21:04
@blowery
Copy link
Contributor Author

blowery commented Feb 2, 2016

Photon landed, PR updated. So far, so good. Need to check the few places we use photon handle the new null return.

@blowery
Copy link
Contributor Author

blowery commented Feb 2, 2016

cc @gwwar could you take a peek at the shrinkwrap change here? I updated it using clingwrap. Seems odd that it removed so much from each entry?

@blowery blowery added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 2, 2016
@gwwar
Copy link
Contributor

gwwar commented Feb 2, 2016

See: goodeggs/clingwrap#1 and npm/npm#3581 for more info.

@gwwar
Copy link
Contributor

gwwar commented Feb 2, 2016

@blowery with regards to the missing fields, I think we should be ok since we don't have any private registries. 👍 I tested the npm install from a clean state and looks good.

@bluefuton
Copy link
Contributor

Looks good to me :shipit:

@blowery
Copy link
Contributor Author

blowery commented Feb 4, 2016

@gwwar huh. that all led me to https://www.npmjs.com/package/shonkwrap

blowery added a commit that referenced this pull request Feb 4, 2016
@blowery
Merge pull request #2922 from Automattic/try/leave-https-images-alone
3061f0f 
Reader/PostNorm: Leave https image srcs from non-wpcom hosts alone.
@blowery blowery merged commit 3061f0f into master Feb 4, 2016
@blowery blowery deleted the try/leave-https-images-alone branch February 4, 2016 14:46
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 4, 2016
This was referenced Jun 29, 2021
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@blowery blowery

Labels
[Feature] Reader The reader site on Calypso.
Projects
None yet
Milestone
Reader: UI Nits
Development

Successfully merging this pull request may close these issues.
4 participants
@blowery @gwwar @bluefuton @scruffian