#1748022: Make CacheBackendInterface::get() return a proper class will improve the return type from get a lot making it quite a bit easier to work with but handling cache misses in Drupal can be a bit complicated. Returning an object means you have to do some inspection and understand the object and the way caches behave when you you just want the caching system to help you avoid doing some costly action over and over. And lets all admit, if you haven't messed with caches in a while, you probably forget about the intermediate object the first time you prototype your code.
I suggest we add a helper that combines get/set into one handy function looking something like this:
$data = $this->cache->remember($cid, $expire, function () {
return 'My expensive value';
});
or to steal from the core documentation is would replace most uses of this:
$cid = 'mymodule_example:' . \Drupal::languageManager()
->getCurrentLanguage()
->getId();
$data = NULL;
if ($cache = \Drupal::cache()
->get($cid)) {
$data = $cache->data;
}
else {
$data = my_module_complicated_calculation();
\Drupal::cache()
->set($cid, $data);
}
with this:
$bin = \Drupal::cache();
$cid = 'mymodule_example:' . \Drupal::languageManager()
->getCurrentLanguage()
->getId();
$data = $bin->remember($cid, Cache::PERMANENT, function() {
return my_module_complicated_calculation();
}
This idea is shamelessly stolen from Laravel's cache repository because it simplifies so much logic out of most cache uses.
https://laravel.com/api/5.4/Illuminate/Contracts/Cache/Repository.html#m...
Discuss plan and agree if its a good idea
Decide on the interface
Write implementation
CR
Update documentation
n/a
New API. Provided as new interface for BC but maybe merged into CacheBackendInterface for 9.x?
n/a
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 3013177-12.patch | 6.23 KB | neclimdul |
| #12 | 3013177-12.interdiff.txt | 3.89 KB | neclimdul |
| #3 | cache_remember_interface.patch | 6.17 KB | neclimdul |
Comments
Comment #2
neclimdulTo open the discussion, since we use "PERMANENT" as the expiration quite often, one thing that could also be useful would be a remember permanent. It could easily be implemented like:
Comment #3
neclimdulA quick and dirty patch to kinda show how it might work.
Because contrib won't have implemented this immediately, actually using it in core will be tricky but that can be a follow up to resolve.
Comment #4
Andre-B+1 I like the style of this
Comment #5
jibranDoes it improve performance?
Comment #6
gabesulliceThere's a name for this: memoization
Should we use that terminology instead? I'm not sure, just wanted to throw it out there.
Comment #7
neclimdulI wasn't targeting performance with this idea so much as developer experience. A more straightforward interface to avoid bugs and codify the general standard for caching and checking caches.
Since the code path ends up mostly being the same I would guess performance is mostly the same but that's a very good question.
Comment #8
neclimdulI love that term and always forget about it. It feels possibly a bit opinionated about the use of the method but I don't feel strongly about the color of this bikeshed ;)
Comment #9
borisson_I really like this, the name of the method is really useful because this is easy to remember and easy to discover. This is another extra function call, so should probably be a little slower. But I personally think that the improvement in readability is a good tradeoff.
Great work!
Comment #10
hussainwebI really like the idea and was waiting for it ever since I used it in Laravel. I also like how this is done with traits that can just be
used in contrib modules.Few points for review:
CacheRememberInterfaceis incorrect.CacheBackendInterfacein Drupal 9. Can we add a todo item for that?setmethod inremembermethods as well. Currently, it is$cid, $data, $expire, $tags, but the remember method is$cid, $expire, $callback, $tags. I realize you might have done it because $callback is a closure but I'd like to consider keeping the signature along the same pattern anyway, even if code appears like this:This is not unprecedented, BTW. Functions like
array_mapare written similarly.rememberandrememberPermanentmethods.Comment #11
hussainwebAnd thanks for doing this! I have wanted this from a long time but never got around to the patch.
Comment #12
neclimdulTaking a moment to update this. A little birdie noted callable is better that \Closure for the interface so fixing that.
Re: #10 & #11
You're welcome!
1. Wrote something. I'm not the best copywriter though so if something has something better feel free to update it.
2. hmm.. Yeah sure. Need to open up an issue for that first I guess. // TODO
3. I considered that when writing the patch and don't like it. For one thing,
array_mapis reversed for the variadic of arrays you can pass. Very different. I also thinksetgot the order wrong for historical reasons and this makes more sense. Finally, I think the methods read significantly different. Putting it after a long method buries important information. Arguably tags is buried too and I'm not sure how I feel about that yet.Comment #19
neclimdulSo this still applies _but_ I've actually been using this over the years and there's a tiny short coming the in the current patch. Specifically, it requires the computing of expires and tags _before_ calling remember but you almost never want to do that. Especially with tags, its not uncommon that the tags you need to add are computed inside the closure which leads to quite a bit of trouble.
I've got a modified version of this I'm sort of using but It needs a bit of polish and this comment is mostly to provide and update but also to remind me to update the patch with a better implementation.
Side note, I _really_ which the cache DO returned from
getwas a real class and documented. it would make some hints easier and the documentation a lot clearer with the new patch.Comment #20
geek-merlinI like this a lot. This is similar to: https://www.php.net/manual/en/function.apcu-entry.php
Comment #24
neclimdulMade a library people can use to "test" this in their projects.
https://www.drupal.org/project/cache_remember
https://packagist.org/packages/drupal/cache_remember
I'm fairly happy with the current version of the methods with one exception and that's the fact its still annoying to set the expiry since we set a timestamp instead of an offset. You need to pull the time service and calculate it instead of having a central way of doing this.
Not sure if there's a good way to address this while maintaining consistency with other Drupal API's but I'm open to suggestions and we can test them out.