Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
I haven't seen that the token expiration is saved anywhere, I think on top of forcing a refresh, the service should be smart enough to handle auto expiration.
Proposed resolution
Save the token expiration and calculate when to expire.
Comment | File | Size | Author |
---|---|---|---|
#7 | 3195685-7.patch | 7.92 KB | john_a |
| |||
#6 | 3195685-6.patch | 6.5 KB | john_a |
Comments
Comment #2
pcambraFWIW this is the code I have in my project for the token expiration
And then on the token method, I return the whole thing:
Needs DI as well, but this handles more or less this issue and #3195684: State API is a better fit to store the token, I can try to make a patch if you're interested
Comment #3
john_a CreditAttribution: john_a as a volunteer commentedThat's an interesting idea. I double checked on The MC site, and it's set to a fixed 20 minutes.
Most builders would probably not care and would use the default TTL from MC. However, although there are no guidelines on specific times for TTL, it should be as short as possible.
Thanks for the code sample, I think we could use the same approach and set a default value on install for token TTL=20 minutes, to stay in line with MC. That way we get the maximum life (if the developer wants that), or they can set it to a lower value if they are security conscious and feel they do not need 20 minutes per session.
Comment #4
john_a CreditAttribution: john_a as a volunteer commentedI didn't see your offer for a patch, and have already created one for #3195684: State API is a better fit to store the token, which includes changes to the functional tests and a hook update.
If you agree with my patch, I can treat that as a dependency, merge that in and then create the patch for this.
Comment #5
pcambraSounds good, even if the docs say 20 mins, my experience is that MC is a bit... stingy? and never gives you 20 minutes, which was causing edges case on my use case
Comment #6
john_a CreditAttribution: john_a as a volunteer commentedPatch here. I've not implemented your change to use only
access_token
from the body in the request token. We had a previous ticket to fix the fetching of the token for v2, where MC for some strange reason changed the key - so this code needs to work for v1 & v2.the patch also includes a hook update.
Comment #7
john_a CreditAttribution: john_a as a volunteer commentedUpdated patch - there is a bug in the tests updated form last time. Not sure why the tests passed last time...
Comment #8
john_a CreditAttribution: john_a as a volunteer commentedComment #10
john_a CreditAttribution: john_a as a volunteer commentedCommitting this to dev and marking as ready for release
Comment #11
john_a CreditAttribution: john_a as a volunteer commentedComment #13
john_a CreditAttribution: john_a as a volunteer commented