Problem

  • CacheBackendInterface::CACHE_PERMANENT exposes too much internal complexity into user-space code.
  • The primary interaction point for modules is cache() (read + create) and \Drupal\Core\Cache\Cache (invalidation).

Proposal

  1. Supply CacheBackendInterface::CACHE_PERMANENT as Cache::PERMANENT to simplify usage, namespace imports, and KISS.

Comments

sun’s picture

StatusFileSize
new52.29 KB

Full conversion of all instances + use statements throughout core, courtesy of @dawehner + phpStorm ;)

For now, I'm intentionally leaving the BC-shim in place; i.e.:

class Cache {

  const PERMANENT = CacheBackendInterface::CACHE_PERMANENT;
}

Doing so should hopefully break less patches in the queue right now.

And perhaps we even want to retain that as-is (not marking CacheBackendInterface::CACHE_PERMANENT as @deprecated). Technically, the constant/value is not really owned by CacheBackend, but would rather belong to a (non-existing) CacheItem class, since it is the cache item that declares when it wants to expire.

Since there is no CacheItem class, and also, given our current efforts that are seemingly moving all of the remainders of cache.inc into the Cache class, the Cache class appears to be the most appropriate home for now.

larowlan’s picture

+1

Status: Needs review » Needs work

The last submitted patch, 1: drupal8.cache-dx.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

1: drupal8.cache-dx.1.patch queued for re-testing.

sun’s picture

Issue tags: +API clean-up

Back to green.

longwave’s picture

StatusFileSize
new52.34 KB
new370 bytes

+1 to improving DX here.

Noted a one line whitespace issue, corrected in the attached patch, otherwise this looks RTBC to me.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I certainly like that we don't expose that internal anymore.

webchick’s picture

Well that's definitely a heck of a lot nicer.

However, unfortunately (and not surprisingly) no longer applies.

damiankloip’s picture

StatusFileSize
new51.71 KB

Rerolled.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2169447-9.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
StatusFileSize
new51.9 KB
new587 bytes

Whoops, sorry. We don;t want to be trying to use the same class twice.

Status: Needs review » Needs work

The last submitted patch, 11: 2169447-11.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
StatusFileSize
new51.61 KB
new543 bytes

Over zealous with the CacheBackendInterface removal in EntityManager.. Sorry for the noise.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

sun’s picture

Quoting myself:

Technically, the constant/value is not really owned by CacheBackend, but would rather belong to a (non-existing) CacheItem class, since it is the cache item that declares when it wants to expire.

Since there is no CacheItem class, and also, given our current efforts that are seemingly moving all of the remainders of cache.inc into the Cache class, the Cache class appears to be the most appropriate home for now.

Please note that work on a CacheItem class + interface happens in #1748022: Make CacheBackendInterface::get() return a proper class

That said, even with that, the DX of Cache::PERMANENT would still be much nicer than CacheItemInterface::PERMANENT.

webchick’s picture

Title: DX: Supply CacheBackendInterface::CACHE_PERMANENT as Cache::PERMANENT » Change notice: DX: Supply CacheBackendInterface::CACHE_PERMANENT as Cache::PERMANENT
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Approved API change, +Needs change record

Getting this in while it's hot!

Committed and pushed to 8.x. Thanks!

Change notice time.

jessebeach’s picture

Status: Active » Fixed
Issue tags: -Needs change record

Change notice authored: https://drupal.org/node/2182907

jessebeach’s picture

Title: Change notice: DX: Supply CacheBackendInterface::CACHE_PERMANENT as Cache::PERMANENT » DX: Supply CacheBackendInterface::CACHE_PERMANENT as Cache::PERMANENT
jibran’s picture

Thanks for the change notice @jessebeach. I have updated minor doc issues and removed approved change line please review. https://drupal.org/node/2182907/revisions/view/6870919/6871249

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.