search

Weble / ZohoClient

11 stars 7 forks source link

Cache Expiration Issue #17

Closed mhallhol closed 3 years ago

mhallhol commented 3 years ago

While upgrading to ^4.0 from ^3.0, I noticed that caching was no longer functioning using Laravel's cache driver (via a cache.psr6 adaptor), and a database cache driver. However, this worked with the file driver without issue.

Delving a little deeper into the issue, it appears the cause of the problem is the cache update block is using ->expiresAfter(), which expects an argument in seconds. The data entered into this is from accessToken->getExpires(), which returns an epoch value rather than a duration in seconds.

This may not have shown up under some cache adapters due to the expiration period not being validated, however Laravel's database cache driver refused to set the expiration date based on current epoch + expires epoch due to it being vastly out of range. The file driver ignored this error, and expiration still functioned correctly due to the serialised access token, including its expiration time, being loaded back out of cache.

The solution to this appears to be to convert the epoch from getExpires() to a \DateTime, and use the cache's ->expiresAt() function to explicitly set the expiration epoch.

I have created PR#16 for review

tm1000 commented 3 years ago

In 4.1 we fixed an issue with zoho and expires however I think you've seen that because you asked the owner of the zoho oauth library to update. Hopefully you also noticed we extended that class to Apple the changes directly

Your PR makes sense. In my testing so far I've only done it with a file system cache driver as I haven't pushed it into production yet

mhallhol commented 3 years ago

That's right, I did see there had previously been an issue with the result returned in milliseconds (in true Zoho style!). I saw the class had been extended, prior to the author for the original zoho oauth client merging the PR (though without releasing).

This does seem to be a separate issue though, I've tested this with Laravel's database and file drivers using the update in the PR code and it seems to function as expected, worth checking though! Originally with the database driver the cache was not getting pushed to database, and was failing silently (even outside of the try block), but altering the expiration seems to remedy it.

Skullbock commented 3 years ago

Good catch @mhallhol ! Merged the PR and released 4.1.1 Thanks!