php-twinfield / twinfield

PHP 7.3+ Library for using the Twinfield API.
https://accounting.twinfield.com/webservices/documentation/#/
Other
33 stars 77 forks source link

Support existing accessToken on OpenIdConnectAuthentication #220

Closed ericclaeren closed 1 year ago

ericclaeren commented 1 year ago

Hi, I've created a pull request allowing people to:

  1. Setup a connection with an existing access token object
  2. Add a custom callback to be called when a token is refresh, to process the token.

There's already a similar idea in: https://github.com/php-twinfield/twinfield/pull/144#issuecomment-1361121363

That PR is fairly old and did not seem to be very active. Also it included some other unrelated improvements, that might makes this hard to merge to master.

The code has some breaking changes in case someone is overriding src/Secure/OpenIdConnectAuthentication.php login() or validateToken()

Summary of improvements

Previous login flow on every getAuthenticatedClient() call

New flow

I've documented the use case in the readme.md file to provide clarity and documentation and wrote tests for this functionality.

Hope this PR can be helpful and if there are questions, don't hesitate to contact me or responding on this PR.

Cheers

rojtjo commented 1 year ago

Thank you for your pull request, it looks great!

I'm just wondering if it would make sense to move these changes to the v4 branch instead. That way we could maybe get rid of the refreshToken property all together as it's already included in the AccessToken.

Let me know what you think!

ericclaeren commented 1 year ago

Hi @rojtjo,

Removing $refeshToken was also one of my initial thoughts, left it in for compatibility. But definitely agree with you, removing this would make sense.

If you agree, I would prefer to do both, I'm happy porting this code to the v4 branch. But also think it might be good to introduce this and gather feedback on it.

And I need it in the current v3 branch, to limit the amount of requests, since I'm syncing a rather large data set to Twinfield. But if you don't want to merge it and hold on to it for v4, that's fine by me, I can still use this PR to patch this package.

rojtjo commented 1 year ago

I guess that makes sense, it's easy to change this for v4 once merged.

ericclaeren commented 1 year ago

Thanks!