-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(cache): add deleteMatching method definition in CacheInterface
#9809
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
feat(cache): add deleteMatching method definition in CacheInterface
#9809
Conversation
9c79e37 to
a880f9d
Compare
|
Thank you. We will need a changelog entry for this, here: https://github.com/codeigniter4/CodeIgniter4/blob/4.7/user_guide_src/source/changelogs/v4.7.0.rst?plain=1#L43 |
|
Done, thank you for the review! |
|
One tiny request: please move the Cache entry before the Image entry so we can keep everything in alphabetical order. Thanks! |
5d59727 to
b2c0a49
Compare
|
@michalsn Done :) |
michalsn
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, thank you.
bb38faf to
babfc9c
Compare
f6ffa71 to
0b549cc
Compare
|
93653d2 to
a8ad4f0
Compare
|
I have added the native missing return types for all Also, I took the liberty to remove the I have added notes to the changelogs for both the changes. Anything else worth changing while we're at it? |
michalsn
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.
Nice catch with WincacheHandler. Can you add this under the "Bugs Fixed" section of the changelog?
|
All done! One more thing, I found a CodeIgniter4/system/Cache/Handlers/MemcachedHandler.php Lines 218 to 220 in 92100b2
The comment says that the third parameter (being the $initial_value) isn't present in other handlers. What does it mean exactly? Should it be? I'm guessing the fix should be another PR anyways? |
|
I don't use this cache, but it seems like The Yes, this should go to a different PR. |
michalsn
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.
Thank you!
paulbalandan
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.
Thanks. I think this should be 2 separate PRs: 1 for the deleteMatching method, the other for the method signature changes.
150a122 to
3f67e40
Compare
Done. Here's the new PR: #9811 I'll wait for this PR to be merged first before rebasing the other one and merging it. |
deleteMatching method definition in CacheInterface
|
Thank you, @yassinedoghri |
Description
I've added the
deleteMatchingmethod back in 2021 to delete multiple cache items based on a pattern. At that time, I didn't get the chance to add the method definition to the CacheInterface because it would introduce a breaking change for users.Being a breaking change, this PR adds the definition by targeting the next minor version branch (4.7).
This would fix any annoying error from static type checkers, like phpstan for example.
Furthermore, I've removed the base implementation from BaseHandler because all handlers have the method implemented anyways.
fixes #7828