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

Support setting refreshToken #994

Merged
merged 8 commits into from
Apr 16, 2023
Merged

Conversation

grimmdude
Copy link
Contributor

@grimmdude grimmdude commented Feb 26, 2023

In the cases where the refresh token isn't included in the response when using it to refresh an access token, it's often necessary to persist the original refresh token for later use. When this happens it's convenient to store the refresh token in the AccessToken object as it was originally.

However, because AccessToken::$refreshToken is a protected property it's not possible to assign this value directly. Implementing AccessToken::setRefreshToken() allows the refresh token to be set and stored along with the AccessToken object.

The workaround I've found is to serialize the AccessToken object to JSON, add the refresh token, then reinstantiate a new AccessToken object. It would be great if this wasn't necessary.

Building the functionality directly into the library to automatically carry over the refresh token would be great (as suggested in #658), but I'm not sure if that behavior would apply to all cases. It might cause confusion if it was intentionally omitted from the response because it's no longer valid.

Related to #658

@codecov
Copy link

codecov bot commented Apr 16, 2023

Codecov Report

Merging #994 (49777e3) into master (164ccb0) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              master      #994   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       190       191    +1     
===========================================
  Files             20        20           
  Lines            514       516    +2     
===========================================
+ Hits             514       516    +2     
Impacted Files Coverage Δ
src/Token/AccessToken.php 100.00% <100.00%> (ø)

@grimmdude grimmdude requested a review from ramsey April 16, 2023 18:47
@ramsey ramsey merged commit 5b64ba6 into thephpleague:master Apr 16, 2023
@grimmdude grimmdude deleted the set_refresh_token branch April 16, 2023 19:20
@Seldaek
Copy link

Seldaek commented Nov 13, 2023

Heya @ramsey @shadowhand could I bother you for a release with this PR? A few commits are missing from 2.7.0 2.7.0...master - thanks 🙏🏻

@Seldaek
Copy link

Seldaek commented Nov 5, 2024

@ramsey @shadowhand sorry to nag again but it's been a year and still no release :)

@Seldaek
Copy link

Seldaek commented Nov 5, 2024

And if someone does a release please merge #1039 first so we get PHP 8.4 compatibility sorted as well :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants