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.

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

Fix handling of redirect response without Location header #116

Merged

Conversation

PikachuEXE
Copy link
Contributor

Fix ArgumentError thrown when the response for the URL does not contain header Location

image

Got this when trying to use this gem to check and report invalid images from a long list of image URLs

@PikachuEXE
Copy link
Contributor Author

Not sure why the build is not displayed but it passed:
https://travis-ci.org/github/sdsykes/fastimage/builds/699561478

@sdsykes
Copy link
Owner

sdsykes commented Jun 18, 2020

Thanks for the patch, I'll merge it.

BTW, FastImage does not robustly check the validity of images at all.

@PikachuEXE
Copy link
Contributor Author

I think it's good enough for our case
Just need to catch common issues for images hosted by others (totally not controlled by us)
Then we can shift the blame as long as the issues are reported

@Nakilon
Copy link
Contributor

Nakilon commented Jun 18, 2020

Not sure how it is "a common issue". Catching redirects for years using my gem built on top of Fastimage and never saw an empty redirect.

@PikachuEXE
Copy link
Contributor Author

Even it's uncommon I don't think it should throw ArgumentError instead of a meaningful one (which is an "image fetch failure")
I consider the lack of Location header as a form of "bad response"

@PikachuEXE
Copy link
Contributor Author

@sdsykes Let me know if anything else needed before merging this

@sdsykes sdsykes merged commit 099c5e4 into sdsykes:master Jun 22, 2020
@PikachuEXE PikachuEXE deleted the fix-incorrect-redirect-response-handling branch June 22, 2020 05:21
@PikachuEXE
Copy link
Contributor Author

Is this going to be released soon?

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.

None yet

3 participants