-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
@JonPurvis
JonPurvis
left a comment
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.
Great implementation!
I had one question regarding resolveCacheExpiry() which I've commented in the file.
Other than that, could you add some tests that:
- Cover
DateTimeImmutablebeing returned - Assert the cached entry is written with the correct expiry
- Any edge cases in
resolveCacheExpirysuch as 0, negative, date in the past
Thanks!
...-expiry # Conflicts: # src/Data/CachedResponse.php
JonPurvis
commented
Mar 4, 2026
Thanks for taking a look at my feedback, I'll get round to giving this a look over later this week / early next week. If it's all good, we can get it merged and get a new release out!
@JonPurvis
JonPurvis
left a comment
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.
👍 Looks good to me.
I'll speak with @Sammyjo20 next week (if not before) to see if he's happy to get this merged and arrange a release of this package!
Thanks for your work on this!
Sammyjo20
commented
Mar 7, 2026
The PR looks good to me but it still looks like it would be a breaking change because we're completely removing the cacheExpiryInSeconds method and changing it to resolveCacheExpiry which no one would have implemented, so updating the package would break it for their existing implementations.
JonPurvis
commented
Mar 7, 2026
Hey @Sammyjo20
We could perhaps release this as v4? We'd need a short upgrade guide somewhere though!
kevinmade
commented
May 11, 2026
@Sammyjo20 Do you already have a rough idea for when v4 is planned to be released?
No description provided.