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

Use cURL instead of file_get_contents #97

Merged
merged 1 commit into from
Nov 28, 2019
Merged

Use cURL instead of file_get_contents #97

merged 1 commit into from
Nov 28, 2019

Conversation

MusikAnimal
Copy link
Member

This is because file_get_contents is slower and can't handle as big of
responses.

Also ensure our response code is the same as WikiWho's

Bug: T231492

@MusikAnimal MusikAnimal added the Ready for review This is ready for code review. label Nov 26, 2019
Copy link
Member

@samwilson samwilson left a comment

Choose a reason for hiding this comment

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

Works in my (limited) testing.

It is returning Content-Type: application/json even for things like HTML 404 errors. That's not the fault of this patch, but now the response headers are available here maybe it should copy that the same way it does the response code? Hardly a big concern though.

This is because file_get_contents is slower and can't handle as big of
responses.

Also ensure our response code is the same as WikiWho's

Bug: T231492
@MusikAnimal
Copy link
Member Author

It is returning Content-Type: application/json even for things like HTML 404 errors. That's not the fault of this patch, but now the response headers are available here maybe it should copy that the same way it does the response code? Hardly a big concern though.

Good point! I think I have this fixed. cURL apparently doesn't give us the full headers as an associative array or what have you, so I'm just copying over the content type, specifically (in addition to the response code).

This all makes me wonder if the proxying should be done entirely with Apache, but I suppose this will do for now.

Copy link
Member

@samwilson samwilson left a comment

Choose a reason for hiding this comment

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

Works in my testing.

@samwilson samwilson merged commit a58700c into master Nov 28, 2019
@samwilson samwilson deleted the proxy-curl branch November 28, 2019 00:56
@samwilson
Copy link
Member

I've also wondered about doing a webserver-only proxy, but it doesn't look like it's possible with a toolforge account; we'd need a VPS. So, if this works I think it's better to just stick with it: less to maintain.

@MusikAnimal
Copy link
Member Author

I've also wondered about doing a webserver-only proxy, but it doesn't look like it's possible with a toolforge account; we'd need a VPS. So, if this works I think it's better to just stick with it: less to maintain.

We're actually using VPS now, https://wikiwho.wmflabs.org/ . I couldn't get it to work on Toolforge, but I think that was because it was using file_get_contents. So we could go back to Toolforge, I suppose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review This is ready for code review.
Development

Successfully merging this pull request may close these issues.

2 participants