Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

Return value of setExtendedAccessToken #72

Closed
wants to merge 2 commits into from
Closed

Return value of setExtendedAccessToken #72

wants to merge 2 commits into from

Conversation

tugrul
Copy link

@tugrul tugrul commented Apr 21, 2013

I added return value to setExtendedAccessToken function to
determine of success API operation. This provides to get
correct access_token value from getAccessToken function

Correct use case of setExtendedAccessToken:

if ($fb->setExtendedAccessToken()) {
    $my->storeAccessToken($fb->getUser(), $fb->getAccessToken());
}

I made small fix for malformed multiline comment

I added return value to setExtendedAccessToken function to
determine of success API operation. This provides to get
correct access_token value from getAccessToken function

I made small fix for malformed multiline comment
I added getExtendedAccessToken function to get exchange_token
oAuth operation's result direcly. This function helps to developers
get extended access token and expire time to store anywhere to use
later.
@eddyvlad
Copy link

I like this because getExtendedAccessToken is all we need. setExtendedAccessToken is just silly because there is no control to whether I want it persistent or not. Also it is not consistent with the pre-existing setAccessToken which accepts argument. Speaking of which, setPersistentData should've been made public.

Well... much of the SDK is flawed anyway because there is no context for the persistent data. This is annoying if you are working with more than one instance.

@tugrul
Copy link
Author

tugrul commented Apr 22, 2013

@eddyvlad I agree with you about setPersistentData. This behavior coming from past. I didn't change this behavior. I just added small functionallity to improve usability

@eddyvlad
Copy link

@tugrul Yeah. I know setPersistentData wasn't from you. I'm just puzzled why they decided to merge it. Then now its too late to change the behaviour because it will be considered as breaking change.
I'm going to merge yours into my fork anyway. Thanks for this.

@gfosco
Copy link
Contributor

gfosco commented Oct 15, 2013

Sorry it took so long to respond to this. Can you sign the Contributor License Agreement? https://developers.facebook.com/opensource/cla

Once that's done we can look at this further, but it looks like a good idea. Thanks for your contribution.

@tugrul
Copy link
Author

tugrul commented Oct 15, 2013

I wish to sign but i dont have got official Facebook account. I have got an unofficial account (who named John Doe) just to use for development.

@Bnaya
Copy link

Bnaya commented Oct 15, 2013

You can get the new access token via getAccessToken, but what you can't currently get its the new expiration time of the token without another api call.
it will be nice to have that.

@gfosco
Copy link
Contributor

gfosco commented Oct 28, 2013

We'll address this and other enhancements for the access token in future releases.

@gfosco gfosco closed this Oct 28, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants