-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
allow facebook responses to be in JSON or text/html when requesting a… #1977
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always submit PRs to master; if there is going to be a patch release to an older branch I'll deal with the cherry-picking then. (but it's very unlikely that there will be patch releases of older versions).
tornado/auth.py
Outdated
@@ -985,9 +986,19 @@ def _on_access_token(self, redirect_uri, client_id, client_secret, | |||
future.set_exception(AuthError('Facebook auth error: %s' % str(response))) | |||
return | |||
|
|||
args = urlparse.parse_qs(escape.native_str(response.body)) | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to tell which version we're dealing with instead of a try/catch? Do they set the content-type header correctly?
And it's a server-side API, so do we even need to deal with the older version, or did the new version just replace it? It looks like version 2.3 reached its end-of-life today.
tornado/auth.py
Outdated
Facebook graph API versions 2.3+ return JSON instead of query strings | ||
in the response body | ||
""" | ||
args = json.loads(response.body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use tornado.escape.json_decode (which will do the right thing on python 3)
… fixes the broken test
@bdarnell thanks for the help. I updated the pull request. For posterity, here's the excerpt from Facebook's changelog:
The changelog can be found here: https://developers.facebook.com/docs/apps/changelog And to echo what you already said, according to that changelog, v2.2 has reached it's end of life as of today, March 25, 2017. |
Thanks! I'll make a 4.4.3 release with this patch and make a new 4.5 beta. |
@daynejones (or anyone else who's watching this issue) Would you mind weighing in on #2001? I don't use this API myself so I'd like to get another opinion on the significance of the |
Since Graph API v2.3, Facebook "fixed" their access_token endpoint. It no longer returns plaintext query strings but JSON. This issue came up during an update to facebook's graph api v2.8.
I'm using 4.3 so I assume that's the branch I should merge into.