Skip to content

don't parse image, for invalid server responses.#56

Closed
palamccc wants to merge 1 commit intowillnorris:masterfrom
palamccc:master
Closed

don't parse image, for invalid server responses.#56
palamccc wants to merge 1 commit intowillnorris:masterfrom
palamccc:master

Conversation

@palamccc
Copy link

No description provided.

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@palamccc
Copy link
Author

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@willnorris
Copy link
Owner

So I'm curious what the motivation for this is. Were you running into specific trouble?

It's entirely possible that the remote server returns an image with a non-200 response. In which case, transforming the image as requested still sort of makes sense. If the response isn't an image, then it obviously won't be transformed anyway.

if err != nil {
glog.Errorf("error transforming image: %v", err)
var img []byte;
if resp.StatusCode == 200 {

Choose a reason for hiding this comment

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

if resp.StatusCode == http.StatusOK {
...
}

huguesalary added a commit to betabrand/imageproxy that referenced this pull request Aug 1, 2017
@willnorris
Copy link
Owner

Closing this PR, since there was never any response to why this is actually needed. (Please feel free to continue the discussion if this is still needed)

@huguesalary's betabrand@34252f7 seemed to be related to this (though I'm not 100% sure how). That has now been merged as 3444fd9, so maybe that fixes the root issue anyway?

@willnorris willnorris closed this Mar 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants