-
Notifications
You must be signed in to change notification settings - Fork 192
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
V2.0 #322
base: main
Are you sure you want to change the base?
Conversation
cc @ericnorris |
@dwsupplee @jdpedrie this is ready for review! |
|
||
private function fetchAuthTokenNoCache(): array | ||
{ | ||
throw new \LogicException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traits can have abstract methods, would that make more sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. It might make sense to make them abstract, but I was thinking that since both these functions are optional (credentials don't have to support caching) that it makes more sense to throw the exception at runtime. In the case of AnonymousCredentials
, there's no way for those exceptions to be thrown because they'd never be called.
Thanks for tagging me in this! There's a lot to go through, and I'll also have to dig into my memory a bit to see if there's anything else I had thought about since the last time we spoke. After a quick glance though, I have some high-level comments that you can take or leave, in no particular order:
Semi-relatedly, during some down-time last year I took a stab at writing what I would have done for a Google PHP auth library. You can take a look at my repo here for any ideas, since I chose to do some of the things I suggested above, or not - no pressure on my end. |
Do you know when this version with PSR Cache v3 support will be released? |
@wlasnapl we do not have an ETA on that, but we do support If you find a library which supports |
@bshaffer I've wrote an issue to Contetful support and I can see that there is PR for that: contentful/contentful.php#308 |
See UPGRADING.md
TODO
Fix SysVCachePool race conditionVerify token is not expired before using it (This is now done with the cache but have additional check)Make fetching IAP certs vs OIDC certs implicit (currently the user has to specify the IAP cert URL)Consider adding caching toOAuth2
Consider adding methodhasValidToken
toOAuth2
final
withScope
andwithProject
methods which clone the immutable objects