starred/spotify-web-api-php

Fork 0

mirror of https://github.com/jwilsson/spotify-web-api-php.git synced 2026-04-26 23:45:49 +03:00

[GH-ISSUE #20] Refresh token should not be in Session #9

New issue

Closed

opened 2026-02-27 19:25:35 +03:00 by kerem · 9 comments

kerem commented

2026-02-27 19:25:35 +03:00

Owner

Originally created by @hvt on GitHub (Dec 25, 2014).
Original GitHub issue: https://github.com/jwilsson/spotify-web-api-php/issues/20

Just wanted to check whether you guys would agree on this, or not.

In my opinion, the refresh_token should not exist in the Session class. It should just be returned by Session::requestToken() (I refactored that to Session::requestRefreshAndAccessToken() in https://github.com/jwilsson/spotify-web-api-php/pull/19) and be supplied when the access token is refreshed.

Why? Well, at the moment it is stored in the class for convenience, but no one would be using it like this. The access token expiration takes quite long (one hour at the moment), that anyone using the refresh token should already save it somewhere persistent. By removing it from the class, it will be more obvious the refresh token is not something volatile. Plus you leave it up to the user to do something with the token or not.

Therefore, I'd like to suggest to:

Remove Session::$refreshToken (plus its associated get and set methods)
Change Session::requestToken() (Session::requestRefreshAndAccessToken() in https://github.com/jwilsson/spotify-web-api-php/pull/19) to return an array containing the three returned values (access token, expiry date, and the refresh token) and null when something goes wrong.
Change Session::refreshToken() (Session::refreshAccessToken() in https://github.com/jwilsson/spotify-web-api-php/pull/19) to have the refresh token as a parameter.

Next, something that just came to my mind: the access token's expire period also doesn't do much. I.e. it's saved, but you know nothing of it (e.g. when was the last access token retrieved). So we should either do something with it (calculate when the access token actually expires and act on it), or just leave this to the user of the class...

Just some ideas, wonder what you guys think ;].

Originally created by @hvt on GitHub (Dec 25, 2014). Original GitHub issue: https://github.com/jwilsson/spotify-web-api-php/issues/20 Just wanted to check whether you guys would agree on this, or not. In my opinion, the `refresh_token` should not exist in the `Session` class. It should just be returned by `Session::requestToken()` (I refactored that to `Session::requestRefreshAndAccessToken()` in https://github.com/jwilsson/spotify-web-api-php/pull/19) and be supplied when the access token is refreshed. Why? Well, at the moment it is stored in the class for convenience, but no one would be using it like this. The access token expiration takes quite long (one hour at the moment), that anyone using the refresh token should already save it somewhere persistent. By removing it from the class, it will be more obvious the refresh token is not something volatile. Plus you leave it up to the user to do something with the token or not. Therefore, I'd like to suggest to: - Remove `Session::$refreshToken` (plus its associated get and set methods) - Change `Session::requestToken()` (`Session::requestRefreshAndAccessToken()` in https://github.com/jwilsson/spotify-web-api-php/pull/19) to return an array containing the three returned values (access token, expiry date, and the refresh token) and `null` when something goes wrong. - Change `Session::refreshToken()` (`Session::refreshAccessToken()` in https://github.com/jwilsson/spotify-web-api-php/pull/19) to have the refresh token as a parameter. Next, something that just came to my mind: the access token's expire period also doesn't do much. I.e. it's saved, but you know nothing of it (e.g. when was the last access token retrieved). So we should either do something with it (calculate when the access token actually expires and act on it), or just leave this to the user of the class... Just some ideas, wonder what you guys think ;].

kerem

2026-02-27 19:25:35 +03:00

closed this issue
added the
feedback wanted
label

kerem commented

2026-02-27 19:25:36 +03:00

Author

Owner

@jwilsson commented on GitHub (Dec 28, 2014):

I think all of these points are great and very valid points. It would probably make the whole authorization process easier as well.

I do however think that we should keep Session::getRefreshToken() but remove Session::setRefreshToken() since one might want to retrieve the token a while after the initial authorization.

I also definitely think we should calculate the expires time for the user and return a Unix timestamp indicating when the access token expires, e.g. time() + $expires but leave it to the user to actually act on it when the token expires. This change will of course let the user check if the access token is expired without sending a request that will fail and then act on it.

Pinging @vdesabou since I know he's a big user of this.

@jwilsson commented on GitHub (Dec 28, 2014): I think all of these points are great and very valid points. It would probably make the whole authorization process easier as well. I do however think that we should keep `Session::getRefreshToken()` but remove `Session::setRefreshToken()` since one might want to retrieve the token a while after the initial authorization. I also definitely think we should calculate the expires time for the user and return a Unix timestamp indicating when the access token expires, e.g. `time() + $expires` but leave it to the user to actually act on it when the token expires. This change will of course let the user check if the access token is expired without sending a request that will fail and then act on it. Pinging @vdesabou since I know he's a big user of this.

kerem commented

2026-02-27 19:25:36 +03:00

Author

Owner

@vdesabou commented on GitHub (Dec 28, 2014):

Pinging @vdesabou since I know he's a big user of this.

I think all the suggestions proposed here are good. On my side, I'm using the library in a command line mode, so I re-create an instance of Session and SpotifyWebAPI every time. I store the refresh token, and the expire time in a DB and calls refreshToken() when needed. I don't think I'm gonna use getExpires() anyway.

@vdesabou commented on GitHub (Dec 28, 2014): > Pinging @vdesabou since I know he's a big user of this. I think all the suggestions proposed here are good. On my side, I'm using the library in a command line mode, so I re-create an instance of Session and SpotifyWebAPI every time. I store the refresh token, and the expire time in a DB and calls `refreshToken()` when needed. I don't think I'm gonna use `getExpires()` anyway.

kerem commented

2026-02-27 19:25:36 +03:00

Author

Owner

@hvt commented on GitHub (Dec 28, 2014):

So we'll keep Session::getRefreshtoken() (plus the attribute).
Remove Session::setRefreshToken().
The setter will be replaced by supplying the refresh token as a parameter in Session::refreshAccessToken().
We save the expiration timestamp of the access token (i.e. time() + $expires) as an attribute of Session.

Some more discussion: what about renaming the Session::getExpires() to Session::getAccessTokenExpiration()? And would adding an Session::isAccessTokenExpired() method be of any help?

@hvt commented on GitHub (Dec 28, 2014): - So we'll keep `Session::getRefreshtoken()` (plus the attribute). - Remove `Session::setRefreshToken()`. - The setter will be replaced by supplying the refresh token as a parameter in `Session::refreshAccessToken()`. - We save the expiration timestamp of the access token (i.e. `time() + $expires`) as an attribute of `Session`. Some more discussion: what about renaming the `Session::getExpires()` to `Session::getAccessTokenExpiration()`? And would adding an `Session::isAccessTokenExpired()` method be of any help?

kerem commented

2026-02-27 19:25:36 +03:00

Author

Owner

@jwilsson commented on GitHub (Dec 30, 2014):

👍

Hmm, I like the renaming idea but I don't know if an Session::isAccessTokenExpired() method would be of any help. I'm not against it but I think we could hold it off for a bit if no one objects.

@jwilsson commented on GitHub (Dec 30, 2014): :+1: Hmm, I like the renaming idea but I don't know if an `Session::isAccessTokenExpired()` method would be of any help. I'm not against it but I think we could hold it off for a bit if no one objects.

kerem commented

2026-02-27 19:25:36 +03:00

Author

Owner

@hvt commented on GitHub (Jan 5, 2015):

Will start on this once I got some more time (probably next week or so).

@hvt commented on GitHub (Jan 5, 2015): Will start on this once I got some more time (probably next week or so).

kerem commented

2026-02-27 19:25:36 +03:00

Author

Owner

@jwilsson commented on GitHub (Jan 5, 2015):

Don't worry about it. I don't think we should include this until the 1.0 release anyway (and it's a while before that will happen).

@jwilsson commented on GitHub (Jan 5, 2015): Don't worry about it. I don't think we should include this until the 1.0 release anyway (and it's a while before that will happen).

kerem commented

2026-02-27 19:25:36 +03:00

Author

Owner

@jwilsson commented on GitHub (Sep 29, 2015):

@hvt you interested in taking a stab at this? Just let me know if not and I'll take care of it :)

@jwilsson commented on GitHub (Sep 29, 2015): @hvt you interested in taking a stab at this? Just let me know if not and I'll take care of it :)

kerem commented

2026-02-27 19:25:36 +03:00

Author

Owner

@hvt commented on GitHub (Sep 29, 2015):

@jwilsson No I haven't. You're good to fix it yourself :].

@hvt commented on GitHub (Sep 29, 2015): @jwilsson No I haven't. You're good to fix it yourself :].

kerem commented

2026-02-27 19:25:36 +03:00

Author

Owner

@jwilsson commented on GitHub (Sep 29, 2015):