Problem/Motivation
1. CacheBackendInterface
contains const CACHE_PERMANENT = -1
.
2. Drupal\Core\Cache\Cache
contains const PERMANENT = CacheBackendInterface::CACHE_PERMANENT
.
3. Both constants are referenced in the interface, but inconsistently:
interface CacheBackendInterface {
/**
* ...
* @param int $expire
* One of the following values:
* - CacheBackendInterface::CACHE_PERMANENT: Indicates that the item should
* not be removed unless it is deleted explicitly.
* ...
*/
public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array());
/**
* ...
* // Optional, defaults to CacheBackendInterface::CACHE_PERMANENT.
* 'expire' => CacheBackendInterface::CACHE_PERMANENT,
* ...
*/
/* ... */
}
There's no obvious explanation as to why these two constants are set equal in the first place, or when one should be used and not the other; which means it's difficult to work out whether or not this is even a bug in code.
Proposed resolution
Either this is a code bug or a (lack of) documentation bug, so either one should be patched.
Remaining tasks
1. Determine what kind of bug it is and maybe update the component accordingly.
2. Propose a fix and create a patch.
3. Approve patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Comments
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedSlice of history:
Cache::PERMANENT
appeared just for simplify usage (see #2169447: DX: Supply CacheBackendInterface::CACHE_PERMANENT as Cache::PERMANENT).Mentioned comment in CacheBackendInterface appeared earlier, but was commited later (see #1167248: Add CacheBackendInterface::setMultiple()).
Comment #3
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented@vapias thanks for the background. I can see why
Cache::PERMANENT
might have been introduced; but I'm not 100% sureCacheInterface::set()
, as an interface method, should be referring to aconst
from a concrete class as an argument default: it feels like we're extending in the wrong direction; like there's like a weak cyclical dependency at work here.Cache::set()
could certainly refer to its ownconst
, though.But I'm not really opinionated either way: the main thing is that, even if there's no change to the code, it would be good if one or other of the docblocks for the
const
s at least made some reference to why the decision was made.Would appreciate opinions from people more opinionated than me: do we want to change the code, or just the docblocks?
Comment #14
catchNo functional bug here but agree this is confusing, I think we should pick one.