CacheBackendInterface->get() docs missing crucial information

Wow. The cache docs are terrible... the ->get() method doesn't even tell me this, and the D8 section on drupal.org is really lame:
https://drupal.org/node/1884796

Also see
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Cache!CacheBacken...
which really really really needs to say that to get the data you put into the cache back out, you need to get ->data().

I think this is a major oversight in the documentation that really needs to be fixed. Should be a good Novice issue.

What needs to be done:

In the @return section of CacheBackendInterface::get(), document that what is returned is a stdObject database row object, and that your data is in the ->data property of it.

This still doesn't tell me that to get back out the data that I placed in the cache, if I save the return value of ->get() in $cache_data, I have to do $cache_data->data. Hopefully that is true for all of the different cache backend classes, because I assume/hope that the user of the cache would not need to know the details of which back end is being used, in order to use the cache (which would completely nullify the idea of having a unified cache interface).

Also, shouldn't all of those other classes' ->get() methods just be using @inheritdoc instead of reproducing the entire ->get() docs?

So I looked into this in more depth.

DatabaseBackEnd returns an object with the following properties:
- data - the cached data
- cid, created, expire, serialized, checksum_invalidations, checksum_deletions [database fields]
- tags [array of the tags]

MemoryBackend (and derived classes like MemoryCountBackend) returns an object with the following properties:
cid, data, created, expire, tags

NullBackend always returns FALSE (the others return FALSE if the cache is expired, nonexistent, etc.).

So, I think we should document the return value of the get() methods as either being:
- FALSE if there is no valid cached data
- An object with properties: cid, data, created, expire, tags [put these in a list and document what each one is].

And I also think all of the individual ->get() methods should use @inheritdoc to inherit the interface docs unless they have something specifically different to say. And I don't think they do. In fact, a few of them just say "Overrides ..." instead of even using @inheritdoc.

For details of how to do @inheritdoc, see:
https://drupal.org/node/1354#inheritdoc

I am not sure exactly what you mean in #7.... An interface should fully document the methods on the interface, including what their parameters and return values are. This is not a "dummy", but rather a "contract" that classes agree to follow when they implement the interface. A class that implements the interface can then use @inheritdoc instead of re-documenting the method. See https://drupal.org/node/1354#inheritdoc for details on how to use inheritdoc. Does that answer your question?

(As a side note, please do not change the status of an issue to "needs review" unless you have a new patch that needs reviewing. There's a link explaining what the priority/status values mean, under the priority/status fields. Thanks!)

No. I don't think so.... not here, anyway. This "Major" issue is about the documentation missing crucial information about what the code is actually currently doing. If you would like to propose different ideas about how the Cache API should work, please open a new issue (not in the Documentation component) to discuss (and in that case, you can set the status to "needs review" to discuss ideas -- but on this issue, which already had a patch that was marked "needs work", please do not use "needs review" unless you are adding a new patch. People subscribed to the issue will notice already if you ask a question.)

So. On this issue, all we want to do is document the return value of the get() methods as either being:
- FALSE if there is no valid cached data
- An object with properties: cid, data, created, expire, tags [put these in a list and document what each one is].

This should be done on the interface, because all implementations of this interface need to have the same return value structure. And all methods that override ->get() on other classes should also be changed to use @inheritdoc.

Thanks!

Yes to #13. Actually, given the context, I'd suggest this:

   *     trigger cache invalidation when updated. For example, if a cached item
   *     represents a node, both the node ID and the author's user ID might be
   *     passed in as tags:
   *     @code
   *     array('node' => array(123), 'user' => array(92))
   *     @endcode

Other nitpicks:

a)

+   *   FALSE if there is no valid cache data; Otherwise a cache item object with

Otherwise should not be capitalized.

b)

+   *       after this time, i.e. it will not be returned unless $allow_invalid

The (sigh) correct punctuation for "i.e." would be: "... time; i.e., it will...". Or better yet, use the words "that is", because 90% of people don't understand the distinction between i.e. and e.g. and will be confused by this anyway. (The words "that is" need to be punctuated the same way.)

c)

+   *     trigger cache invalidation when updated. For example if a cached item

Comma after "For example".

d) Shouldn't DatabaseBackend::get() and MemoryBackend::get() also just use @inheritdoc? Otherwise we need the full docs about the return value in those methods, or a link to the interface method for full docs.

OK, let's see if we can sort this out... And please in the future do not put the "do not test" suffix on patches. At a minimum, it sorts out whether the patch applies, and also makes sure there were no inadvertent PHP syntax problems.

So, back to basics.

a) We have CacheBackendInterface, and these extending classes:
BackendChain
DatabaseBackend
MemoryBackend
MemoryCounterBackend
NullBackend

b) The interface has two methods, get() and getMultiple(), which both refer to "cache item objects", and both of them need to either define what that means, or refer to the other method where it's defined, in their @return section. That's the main subject of this issue, although it originally only applied to get() -- I think we should fix getMultiple() too now that you point it out.

c) This patch adds documentation to get(). I suggest rather than repeating the docs in getMultiple(), we should instead make the @return for getMultiple say something like "An array of cache item objects indexed by cache ID; see CacheBackendInterface::get() for documentation of these objects."

d) I am also thinking that in the tags section of the get() return value documentation, instead of fully explaining tags, we should just say "The tags array that was passed into CacheBackendInterface::set()", since that method already documents what the tags are. And I think the change in this patch to the $tags param in set() is good too, plus the other cleanups to the interface docs.

e) So now on to the classes. I think that it is fine if this patch expands in scope slightly to replace existing docs in class methods with @inheritdoc, for these methods, on all of the classes:

get, getMultiple, set, delete, deleteMultiple, deleteTags, deleteAll, invalidate, invalidateMultiple, invalidateTags, invalidateAll, garbageCollection, isEmpty

Thanks!

RE #18, do you think we should postpone this issue on that one? Is it likely to get done?