Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added ability to extend tokens #24

Merged
merged 2 commits into from Aug 7, 2012

Conversation

Projects
None yet
7 participants
Contributor

mattwkelly commented May 31, 2012

This adds the ability to get extended tokens. Most folks use the JS client-side flow to auth users, which generates a short-lived token.

They can now call extendAccessToken() in order to get an long-lived token.

Note that this also destroys the original JS session, so some discussion may be needed as I don't know of any unintended consequences this could have.

Contributor

mattwkelly commented May 31, 2012

This shouldn't be in the PHP but in the JS.

Formatting is messed up here, new line and spaces between 'key' and the =>

Contributor

mattwkelly commented Jun 1, 2012

@scottmac Not sure what you mean. The JS SDK should be able to support getting long-lived tokens, but the PHP SDK should also support that.

Contributor

daaku commented Jun 12, 2012

This looks fine -- could you fix up the indentation/style and 80 col issues.

@onema onema and 2 others commented on an outdated diff Jun 23, 2012

src/base_facebook.php
+ }
+
+ if (empty($access_token_response)) {
+ return false;
+ }
+
+ $response_params = array();
+ parse_str($access_token_response, $response_params);
+
+ if (!isset($response_params['access_token'])) {
+ return false;
+ }
+
+ $this->destroySession();
+
+ $this->setPersistentData('access_token', $response_params['access_token']);
@onema

onema Jun 23, 2012

Shouldn't this method return the extended token value? after all is called getExtendedAccessToken, right?

@mattwkelly

mattwkelly Jun 29, 2012

Contributor

@onema: I'll change the function name to set*. Sound reasonable?

@onema

onema Jun 30, 2012

actually get* is not a bad name, just return the extended token you just requested for. set* is not a very good name because you are not setting anything (not passing any parameters, see other setter methods).

If you really don't want to return the token I think request* would be a better name?

@mattwkelly

mattwkelly Jul 3, 2012

Contributor

It is setting a new access token.

@MarcHoogvliet

MarcHoogvliet Jul 19, 2012

Hey, I posted this in January on StackOverflow, called it getExtendedAccessToken which seemed to make the most sense to me back then. I found most use in it when it returns the new accesstoken.

You can see it here, http://stackoverflow.com/questions/8982025/how-to-extend-access-token-validity-since-offline-access-deprecation/9035036#9035036

I'm also not sure why you'd want to destroy the session in there?

@mattwkelly

mattwkelly Jul 26, 2012

Contributor

@joinscribbly, if you don't destroy the session, you'll end up with two, which can lead to all sorts of problems.

Contributor

mattwkelly commented Jun 29, 2012

Updated. @NShah, @scottmac, good to go?

@onema onema commented on the diff Jun 30, 2012

src/base_facebook.php
+ // In any event, we don't have an access token, so say so.
+ return false;
+ }
+
+ if (empty($access_token_response)) {
+ return false;
+ }
+
+ $response_params = array();
+ parse_str($access_token_response, $response_params);
+
+ if (!isset($response_params['access_token'])) {
+ return false;
+ }
+
+ $this->destroySession();
@onema

onema Jun 30, 2012

Should you be destroying the session here? The only methods that use this in the BaseFacebook class are _rest and throwAPIException. this can cause problems if you don't leave it up to the user of the class to kill the session. Makes sense?

@mattwkelly

mattwkelly Jul 3, 2012

Contributor

What problems can it lead to?

@roman02

roman02 Nov 30, 2013

Is there a benefit in destroying the session here?

@mattwkelly mattwkelly added a commit that referenced this pull request Aug 7, 2012

@mattwkelly mattwkelly Merge pull request #24 from mattwkelly/master
Added ability to extend tokens
1270f0d

@mattwkelly mattwkelly merged commit 1270f0d into facebookarchive:master Aug 7, 2012

Why does this not call setAccessToken($response_params['access_token'])?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment