Make cache()->get() return a classed CacheItem object

We need to specify at least the tags array as it is specified in the cache backend interface. I'm not sure that created and expires are revelant even though they could be used in very advanced use cases (we might want to get rid of those kind of advanced usages but that's another issue).

Here is a simple PoC that needs some reflexion (see @todo's) and some review.

What I see for the future:

• Make an interface out of this class, thus backends can completely override it
• Separate and generalize checksum and tags computation in a specific subclass, and use it for both database and memory backends if possible
• Allow NULL and FALSE values, this is more a task for backends, but the class as it is now will support it
• Review :)
• Use the prototype pattern instead of instanciating objects in the backends.

Right now all properties are still public for BC, it will work gracefully with all core and contrib, in the middle or long term, I'd like to see those going out (with the interface) and live only as getters.

Any suggestions, ideas, and other rants against what I propose are more than welcome, I'd like to see this move forward in order to unblock #1651206: Added a cache chain implementation.

Here is a new version of the patch, with a serialization related problem fixed.

Here is a working implementation (should pass tests).
Added:

• Added a CacheEntryInterface interface, with no public properties
• CacheEntry class keeps the BC layer with the public properties, but they must be removed later
• Use of the prototype pattern for objects instanciation in both database and memory backends
• Cache backend tests pass

@fubhy Ready to talk

WTF testbot... Does anyone knows where it failed?

As per IRC chat with fubhy, here is a new implementation:

• Removed the serialization handling from the cache entry and moved it into the backend
• Removed the ultra generic but quite ugly populate() method and use the constructor instead
• Kept checksum property, but removed the setter in the default implementation (consider this as encapsulated and private for the database and memory implmentation)
• Cache checksum handling in array implementation was wonky and broken, deeply simplified the code and removed useless methods
• Fixed some internal PHP documentation in both memory and database backends (they probably were here since a long time! but I had to fix those methods so I fixed the PHP doc too)

All cache related tests are actually passing on my box, hope the test bot will be happy.

Ok, one last fix to do in drupal page cache handling.

• The drupal_serve_page_from_cache() is a bit tricky because it needs an stdClass as input, I had to type hint with the CacheEntryInterface interface.
• It had a nasty side effect: the drupal_page_set_cache() returns an stdClass, I had to implement a workarround using the memory backend to set and return the entry so I could fetch a CacheEntryItem. This function body does not respect its own signature and needs fixing.
• The drupal_page_set_cache() does a cache set, and need to return the entry right behind, so I adapted the CacheBackendInterface::set() method to return the newly created cache entry (it was returning nothing) and added some tests in generic backend test suite.

Here is the patch, not passing full tests on my box, but I'm suspicious about my environment, I think it alters the header sent (not sure, but it might). At least there is no fatal anymore.

Ok, so page cache is fixed, but it happens that user input or database record always contains strings instead of integers, hence the test fails. Added a few int casts for fixing this, and it should work.

I also reverted the time() call to REQUEST_TIME per fubhy's request, even if I personally prefer to use time() to avoid expiration delay latency due to long requests, but that will be a topic for a follow-up.

+++ b/core/lib/Drupal/Core/Cache/CacheEntry.phpundefined
@@ -0,0 +1,115 @@
+  public function __construct($cid, $data, $created, $expires, $tags = array(), $checksum = null) {

Just in order to get this in-line we should use the static CACHE_PERMANENT as a default for expires here as well. Even if it is redundant.

+++ b/core/lib/Drupal/Core/Cache/CacheEntry.phpundefined
@@ -0,0 +1,115 @@
+    return time() > $this->expires;

Whether or not we use REQUEST_TIME or time() this is wrong. As of now we have been using ">=" for this stuff (the DatabaseBackend still does that) - And I think that's also more accurate.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -85,34 +85,33 @@ class DatabaseBackend implements CacheBackendInterface {
+   * @param stdClass $record

This is not a stdClass anymore.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -85,34 +85,33 @@ class DatabaseBackend implements CacheBackendInterface {
+  protected function prepareItem($record) {

Should require CacheEntryInterface here too.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -122,7 +121,8 @@ class DatabaseBackend implements CacheBackendInterface {
-      'expire' => $expire,
+      // User code often gives us strings instead of integers.
+      'expire' => (int)$expire,

Is that really required? After all we are doing a db insert on an integer field here so that should happen anyways. It doesn't do any harm... Just wondering why you added it here :).

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -140,9 +140,12 @@ class DatabaseBackend implements CacheBackendInterface {
+      return new CacheEntry($cid, $data, $fields['created'], $expire);

Just a nit-pick... but we could directly access REQUEST_TIME here too instead of pulling it from the $fields array :P.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -140,9 +140,12 @@ class DatabaseBackend implements CacheBackendInterface {
       // The database may not be available, so we'll ignore cache_set requests.

Aww, we have to clean up the cache-related code at some point and remove all those references to removed/deprecated functions.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -67,51 +75,20 @@ class MemoryBackend implements CacheBackendInterface {
+    // User code often gives us strings instead of integers.
+    return $this->cache[$cid] = new CacheEntry($cid, $data, REQUEST_TIME, (int)$expire, $tags, $this->checksum($tags));

I really wonder if that is actually our problem :P.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -67,51 +75,20 @@ class MemoryBackend implements CacheBackendInterface {
+    foreach($this->flattenTags($tags) as $tag) {

I already spoke to pounard about this on IRC: Now that we have a getTags method in the CacheEntry interface... We could move the flattenTags method over to the CacheEntry class (doesn't have to be in the interface.). Same with the checksum and hasInvalidatedTags(). We should open follow-up issues for that.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -167,17 +144,14 @@ class MemoryBackend implements CacheBackendInterface {
+   * @param object $tags
+   *   The entry tags array.

It's not a plain object and it's not an array.... It's a CacheEntry object!

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -167,17 +144,14 @@ class MemoryBackend implements CacheBackendInterface {
+    return $entry->checksum != $this->checksum($entry->getTags());

Ahh, that's much better.

Just in order to get this in-line we should use the static CACHE_PERMANENT as a default for expires here as well. Even if it is redundant.

Yes we can.

Whether or not we use REQUEST_TIME or time() this is wrong. As of now we have been using ">=" for this stuff (the DatabaseBackend still does that) - And I think that's also more accurate.

Makes sense.

This is not a stdClass anymore.

Actually it is, the prepareItem() method gets the database row and returns the CacheEntry.

Should require CacheEntryInterface here too.

Nope, see upper entry.

Is that really required? After all we are doing a db insert on an integer field here so that should happen anyways. It doesn't do any harm... Just wondering why you added it here :).

It also cast the structure which is reused below in the code in order to return the newly created cache entry.

Just a nit-pick... but we could directly access REQUEST_TIME here too instead of pulling it from the $fields array :P.

Nope, this would be conceptally wrong, if we change the REQUEST_TIME to time() upper, we'd also have to change it here, while with the current code, if I change the REQUEST_TIME uppre with time() I won't have to change this line too!

I really wonder if that is actually our problem :P.

Indeed it isn't our problem, yes, but this quick fix allows some tests to pass, and I'm OK wih keeping it this way, this doesn't hurt.

I already spoke to pounard about this on IRC: Now that we have a getTags method in the CacheEntry interface... We could move the flattenTags method over to the CacheEntry class (doesn't have to be in the interface.). Same with the checksum and hasInvalidatedTags(). We should open follow-up issues for that.

I am definitely OK with that!

It's not a plain object and it's not an array.... It's a CacheEntry object!

I can't answer to this one, the patch doesn't seem to apply on my d8 copy anymore, but you're probably right.

Ahh, that's much better.

It is!

Taking into account various #14 concerns.

Also specialized exception catching in database backend: non database related exceptions should always pass, not been silentely catched.

@{#!^$$-#!!& WHY BUT WHY does the testbot always reject branches diff?

Manually fixed patch.

Various fixes:

• Changed a do { /* ... */ } while(count($cids)); to a direct while(count($cids)) { /* ... */ } because the input cid array can be empty.
• Unified invalidated tags handling of memory and database backend. Code is still duplicated in both backends but all wrong static properties have been removed.
• Fixed minor wrong direct entry property usage in database backend, usage of CacheEntryInterface instead.
• Added some missing $tags parameter pass into CacheEntry::__construct() when returned on DatabaseBackend::set() calls.

What I didn't do is moving checksum computating into CacheEntry default implementation: currently, database backend needs those helpers in order to do database operations, that the memory backend actually don't need at all. This is uneasy to generalize right now. I will try later.

I would like to assign this to catch for review.

Here you go :)

Thank you!

Why not $cache->created() or $cache-getCreated()?

Forgot the call to the accessor here, you asked the question about naming lower :) Needs to be fixed.

This is a bit bizarre. Why can't the cache object creation instantiate its own CacheEntry instance up top instead of doing it by proxy via the cache backend.

As designed the default implementation is only meant to be instanciated by the backend, it never has been meant to be arbitrarily instanciated anywhere. I know the workaround is bizarre, but this function code does seem to have some historic code issues. Right now, instanciating a CacheEntry object outside of a backend class would be a separation of concerns and encapsulation violations.

I know pounard won't like this comment but all these are too verbose. ->getCreated() ->getExpires() ->getId() would be plenty.

Actually I'm not against those changes, I just did put extremely long names waiting for the comments :) As long as the method names are explicit enough I'm OK for shorter. I will fix that and use your names proposals if that's OK with everyone else following this issue.

Please revert this. Tags can be checked across multiple cache bins, which will be different instances of the storage class, there's a good reason for the static cache here - so we don't check the same tags against multiple different bins during a request. Also it's completely unrelated to this issue.

It sounds like a bug to me. Why would you share tags invalidation to multiple backends? If so, this should be moved out of the cache backend class. Code as it is will share tags among all database backend but won't for the other backends: this sounds like a unpredictable and random behavior and raises a red flag in my mind. I will revert it as you wish but I'll open a follow up.

This is completely unrelated to the issue as well.

I do not agree, I should have documented it, but from what I remember I did this because there was some non-database related exception catched in some edge cases while testing: I have fixed those but in order to find it out, I had to remove the excessive exception catching, if we revert back, it may happen again. I will revert it, but this try/catch statement must definitely be removed one way or another.

Ok I see, this is a purely technical workaround to the fact there is only one tags table. So yet it is technically important that this value remains shared. But I see as side effect potential tags clear on backends that should not invalidate: it may cause excessive cache clears?

I was only speaking about the database backend, of course. Now there is something that bothers me: the actual code is indeed the most performant one since the cache_tags table is shared (it will avoid additional queries on cache tags invalidate operations) but if I have the same tag used in two different bins, and I invalidate it in the first one, the tag checksum will be invalidated into the second one.

Today the CacheBackendInterface::invalidateTags() method does not specify or relates this behavior. It will not create any bugs because cache items are meant to be arbitrary invalidated, but it may proceed to excessive invalidations on some sites.

So there is a point of untying tags for all database bin: avoiding useless tags invalidations.

Nevertheless you are right about my code: it's not sufficient alone, untying would also need to add a bin column in the cache_tags table. This would probably make update and insert operations slower, but those operations are done only on set() and invalidateTags() calls, it's not supposed to affect the get() operation.

Today it is affecting the get() op because items are lazy invalidated at get() time by by computing the checksum, but this is bad because when an item has one or more tag, you actually triggers two SQL queries instead of one when fetching it (and it's not even factorized in getMultiple() which could have been optimized into 2 SQL queries, while today it's N + 1 queries, where N is the number of items having tags).

If the field API starts to use the tags on field cache, a lot of sites are going to die in an extremely painful manner when it will try to display 20 contents in a single view (which make it being 40 entities if you are using Commerce, which is a concentrate of performance happiness).

EDIT: Just as an example, imagine displaying 20 commerce products, so you'd also display 20 nodes, and let's say you have an average of 10 fields each bundle (which is a small number for a lot of sites) you'll then do:
- 1 query for node load multiple
- 1 query for field cache multiple
- 10 queries for each cached entry with tags for invalidation
- 10 other queries for each product load (which are not factorized)
- 2 * 10 cache queries for product fields (10 for invalidation)
Which is a total of 42 queries.
While a normal D7 runtime would do:
- 1 query for node load multiple
- 1 query for field cache multiple
- 10 other queries for each product load
- 10 cache queries for product fields
Which is a total of 22 queries, a lot less.

There are in fact quite a few things that we need to clean up in the new Cache API and the backends. We should set up a new META issue for a second iteration of #81461: Clean up the cache API (stop overloading function arguments, remove procedural wrappers). I will do that within the next couple of hours.

#30 Agree, let's proceed with the cache entry object only.

Let's try and revive this issue, here is a (serious) reroll to get us started. I haven't tested yet, but should get us back in motion.

+++ b/core/includes/bootstrap.incundefined
@@ -1229,8 +1230,11 @@ function drupal_page_header() {
+ * @param Drupal\Cache\CacheEntryInterface $cache

Missing "\" and it uses the wrong namespace.

+++ b/core/includes/bootstrap.incundefined
@@ -1242,13 +1246,15 @@ function drupal_serve_page_from_cache(stdClass $cache, Response $response, Reque
+  $cache_data = $cache->getData();
+
+  foreach ($cache_data['headers'] as $name => $value) {

This seems to be a worse DX, can't we have a key on getData maybe? (just throwing out some ideas)

+++ b/core/includes/bootstrap.incundefined
@@ -1242,13 +1246,15 @@ function drupal_serve_page_from_cache(stdClass $cache, Response $response, Reque
-      $response->headers->set($name, $value);
-      unset($cache->data['headers'][$name]);
+      drupal_add_http_header($name, $value);
+      unset($cache_data['headers'][$name]);

Oh, we don't use the response object anymore?

+++ b/core/includes/bootstrap.incundefined
@@ -1261,6 +1267,8 @@ function drupal_serve_page_from_cache(stdClass $cache, Response $response, Reque
+  $etag = '"' . $cache->getCreationTimestamp() . '-' . intval($return_compressed) . '"';
+  header('Etag: ' . $etag);

What about setting this on the response object as well?

+++ b/core/includes/common.incundefined
@@ -3197,9 +3199,14 @@ function drupal_page_set_cache(Response $response, Request $request) {
+      // @todo This is a workarround that allows to set the CacheEntryInterface
+      // type hint in the drupal_serve_page_from_cache() function.
+      $backend = new MemoryBackend('foo');
+      return $backend->set($cache->cid, $cache->data, $cache->expire, $cache->tags);

Can't we also just create a manual instance of the cache entry object?

+++ b/core/lib/Drupal/Core/Cache/CacheEntry.phpundefined
@@ -0,0 +1,127 @@
+  public $data;
...
+  public $tags = array();
...
+  public $created;
...
+  public $expire;
...
+  public $cid;
...
+  public $valid;

So these are public to not construct an api change?

Updated issue summary.

I'd like to resurrect this. After re-rolling and briefly talking about it in IRC, @Crell pointed me to the PSR-6 proposal that also includes a Cache\ItemInterface. Apparently, the proposal there reflects a lot of the ideas that I had in mind myself.

I think we can vastly improve the DX of working with cache items. Instead of designing around technical aspects, we want to design a nice API for consumers.

Attached patch simplifies the 99% use-case in all code to this pattern:

$item = cache('bin')->get('key');
if ($item->isHit()) {
  $value = $item->get();
}
else {
  $value = ...;
  $item->set($value);
}
return $value;

Thoughts?

It can be even simpler:

$item = cache('bin')->get('key');
if (!$item->isHit()) {
  $value = ...;
  $item->set($value);
}
return $item->get();

Yeah, that's another way to do it, which should be possible with this patch already.

Merged 8.x.

Updated cache usage in simpletest.module.

Returning a value item seems nice, but I don't see the use for a null object here if we're gonna call a isHit() method anyway. Why not continue to return null or false on the backend get() method instead of a value object in case of no hit? The value object is a good idea but I think having a null object with a partial active record pattern on it is confusing on whose responsability on the CRUD: the backend gets the items but the items save themselves?

It's a personal opinion not gratuitous criticism, and an open question, but I don't see the benefit of this:

$item = cache('bin')->get('key');
if (!$item->isHit()) {
  $value = ...;
  $item->set($value);
}
return $item->get();

over this:

if ($item = cache('bin')->get('key')) {
  return $item->getValue();
}
$value = ...
cache('bin')->set('key', $value);
return $value;

Even if it looks shorter (which is not by the way).

Ideally I'd prefer to have a higher level API such as this:

return cache('bin')->getOrBuildValue('key', function () {
  $value = ...
  return $value;
});

Which would hide the complexity for me, and almost nullify the need of even having a value object in the end.

Note that I'm not against this, but I don't see the benefit of PSR-6 at all in the Drupal case since it has its already more-than-complete API for this. Let's not forget the original reason and benefit of this thread is too have a documented object propertie set, in order to ensure that all backend return normalized and consistent entries, and thus ensures the interoperability of those backends. Let's just focus on that?

Re-rolling and touching up.

• I improved the documentation of CacheItemInterface.
• I added a template for a PHPUnit test of CacheItem. All test methods fail, because I simply haven't filled them in yet.
• I fixed a number of other test failures that were the result of incomplete conversions. There are still a lot left, though.

I am pondering whether we want to rename CacheItemInterface::get() to CacheItemInterface::getData(), because we have so many other getters now as well. The method name needs to be self-explanatory.

Returning a value item seems nice, but I don't see the use for a null object here if we're gonna call a isHit() method anyway.

Even though I did not address this in my last patch, I share this concern. If there is no data, there are also no tags, no key, no creation and expiration timestamps...

So if you share my concerns why did you actually reroll the patch with the isHit() method and all?

Because I may not have read the entire issue and realized isHit() may be totally useless only after a few conversions (ssssht, don't tell anyone!)

The patch also contains some other useful changes, though, so it wasn't entirely in vain.

We discussed returning NULL at length in the FIG. Long story short, NULL is a valid value to cache. Always returning a cache object means we don't have to have a sentinel value for "no hit", which we want to avoid.

#50 This argument is invalid, it does not matter if null is a valid value or not, if your method signature is returning an object carrying the value, if you return null instead because there is no cached item instead of an object here is no ambiguity here and null can still be a valid value.

#50 This argument is invalid, it does not matter if null is a valid value or not, if your method signature is returning an object carrying the value you can return null when there is no cached item instead: there is no ambiguity and null can still be a valid value carried by the object when returned.

isHit() actually does not have any added value in Drupal, IMHO, at all; Except if you want to implement a null object pattern or add this incomplete active-record pattern. An incomplete active record pattern does not make sense because it does not apply to multiple operations thus batlantly ignoring 50% of use cases, and the backend already handles everything for us. This implies that for we'd have to implement different concrete code for the same use case depending on it's multiple or not; Which will end up being more confusing to developers than it brings actual DX.

Because when you fetch a cached item you always want to implement a counterpart when the item does not exist (which actually reprensent the original and truth algorithm of the function itself) the null object has no sense at all: null object solves problem of chaining calls on objects when they don't exist, and thus by extension avoid breaking the application on a critical path (mostly UI) where the user must be able to recover from the error. Here cache fetch is not an error case and makes the null pattern null: it is a normal behavior (actually reprensents the normal behavior of the application since it's mandatorily working without cache at all).

What would really be a value added here would be a even higher API. The cache item interface has no other mean, in our context, than documenting its properties and ensure all backends set them up right for the user to have no surprises.

While I can partially see the architectural arguments being made, I'm a fan of designing APIs for their primary target audience; their consumers. Sadly, we're doing that way too rarely, and especially in D8, developers are exposed to and confronted with a lot of internal complexity that wasn't really designed for frequent usage.

The primary benefit from my perspective is entirely focused on DX:

The API for working with cache items has always been troublesome for developers, in particular due to the different signatures of CacheBackend::get() and CacheBackend::set(), which were slightly improved to be more consistent already, but as a developer, I still feel that too much internal complexity is thrown and enforced on me.

Especially because the first and foremost use-case is to (1) check a particular cache backend for a certain key and (2) instantly rebuild and set on miss, I really enjoyed the simplicity and DX of simply acting on the cache item only, once instantiated:

  $item = cache('bin')->get('key');
  if (!$item->isHit()) {
    $value = ...;
    $item->set($value);
  }
  return $item->get();

As an API consumer, it doesn't make sense to me to have to re-state once more that I want (1) a cache backend (2) to store a cache item with a value for (3) a particular key:

  $item = cache('bin')->get('key');
  if ($item) {
    $value = $item->get();
  }
  else {
    $value = ...;
    cache('bin')->set('key', $value, $ttl, $tags);
  }
  return $value;

If you just briefly scan through this patch, then you'll find a lot of instances in which the consuming code is greatly simplified and prettified, because it no longer needs to (1) backup $cid in a separate variable for complex keys nor (2) duplicate the code for the cache bin/backend and key. As a developer, I enjoy to write simple and pretty code. :)

In fact, if you complete the counter-argument to its full extent, then why does CacheBackend::get() return a cache item/object in the first place, instead of just the value?

The answer is, because consumers need to be able to differentiate between a cache hit and a cache miss (whereas both NULL and FALSE could be legit cached values), and also, because there are advanced use-cases in which consumers need to know the cache item's expiration date.

So if we're returning a cache item on hit/success, why not on miss/failure?

Before I went ahead to adjust all callers, I researched whether there is any smart/creative way to allow a polluted object to be cast-compared to FALSE. I looked into magic __isset and __call methods, Serializable, and also grepped the net, but wasn't able to find one. I would have certainly avoided/omitted that change otherwise.

In addition, I further simplified and improved DX by exposing two discrete setter methods on CacheItem for the two primary use-cases in our code base:

1. Time/TTL-based cache items

   → most often permanent + manually invalidated.

     $item->set($value, $ttl = Cache::PERMANENT);
   

2. Tagged cache items

   → usually permanent + valid until tags are invalidated.

     $item->setWithTags($value, $tags, $ttl = Cache::PERMANENT);
   

And yes, sure, that could also be done to CacheBackendInterface, but (1) having just the atomic get($key) and set($key, $value) methods there feels natural/OK to me and (2) I'd still have to re-retrieve the cache backend by bin and re-specify the cache item key in consuming code.

I think our primary focus should be DX. The isHit() method may feel new, but in terms of DX, it is much more clear than our existing !$cache checks. In fact, if you study the patch some more, you will find a few instances that wrongly checked for a cache miss via !isset($cache) and in other creative ways. → The current API is not clear, isHit() is 100% clear.

While I can partially see the architectural arguments being made, I'm a fan of designing APIs for their primary target audience; their consumers.

Cache backends are already an API, and a quite really simple one, this is the best DX you can have. And your proposed API actually targets only 50% of the audience because you totally ignore the multiple operations.

Sadly, we're doing that way too rarely

I do have to agree with you, but cache backend is already an API and is not complex.

The API for working with cache items has always been troublesome for developers, in particular due to the different signatures of CacheBackend::get() and CacheBackend::set()

What? Those two methods are pretty clear about what they do. The only thing I don't think handy in this is tags not being an documented argument of set() method.

Especially because the first and foremost use-case is to (1) check a particular cache backend for a certain key and (2) instantly rebuild and set on miss [...] As an API consumer, it doesn't make sense to me to have to re-state once more that I want (1) a cache backend (2) to store a cache item with a value for (3) a particular key

What? This does not make any sense. Your patch sure propose a certain level of shortcuts; But in the end repeating stuff is not bad, on the opposites it makes clearer who is responsible for storing: the backend. It makes code actually clearer to repeat.

In fact, if you complete the counter-argument to its full extent, then why does CacheBackend::get() return a cache item/object in the first place, instead of just the value?

The answer is, because consumers need to be able to differentiate between a cache hit and a cache miss (whereas both NULL and FALSE could be legit cached values), and also, because there are advanced use-cases in which consumers need to know the cache item's expiration date.

Actually this statement makes absolutely no sense if your comment is answering to mine: the cache backend interface already handles everything, including null/fase values. With or without your isHit() method.

In addition, I further simplified and improved DX by exposing two discrete setter methods on CacheItem for the two primary use-cases in our code base:

So you actually splitted the set() method as multiple methods. Why TTL is always an argument while tags is not? This is actually inconsistent.
To solve this backend set() method signature should be set($key, $value = null, $ttl = null, string[] $tags = null) instead. Even though you're trying to make it better on the value, the way you choose here is even more inconsistent, and there should be only one set() method with those three arguments.

And yes, sure, that could also be done to CacheBackendInterface, but (1) having just the atomic get($key) and set($key, $value) methods there feels natural/OK to me and (2) I'd still have to re-retrieve the cache backend by bin and re-specify the cache item key in consuming code.

Please don't use that arguments, it makes no sense. Everything can look ulgy depending on how you write it. I could actually write your second code sample this way (considering we're in an object in the most common use case):

  if ($item = $this->cache->get("my_key")) {
    return $item->getData();
  }
  $value = ... ;
  $this->cache->set("my_key", $value, 12345456, array("test", "foo"));
  return $value;

Which would be in procedural code something like:

  $cache = \Drupal::cache("bin");
  if ($item = $cache->get("my_key")) {
    return $item->getData();
  }
  $value = ... ;
  $cache->set("my_key", $value, 12345456, array("test", "foo"));
  return $value;

And it doesn't look as bad as you state it is, and in this code, the responsability always belong to cache backend, instead being once on the backend, then on the item.

--

DX is not about reapeating or not two variables, it's about consistency and readability, with or without the isHit() method, readability is the same. It's IMHO, but it depends on people I guess, even less clear: when you return an object which can be a representation of a non existing item, this sounds quites confusing.

Whereas I really appreciate the effort, I do, I don't think the API you are proposing is good enough, and while it does bring some code shortcuts (whereas it doesn't solve any problem) it doesn't bring the same shortcuts for the the multiple operations, and thus make the code diverge, adds one more way to do things, and will probably confuse more new developers that it will make the code readable. Especially when you consider that a lot of people will still use the backend directly (which is legit and valid), others will use your value wrapper, others will do it totally differently because they have more complex needs, etc... There's many ways to skin a cat, but let's not confuse people by propose one more, which is neither universal neither evident.

[this thread aside] And if you want my opinion about the PSR-6 proposal (which is way out of scope of this issue actually) I think that the proposed interfaces are not really good: having only one backend interface carrying all the needed method (more like actual Drupal cache backend) would be way better; This way backend implementors could rely on a single backend interface and a generic value object implementation to write a fully function backend, instead of having to implement both. But that's another issue, let's leave FIG solve this and decide [/this thread aside]

So, the result of all those discussions was that exactly nothing happened :)

PSR-6 happened, but our cache API is way, way beyond what that is defining.

There is exactly one possibility to still make this happen and considerably improve DX for everyone using caching.

We introduce CacheItem with public properties, so that it is backwards compatible and also add methods. Then, in Drupal 9, it will be easy to make them protected and force that methods are used.

I'm assigning to myself for now, Xano, come and poke me if you don't agree

Ok talked with Berdir about this. Im going to start all over again and we are going for the minimal solution.

Get
- CacheItem in
- CacheItemInterface in

And convert the backends.

Ok this version actually doesn't fatal on my local machine

Ok patch has an extra file that sneaked in...

I needed some help from Berdir to figure out what was happening. Lets see if we get less fails now.

I already noticed we are missing the "valid" kay on the object?

Hopefully this one is green.
So it works for the databasebackend.

And this one should work for all the backends.

So close! :)

1. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,221 @@
   +  /**
   +   * The cache backend of this cache item.
   +   *
   +   * @var \Drupal\Core\Cache\CacheBackendInterface
   +   */
   +  protected $cache;
   

   As discussed, the isHit stuff is not actually implemented right now and I'm also not convinced about adding those additional helper methods here, seems like a separate issue to consider that so that we can get this in as an as small as possible patch?

2. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,221 @@
   +   *
   +   * @var array[]
   +   *   The entry cache tags. First dimension of the array are key value pairs,
   +   *   keys being the business name of tag (e.g. "node") and value is an array
   +   *   of tag values (e.g. "1" for node 1).
   +   */
   +  public $tags = array();
   +
   +  /**
   +   * The current checksum.
   +   *
   +   * @var int
   +   */
   +  public $checksum = 0;
   +
   

   This is old, tags are just strings now. Probably copy from set() or so.

   Especially checksum and possibly also the stored tags are an implementation detail, not all backends might return them.

3. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,221 @@
   +   * {@inheritdoc}
   +   */
   +  public function get() {
   +    return $this->data;
   +  }
   

   Not sure if this should be getData() ? Otherwise we basically have $backend->get()->get() now..

4. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -100,7 +100,7 @@ public function getMultiple(&$cids, $allow_invalid = FALSE) {
        foreach ($cids as $cid) {
          if ($item = $this->get($cid, $allow_invalid)) {
   -        $ret[$item->cid] = $item;
   +        $ret[$item->cid] = new CacheItem($this, $item->cid, $item->data, $item->expire, $item->tags, $item->created, $item->checksum, $item->valid, TRUE);;
          }
   

   We might be able to just write CacheItem objects here, at least if we remove the $cache/isHit stuff.

1. fixed
2. fixed
3. fixed
4. What do you mean by that.

New patch major cleanup, now that I better understand how it all works.

Alexpott asked for some profiling.
As I'm a total profiling noob I tried this minimal approach by hacking a test.

All my runs printed a result between 2.1 and 2.3 seconds.
I didn't notice any difference between the stdClass and the new CacheItem class.

This one should be 100% fully BC

And even less changes

This looks much simpler than before, very nice :)

1. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,177 @@
   +/**
   + * Provides a default cache item.
   + *
   + */
   

   Line 13 can be deleted.

2. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,177 @@
   +class CacheItem extends \stdClass {
   

   This is interesting :D

   Is this intentional, for BC or something?

3. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,177 @@
   +   * The key of this cache item.
   ...
   +   *   The item's unique key within the cache bin it was stored.
   ...
   +   * Returns the key for the current cache item.
   ...
   +   *   The key string for this cache item.
   ...
   +  public function getKey() {
   

   s/key/cache ID/

4. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,177 @@
   +  public $expire = 0;
   

   Shouldn't this default to CacheBackendInterface::CACHE_PERMANENT?

5. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,177 @@
   +   * @var array[] $tags
   

   s/array[]/string[]/

6. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,177 @@
   +   *   An associative array of cache tags whose keys are the cache tag names and
   +   *   whose values are arrays containing the tag values.
   ...
   +   * @param array[] $tags
   +   *   An associative array of cache tags whose keys are the cache tag names and
   +   *   whose values are arrays containing the tag values.
   

   This is also wrong/outdated (probably leftover from the initial patch, it used to be correct at some point).

   Look at the docs for CacheBackendInterface.

7. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,177 @@
   +  public $tags = array();
   

   s/array()/[]/

8. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,177 @@
   +   * Constructs a new class instance.
   

   s/class instance/CacheItem/

9. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,177 @@
   +   * Retrieves the value of the item from the cache associated with this objects key.
   

   80 cols.

10. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
    @@ -0,0 +1,177 @@
    +   * Gets cache entry creation Unix timestamp.
    ...
    +   * Gets cache entry expiration Unix timestamp.
    ...
    +   * Is this entry expired.
    

    s/entry/item/

11. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
    @@ -0,0 +1,177 @@
    +   * The computed return is based upon the local site time (depending on the configured locale) which means that the
    +   * entry may already have expire or is still valid in the backend while this tells the opposite.
    

    80 cols.

12. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
    @@ -0,0 +1,177 @@
    +   *   An associative array of cache tags whose keys are the cache tag names and whose values are arrays containing the
    

    80 cols.

1. fixed
2. yes AlexPott asked for it, in case someone assumes it is a stdClass at the moment
3. fixed
4. fixed
5. fixed
6. fixed
7. fixed
8. fixed
9. fixed
10. fixed
11. fixed
12. fixed

+++ b/core/lib/Drupal/Core/Cache/CacheItem.php
@@ -0,0 +1,179 @@
+class CacheItem extends \stdClass {
+
+  /**
+   * The key of this cache item.
+   *
+   * @var string
+   */
+  public $cid;

I'm assuming the public properties in this class are for BC reasons, since we've eschewed them elsewhere. We should document that in the class docblock so people don't get confused and try to figure out when public properties are OK and when they're not. (In this case, they're not OK, just a BC shim.)

Yes the only reason this will ever get into 8.0 is if we keep it 100% backwards compatible.
So in this case we have to make them public.
I'm un assigning this issue from myself because I'm done sprinting.

So I guess we only need an extra docblock here and some extra more professional profiling.

RE: #83.2 + #84: could you document that we extend \stdClass and public properties to not break BC, and that those things should be fixed (no extending, no public properties) in 9.0?

Added information in the class docblock to inform about the extension of stdClass.

Let's add an interface so we can type hint on that.

This is silly. I now realize that the interface was removed in #79. @aspilicious: what was the reason for that?

//Edit: Probably because that patch changed CacheItem from implementing CacheInterface to extending \stdClass. However, we can do both in the default implementation, so code can be changed to work with the interface only, making it more forwards-compatible.

1. +++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.php
   @@ -38,7 +38,7 @@
   +   * @return \Drupal\Core\Cache\CacheItem|false
   
   @@ -60,7 +60,7 @@ public function get($cid, $allow_invalid = FALSE);
   +   * @return \Drupal\Core\Cache\CacheItem[]
   
   +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,158 @@
   +class CacheItem extends \stdClass implements CacheItemInterface {
   

   Even I don't get the point of the interface, in case there is one, we should use it in the documentation. On the other hand the Cache item is IMHO a value object, that is not part of our extensibility

2. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,158 @@
   +      return time() >= $this->expire;
   

   Should we better use REQUEST_TIME?

+++ b/core/lib/Drupal/Core/Cache/CacheItem.php
@@ -0,0 +1,158 @@
+      return time() >= $this->expire;

Fixed the issue point no 1 on #92

For point 1 I am agree with

//Edit: Probably because that patch changed CacheItem from implementing CacheInterface to extending \stdClass. However, we can do both in the default implementation, so code can be changed to work with the interface only, making it more forwards-compatible.

on #91

@

+++ b/core/lib/Drupal/Core/Cache/CacheItemInterface.php
@@ -0,0 +1,75 @@
+/**
+ * Defines a cache item.
+ */
+interface CacheItemInterface {

IMHO this is not really helpful information

I did a google search for PSR6 and Drupal and this was the only Drupal issue that came up.

https://github.com/php-fig/fig-standards/blob/master/proposed/cache.md

The standard recently passed. How does it impact us?

(I probably should create a new issue, right?)

It does not impact us at all, as we are not required to use it, and we cannot refactor our Cache API at this point in the release cycle. Sometime someone may, in core or contrib, write an adapter between our Cache API and PSR-6.

I'd just like to clarify what @catch said: PSR-6 is not an "all or nothing" feature. If we decide that it is useful, we can extend on it. We can even publish additional interfaces of our own to support our caching trinity (tags, contexts, max age).

Based on what I've seen, a big difference between our and their API is that they always return an object and you have to check that with isHit()? Correct me if I'm wrong, but that behavior is incompatible with what we are doing and there's no adapter or other BC thing that can handle that.

An adapter from our caching system to a PSR-6 backend could simply return NULL if ::isHit() returns FALSE?

catch is correct, PSR-6 in its vanilla form is not directly usable by Drupal's cache system. We would, at minimum, need to extend it with a set of tagging interfaces; there's an example of that in the meta document, in fact. But yes, it would not be a drop-in replacement for our current cache system. That's probably a 9.x task.

Providing a PSR-6 front-end to our current cache system should be reasonably straightfoward, I think, if someone were inclined to do so. That's probably best started as a stand-alone module for the time being, and possibly moved into core later if we find it useful or add in a library that depends on it.

Integration with PSR-6 should be something we discussed in the future, if that is still something that we want to do. I rerolled the patch and fixed #94 and #91.

Adds some unit test coverage + use the Time service. This should probably be injected, but I'm not sure how we can accomplish that.

Isn't this a BC break during D8?

Because the CacheItem object extends \stdClass, this isn't a BC break.

You asked the same thing in #82. Responses in the few comments after that explain that this is not a BC-break because of that.

Well, that was >3 years ago, and before D8.0.0 was released :D

And thanks, that makes sense! But this is in an area that we need to be extra careful in, so it's possible that core committers will still decide we can't do this. Let's get a framework manager to provide feedback.

BC is a complicated topic.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.php
@@ -33,7 +33,7 @@
    *
-   * @return object|false
+   * @return \Drupal\Core\Cache\CacheItemInterface|false
    *   The cache item or FALSE on failure.
    *
    * @see \Drupal\Core\Cache\CacheBackendInterface::getMultiple()
@@ -55,7 +55,7 @@ public function get($cid, $allow_invalid = FALSE);

@@ -55,7 +55,7 @@ public function get($cid, $allow_invalid = FALSE);
    *   "valid" property of the returned objects indicates whether the items are
    *   valid or not. Defaults to FALSE.
    *
-   * @return array
+   * @return \Drupal\Core\Cache\CacheItemInterface[]
    *   An array of cache item objects indexed by cache ID.

While this might not be a BC change for anyone who is *calling* those methods, it is for alternative cache backend implementations.

If anyone now relies on this object being returned and switches the cache backend, they have a fatal error.

I'm not sure how we can deal with that, other than documenting that both are valid return values, which means that nobody must use the methods. Which means maybe we should not (yet) add them and just have the public properties? In 9.x, we can make it a requirement to return a CacheItem and at that point, also add methods?

I'm not sure how we can deal with that, other than documenting that both are valid return values, which means that nobody must use the methods. Which means maybe we should not (yet) add them and just have the public properties? In 9.x, we can make it a requirement to return a CacheItem and at that point, also add methods?

That might be a good idea, or we can mark the methods as @internal to make sure that people know that can't or shouldn't use those methods yet. (Is there an @future or something we can tag these with?)

One alternative we could do is to wrap cache backends with a decorator which ensures that always CacheItem ids are returned. On top of that there could be something like an interface/service flags which indicates that a specific cache backend doesn't actually need this BC layer, as it already returns CacheItem objects.

That sounds like a very nifty solution. I'm not sure how we should go about doing that.

I'm been trying to make some prototype. It might work, it might not work :P

I think the serviceprovider doesn't have the correct use statement? That seems to be why the tests are failing.

I do like your solution, but I'm afraid that will slow down getting an item from the cache, since this is in such a critical path, I'm not sure if that's a good idea.

Well, when you tag your cache backend there should be no difference at all, which might be acceptable?

@borisson_ Yeah this was indeed one of the 2 problems.

Oops, calling itself is a bad idea.

Some review comments:

1. +++ b/core/lib/Drupal/Core/Cache/CacheBackendPass.php
   @@ -0,0 +1,23 @@
   +/**
   + */
   +class CacheBackendPass implements CompilerPassInterface {
   

   Missing class descriptions

2. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -33,6 +33,11 @@ class CacheFactory implements CacheFactoryInterface, ContainerAwareInterface {
   +  /**
   +   * @var string[]
   +   */
   +  protected $cacheBackendsWithNonLegacy;
   

   Missing variable description.

3. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -41,10 +46,13 @@ class CacheFactory implements CacheFactoryInterface, ContainerAwareInterface {
   +   * @param array $cache_backends_non_legacy
   +   *   (optional)
   

   Missing parameter description.

4. +++ b/core/lib/Drupal/Core/Cache/CacheItem.php
   @@ -0,0 +1,153 @@
   +  public function isExpired() {
   +    if (CacheBackendInterface::CACHE_PERMANENT === $this->expire) {
   +      return FALSE;
   +    }
   +    else {
   +      return \Drupal::time()->getCurrentTime() >= $this->expire;
   +    }
   +  }
   

   Unnecessary use of else

5. +++ b/core/lib/Drupal/Core/Cache/CacheItemCacheBackendDecorator.php
   @@ -0,0 +1,138 @@
   +class CacheItemCacheBackendDecorator implements CacheBackendInterface {
   

   Missing class description

6. +++ b/core/lib/Drupal/Core/Cache/CacheItemCacheBackendDecorator.php
   @@ -0,0 +1,138 @@
   +  /**
   +   * @var \Drupal\Core\Cache\CacheBackendInterface
   +   */
   +  protected $cacheBackend;
   

   Missing variable description

7. +++ b/core/lib/Drupal/Core/Cache/CacheItemCacheBackendDecorator.php
   @@ -0,0 +1,138 @@
   +  protected function wrapResult($cid, $result) {
   

   I guess this function should return FALSE when no result is found. See CacheBackendInterface::get()

   Missing function description.

Thank you to @Berdir and @borisson_ to figure out the last test failure with me

This time with patches

Looks like there is another failure in that same test. It was green when we tested it manually on your machine yesterday iirc. I have no idea why this is still failing, should we implement the __call that we discussed yesterday - does that make a difference?

We should also document the new reset method.

So... lots of changes.

* The test fail was caused by the decorator not implementing CacheTagsInvalidatorInterface interface/methods, the reason I think only this specific test was failing on that stuff is that afaik that user permission thing is practically the only use of the cache.static cache bin we have atm.
* Instead of adding a new decorator, I'm using the existing BackendChain which already supports this with the only difference that it is usually used for multiple backends but works fine with one as well.
* I then noticed that the check for using the wrapper or not in CacheFactory was inversed and we did use the wrapper everywhere in core despite having the tags. Fixing that caused test Fails in CacheFactoryTest, which makes sense, so added a unit test for using the decorator and updated the others to not use it
* As discussed with @dawehner and @alexpott, I removed CacheItemInterface as that should be an immutable value object and there is no reason to have alternative implementations of it.
* Then I updated GenericCacheBackendUnitTestBase to actually use the new methods, which pointed out a whole bunch of bugs in the changes in the backends, some were for example only returning a CacheItem from getMultiple() and not get().
* I also added a CacheItemBcBackendTest test that extends that generic test to ensure that BackendChain can handle that.

As this class immutable maybe set it final?

+++ b/core/lib/Drupal/Core/Cache/CacheItem.php
@@ -6,16 +6,18 @@
-class CacheItem extends \stdClass implements CacheItemInterface {
+class CacheItem extends \stdClass {

Any reason to extend stdClass?

Fixing those test fails.

I created a draft change record and reviewed the patch again, it looks super solid! There are 5 new CS fails introduced in this patch that we should fix.

Fixed CS fails.

The last patch I added for this issue was ~3 months ago and has been changed significantly in the meantime. I'm pretty sure that makes it ok for me to RTBC this issue now.

1. +++ b/core/core.services.yml
   @@ -168,7 +168,7 @@ services:
   +    arguments: ['@settings', '%cache_default_bin_backends%', '%cache_backends_non_legacy%']
   
   @@ -194,14 +194,22 @@ services:
   +      - { name: cache_backend_non_legacy }
   ...
   +      - { name: cache_backend_non_legacy }
   ...
   +      - { name: cache_backend_non_legacy }
   ...
   +      - { name: cache_backend_non_legacy }
   

   cache_backends_non_legacy is an interesting name. It almost feels like a double negative. How about more descriptive tag name so something like cache_item_backend and a parameter of cache_item_backends?

2. +++ b/core/lib/Drupal/Core/Cache/BackendChain.php
   @@ -53,6 +53,23 @@ public function appendBackend(CacheBackendInterface $backend) {
   +  /**
   +   * Wraps a cache result inside a CacheItem.
   +   *
   +   * @param \stdClass|\Drupal\Core\Cache\CacheItem $item
   +   *   The cache item that might need to be wrapped.
   +   *
   +   * @return \Drupal\Core\Cache\CacheItem
   +   *   The cache item.
   +   */
   +  protected function wrapItem($item) {
   +    if ($item instanceof CacheItem) {
   +      return $item;
   +    }
   +
   +    return new CacheItem($item->cid, $item->data, $item->expire, $item->tags, $item->created, $item->valid);
   +  }
   +
   

   Don't we need to add this to \Drupal\Core\Cache\ChainedFastBackend too and also tag cache.backend.chainedfast or are we relying on the cache factory to decorate that too. I'm not sure we should do that though.

3. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -33,6 +33,13 @@ class CacheFactory implements CacheFactoryInterface, ContainerAwareInterface {
   +  /**
   +   * A list of cache backends without a BC layer.
   +   *
   +   * @var string[]
   +   */
   +  protected $cacheBackendsWithNonLegacy;
   

   This is a list of cache backend which don't need decorating. The comment feels like it implies that there are cache backends with a BC layer and without. I think this could be improved by naming this like $cacheItemBackends since that is what this is a list of. This is kinda the same point as the container tag and parameter name above.

2. Interesting, \Drupal\Core\Cache\ChainedFastBackendFactory gets the inner backends through the backend factory service (has to, it doesn't know how to construct them), but those aren't wrapped. We can't get them through the wrapped factory, so we might need to wrap them ourself there, yes. Interesting point though, anything that accesses those backends directly (which they usually should not do) would skip the decorator stuff.

In #3025867: Provide a classed object for TempStore metadata I am replacing a different \stdClass with a real object.
For BC I went the other way and made the public properties into private properties, and used __get() and __set() to provide access to them as well as deprecation messages.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.