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

moves caching to wrapper class 'FetchAuthTokenCache' and adds Memory cache #123

Merged
merged 2 commits into from
Aug 2, 2016

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Jul 19, 2016

Addresses #121

Highlights:

  • Adds FetchAuthTokenCache wrapper class to implement caching on anything implementing FetchAuthTokenInterface.
  • Removes caching from all Middleware / Subscriber classes
  • Bypasses caching (by setting the key to null) for AppIdentityCredentials - Caching here is handled by the AppIdentityService
  • Removes calls to getCacheKey from Cache Trait - that function does not exist as part of the trait, and as a result was causing some unexpected problems. The cache key is now explicitly passed in.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 19, 2016
@bshaffer
Copy link
Contributor Author

@dwsupplee and @jdpedrie I would love it if you could take a look as well!

@bshaffer bshaffer force-pushed the move-caching-to-credentials branch from 0da60bd to c781f7d Compare July 19, 2016 18:21
// Use the cached value if its available.
//
// TODO: correct caching; update the call to setCachedValue to set the expiry
// to the value returned with the auth token.

This comment was marked as spam.

This comment was marked as spam.

@tmatsuo
Copy link
Contributor

tmatsuo commented Jul 19, 2016

LGTM, I have one question but it can be left as TODO and you can file another PR.

@bshaffer
Copy link
Contributor Author

As discussed, we should add an in-memory implementation for PSR-6 (for example) to this library to use as a default caching mechanism.

This can be consumed by googleapis/gax-php and googlecloudplatform/gcloud-php

@dwsupplee
Copy link
Contributor

I've opened #126 against this PR which includes an in-memory implementation. Let me know what you think :).

* add simple in-memory cache implementation
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 2, 2016
@bshaffer bshaffer changed the title moves caching to wrapper class 'FetchAuthTokenCache' moves caching to wrapper class 'FetchAuthTokenCache' and adds Memory cache Aug 2, 2016
@dwsupplee
Copy link
Contributor

I consent

@bshaffer
Copy link
Contributor Author

bshaffer commented Aug 2, 2016

yeah it doesn't seem to work quite right in this scenario 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no This human has *not* signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants