The cache API deals with stale cache items in a non-consistent way. I propose some changes that will make common usage of the API easier while allowing callers to make better use of stale (invalid/expired) data.

$cache->get() may return expired values. This may sometimes be useful, but callers need to check the expire time themselves. This is a somewhat unexpected behaviour (filed as a bug here: #534092: cache_get() returns expired cache items).

On the other hand, if an item has been invalided via $cache->invalidateTags(), it cannot be retrieved via $cache->get(), even if it has not yet been deleted from the backend.

The cache API uses very inconsistent wording when it comes to “removing” cache entries:

  • $cache->flush() deletes all items – I propose a renaming to deleteAll() (for consistency with delete() and deleteMultiple()).
  • $cache->expire() deletes entries that have expired – I propose a renaming to deleteExpired().
  • $cache->invalidateTags() effectively deletes entries with the specified tags (they are not deleted right away, but they cannot be retrieved through get()). I suggest we stick to this but extend $cache->get() with the possibility to retrieve those values.

In addition to implementing these proposals, the attached patch adds the following:

  • Two new functions, $cache->invalidate($cid) and $cache->invalidateMultiple($cid), that mark existing cache items as invalid. This is equivalent to modifying the items so they get an expire time in the past.
  • $cache->get()/getMultiple() gets an options second argument, $cache->get($cid, $allowInvalid) and $cache->getMultiple($cids, $allowInvalid), that specifies whether expired or explicitly invalidated (via $cache->invalidate()/invalidateMultiple()/invalidateTags()) items should be returned. By default only valid items are returned.
  • cache_invalidate() is renamed to cache_invalidate_tags() to match $cache->invalidateTags() and not be confused with $cache->invalidate().
  • The structure of DatabaseBackend and MemoryBackend are brought more in sync. We should consider introducing a common abstract base class, BackendBase, that implements shared functionality.
  • Terminology clean-up in tests and Doxygen comments.
Files: 
CommentFileSizeAuthor
#38 cache-invalid-16.patch100.95 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 48,948 pass(es).
[ View ]
#35 cache-invalid-15.patch100.95 KBc960657
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cache-invalid-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#30 cache-invalid-14.patch95.33 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 48,865 pass(es).
[ View ]
#28 cache-invalid-13.patch94.06 KBc960657
FAILED: [[SimpleTest]]: [MySQL] 48,201 pass(es), 16 fail(s), and 30 exception(s).
[ View ]
#27 cache-invalid-12.patch94.68 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 46,330 pass(es).
[ View ]
#26 cache-invalid-11.patch94.97 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 46,323 pass(es).
[ View ]
#24 cache-invalid-10.patch91.89 KBc960657
FAILED: [[SimpleTest]]: [MySQL] 43,265 pass(es), 117 fail(s), and 0 exception(s).
[ View ]
#23 cache-invalid-9.patch8.35 KBc960657
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cache-invalid-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 cache-invalid-6.patch81.87 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 42,172 pass(es).
[ View ]
#13 cache-invalid-5.patch81.14 KBc960657
FAILED: [[SimpleTest]]: [MySQL] 42,164 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#8 cache-invalid-4.patch77.47 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 41,503 pass(es).
[ View ]
#6 cache-invalid-3.patch77.38 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 41,499 pass(es).
[ View ]
#4 cache-invalid-2.patch77.29 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 41,405 pass(es).
[ View ]
cache-invalid-1.patch58.52 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 41,144 pass(es).
[ View ]

Comments

Assigned:Unassigned» c960657
Issue tags:+API change, +API clean-up

I haven't reviewed the actual patch yet but the overall description sounds good.

If/when this is re-rolled, please consider the following documentation adjustments. Thanks.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -65,45 +63,48 @@ interface CacheBackendInterface {
+   * @param bool $allowInvalid
+   *   If TRUE, a cache item may be returned even if it is expired or has been
+   *   invalidated. The "valid" property of the returned object indicates

I think that description should start with '(optional)' as well as specifying what the default value is.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -65,45 +63,48 @@ interface CacheBackendInterface {
+   * @return object
+   *   The cache item or FALSE on failure.

I think this should be '@return object|false'.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -65,45 +63,48 @@ interface CacheBackendInterface {
+   * @param bool $allowInvalid
+   *   If TRUE, cache items may be returned even if they have expired or been
+   *   invalidated. The "valid" property of the returned objects indicates

Also needs '(optional)' at start as well as noting what the default value is.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -65,45 +63,48 @@ interface CacheBackendInterface {
    * @return
-   *   An array of the items successfully returned from cache indexed by cid.

Can we add a @return type hint here? Also I would suggest replacing CID with 'cache ID'.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -211,10 +219,10 @@ class DatabaseBackend implements CacheBackendInterface {
    * @return boolean
-   *   TRUE if the checksums match, FALSE otherwise.
+   *   TRUE if the checksums match, i.e. the item is valid, FALSE otherwise.

I think the coding standard is bool, not boolean, in this case.

+++ b/core/lib/Drupal/Core/Cache/InstallBackend.phpundefined
@@ -89,36 +89,60 @@ class InstallBackend extends DatabaseBackend {
+   * Overrides Drupal\Core\Cache\DatabaseBackend::flush().

I think this should be ::deleteAll() instead of ::flush().

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -147,37 +150,36 @@ class MemoryBackend implements CacheBackendInterface {
    * Implements Drupal\Core\Cache\CacheBackendInterface::expire().

I think you meant ::deleteExpired() here.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -147,37 +150,36 @@ class MemoryBackend implements CacheBackendInterface {
+   * Cache expiration is not implemented for MemoryBackend as this backend
    * only persists during a single request and expiration are done using

This should be rewrapped for 80 characters.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -147,37 +150,36 @@ class MemoryBackend implements CacheBackendInterface {
+   * @param integer @checksum
+   *   The initial checksum to compare against.
+   * @param array @tags
+   *   An array of tags to calculate a checksum for.

Left over from some development? This does not match the function. Also should be '@return bool'.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -209,6 +211,22 @@ class MemoryBackend implements CacheBackendInterface {
+   * Implements Drupal\Core\Cache\CacheBackendInterface::invalidate().

Should be ::invalidateMultiple() perhaps?

StatusFileSize
new77.29 KB
PASSED: [[SimpleTest]]: [MySQL] 41,405 pass(es).
[ View ]

Thanks for the review. This patch addressess the concerns raised.

Also, deleteTags() is added for completeness. In the database backend, the cache items are not actually deleted from the database, but they are never returned.

Status:Needs review» Needs work

Thanks @c960657 for incorporating many of the suggested changes. I have now done a fresh review from start and noticed the following. Hopefully, you you might be able to include these comments in your next re-roll.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -87,24 +83,39 @@ class DatabaseBackend implements CacheBackendInterface {
    *
    * @return mixed
-   *   The item with data unserialized as appropriate or FALSE if there is no

Small nit... this should be type hinted as 'mixed|false' to help developers.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -246,51 +295,40 @@ class DatabaseBackend implements CacheBackendInterface {
-   *   Associative array of tags.
+   *   Array of flat tags.

Not sure what is meant by the phrase 'flat tags'. Can you expand the comment/explanation?

+++ b/core/lib/Drupal/Core/Cache/NullBackend.phpundefined
@@ -60,19 +60,29 @@ class NullBackend implements CacheBackendInterface {
   /**
-   * Implements Drupal\Core\Cache\CacheBackendInterface::garbageCollection().
+   * Implements Drupal\Core\Cache\CacheBackendInterface::deleteAll().
    */
-  function garbageCollection() {}
+  function deleteTags(array $tags) {}

I think that '::deleteAll()' here should be '::deleteTags()'.

Status:Needs work» Needs review
StatusFileSize
new77.38 KB
PASSED: [[SimpleTest]]: [MySQL] 41,499 pass(es).
[ View ]

Thank you for your scrutinous review. This patch addresses your latest comments.

Not sure what is meant by the phrase 'flat tags'. Can you expand the comment/explanation?

I have added a @see pointer DatabaseBackend::flattenTags(). The concept of flat tags is used internally in this class. This is the documentation for a protected method, so I think it is acceptable to use a "local" lingo.

Status:Needs review» Needs work

Thanks @c960657 for incorporating many of the suggested changes in the patch #6. I have now done a fresh review from start and noticed the following. Hopefully, you you might be able to include these comments in your next re-roll.

I am leaving as needs work because at minimum the unrelated changes (#mail) need to be deleted.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -65,45 +63,48 @@ interface CacheBackendInterface {
    *   The cache ID of the data to retrieve.
+   * @param boolean $allowInvalid
+   *   (optional) If TRUE, a cache item may be returned even if it is expired or
+   *   has been invalidated. The "valid" property of the returned object

Sorry for not noticing this before... I understand that the correct type hint is bool instead of boolean (although I prefer boolean!!).

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -116,39 +117,72 @@ interface CacheBackendInterface {
+  /**
+   * Delete all cache items in a bin.

Change s/Delete/Deletes/

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -116,39 +117,72 @@ interface CacheBackendInterface {
+  /**
+   * Delete expired items from the cache.

Ibid.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -116,39 +117,72 @@ interface CacheBackendInterface {
-   * Invalidates each tag in the $tags array.
+   * Invalidates items with any of the specified tags.

For clarity in api display, it might be helpful to change to 'Invalidates cache items ...'

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -180,14 +209,14 @@ class DatabaseBackend implements CacheBackendInterface {
   /**
    * Implements Drupal\Core\Cache\CacheBackendInterface::flush().
    */
-  function flush() {

I think this should be '::deleteAll()' instead of '::flush()'.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -180,14 +209,14 @@ class DatabaseBackend implements CacheBackendInterface {
   /**
    * Implements Drupal\Core\Cache\CacheBackendInterface::expire().
    */
-  function expire() {
+  function deleteExpired() {
     Database::getConnection()->delete($this->bin)

This docblock should be '::deleteExpired()'.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -224,7 +273,7 @@ class DatabaseBackend implements CacheBackendInterface {
-   *   Numeric array of flattened tag identifiers.
+   *   Numeric array of flat tag identifiers.

Just my opinion, but the existing description was better especially if put in single quotes to connote jargon.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -246,51 +295,42 @@ class DatabaseBackend implements CacheBackendInterface {
    * @return integer
    *   Sum of all invalidations.

Should be int instead of integer.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -246,51 +295,42 @@ class DatabaseBackend implements CacheBackendInterface {
+      // Fill static cache with empty objects for tags not found in the database.
+      $tag_cache += array_fill_keys(array_diff($query_tags, array_keys($db_tags)), $checksum);

Exceeds 80 characters... maybe drop the 'the'?

+++ b/core/lib/Drupal/Core/Database/Query/Merge.phpundefined
@@ -421,6 +421,7 @@ class Merge extends Query implements ConditionInterface {
+          #mail('chsc@chsc.dk', __METHOD__, ''.$insert.print_r(debug_backtrace(),1));exit;

Leftover debug code??

+++ b/core/lib/Drupal/Core/Database/Query/Merge.phpundefined
@@ -442,6 +443,7 @@ class Merge extends Query implements ConditionInterface {
         }
+        #mail('chsc@chsc.dk', __METHOD__, ''.$update);
         $update->execute();

Ibid.

Status:Needs work» Needs review
StatusFileSize
new77.47 KB
PASSED: [[SimpleTest]]: [MySQL] 41,503 pass(es).
[ View ]

Just my opinion, but the existing description was better especially if put in single quotes to connote jargon.

I have reworded the whole block. The @return part is now expressed in terms of data types ("An indexed array of strings").

A couple of thoughts from fits reading through of the code.....

+++ b/core/includes/cache.incundefined
@@ -40,6 +40,25 @@ function cache($bin = 'cache') {
+ *
+ * Many sites have more than one active cache backend, and each backend my use
+ * a different strategy for storing tags against cache items, and deleting cache

I would think you meant 'may' here.

+++ b/core/includes/cache.incundefined
@@ -40,6 +40,25 @@ function cache($bin = 'cache') {
+ * @param array $tags

Being naive, it would be really helpful if the documentation could specify if these are exact matches or partial matches.

Status:Needs review» Needs work

Needs reroll at least for that typo. otherwise, looks good.

Status:Needs work» Needs review

Reroll. There were quite a few merge conflicts due to #1393392: Convert prefix cache clears to cache tags, then remove support for them.

Being naive, it would be really helpful if the documentation could specify if these are exact matches or partial matches.

You mean whether all tags should match or just one? I have now used the wording from invalidateTags() and deleteTags():

* Deletes items from all bins with any of the specified tags.
...
* Marks cache items from all bins with any of the specified tags as invalid.

Patch missing :(

StatusFileSize
new81.14 KB
FAILED: [[SimpleTest]]: [MySQL] 42,164 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Oops :-)

Status:Needs review» Needs work

The last submitted patch, cache-invalid-5.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new81.87 KB
PASSED: [[SimpleTest]]: [MySQL] 42,172 pass(es).
[ View ]

I had to add the following to Drupal\locale\Tests\LocaleTranslationTest::testStringTranslation().

+    // Reset the tag cache on the tester side in order to pick up the call to
+    // cache()->deleteTags() on the tested side.
+    drupal_static_reset('Drupal\Core\Cache\CacheBackendInterface::tagCache');

In general, I am not sure I like the static caching of tags in invalidateTags() that is not also in deleteTags(). It means that other long-running jobs don't pick up what has been deleted/invalidated after they started. This behaviour is not consistent with that of delete()/invalidate()/deleteAll()/invalidateAll(). I suggest we deal with this in a separate ticket (the issue exists for invalidateTags() already).

Status:Needs review» Needs work
Issue tags:-API change, -API clean-up

The last submitted patch, cache-invalid-6.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+API change, +API clean-up

#15: cache-invalid-6.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Reviewed and it looks like a good cleanup to me. catch is really the best evaluator so lets send this along ...

Status:Reviewed & tested by the community» Needs work

+++ b/core/includes/cache.incundefined
@@ -44,19 +44,38 @@ function cache($bin = 'cache') {
/**
- * Invalidates the items associated with given list of tags.
+ * Deletes items from all bins with any of the specified tags.
+ *
+ * Many sites have more than one active cache backend, and each backend may use
+ * a different strategy for storing tags against cache items, and deleting cache
+ * items associated with a given tag.
+ *
+ * When deleting a given list of tags, we iterate over each cache backend, and
+ * and call deleteTags() on each.
+ *
+ * @param array $tags
+ *   The list of tags to delete cache items for.
+ */
+function cache_delete_tags(array $tags) {
+  foreach (CacheFactory::getBackends() as $bin => $class) {
+    cache($bin)->invalidateTags($tags);
+  }
+}
+
+/**
+ * Marks cache items from all bins with any of the specified tags as invalid.
  *
  * Many sites have more than one active cache backend, and each backend my use
  * a different strategy for storing tags against cache items, and invalidating
  * cache items associated with a given tag.
  *
  * When invalidating a given list of tags, we iterate over each cache backend,
- * and call invalidate on each.
+ * and call invalidateTags() on each.
  *
  * @param array $tags
  *   The list of tags to invalidate cache items for.
  */
-function cache_invalidate(array $tags) {
+function cache_invalidate_tags(array $tags) {
   foreach (CacheFactory::getBackends() as $bin => $class) {
     cache($bin)->invalidateTags($tags);

These functions are exactly the same.

+++ b/core/includes/entity.incundefined
@@ -102,7 +102,7 @@ function entity_get_info($entity_type = NULL) {
function entity_info_cache_clear() {
   drupal_static_reset('entity_get_info');
   // Clear all languages.
-  cache()->invalidateTags(array('entity_info' => TRUE));
+  cache()->deleteTags(array('entity_info' => TRUE));

Yet this is changing?

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -116,31 +117,64 @@ function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANENT, arra
+   * Marks a cache item as invalid.
+   *
+   * Invalid items may be returned in later calls to get(), if the $allowInvalid
+   * argument is TRUE.
+   *
+   * @param string $cid
+   *   The cache ID to invalidate.
    */
-  function flush();

There's no information here about why you'd invalidate an item rather than delete it. Overall I can see the case for allowing expired and tag-invalidated items to potentially be returned from cache()->get(), but I can't see any value in allowing people expiring tags to differentiate bewteen invalidating them and deleting them. What's the use-case for that?

Additionally with things like tags it's simply not possible to physically delete items in particular backends (even the MySQL implementation doesn't do this). Memcache doesn't even delete items when you flush a bin at this point, since it uses 'virtual' Drupal bins inside a single Memcache bin depending on configuration. So I agree with the general aim to allow getters to say "give me something if it happens to be there" - since we're inconsistent on that with expired items at the moment, but not at all with the distinction between invalidation and deletion that's added here - it seems like a lot of additional complexity for no gain to me.

These functions are exactly the same.

Oops, cache_delete_tags() should call deleteTags().

The patch replaces all calls to invalidateTags() with deleteTags(), because the current invalidateTags() actually deletes the items (from the callers point of view).

There's no information here about why you'd invalidate an item rather than delete it. Overall I can see the case for allowing expired and tag-invalidated items to potentially be returned from cache()->get(), but I can't see any value in allowing people expiring tags to differentiate bewteen invalidating them and deleting them. What's the use-case for that?

delete() is relevant if the data being cached itself has been deleted, e.g. if you delete a node, then any cached information about that node will probably be of little use. In some cases would exposing information of deleted content be considered a bug.
invalidate() is relevant if you want to mark a cache item as stale but still allow it to be used until the item has been rebuilt. This would be useful e.g. in drupal_theme_rebuild(). Currently we destroy any cached information, i.e. all threads has to wait for the registry to be rebuilt. If we had just marked it as invalid, one thread could do the rebuilding while others use the cached value.

Additionally with things like tags it's simply not possible to physically delete items in particular backends (even the MySQL implementation doesn't do this).

The idea behind this API change is that delete()/deleteTags() etc. should make the data appear to the user as being deleted. Whether it is actually deleted from the MySQL table or just marked as deleted is an implementation detail that is not revealed externally. Physically deleting items that are marked as deleted can be done during garbage collection.

but not at all with the distinction between invalidation and deletion that's added here - it seems like a lot of additional complexity for no gain to me.

Do you still disagree after reading my explanation above?

Hmm I can see the value, but I think something like drupal_theme_rebuild() should probably just change to a write-through cache instead. Would you mind maybe trying to implement the invalidate logic in that function (or another one if it's easier) so we can see how it looks?

A write-through cache is not always feasible. Sometimes rebuilding the cache may take a long time (especially if many items are invalidated at once), and sometimes rebuilding it may not even be possible, e.g. if rebuilding function is calling an external REST service that is not responding. Spawning a cache rebuild in a background/queue job is one option, but we don't have a good framework for that (yet).

Here is a rewrite of the theme registry load/rebuild functions with added stampede protection (the code has not been tested). Contrary to "standard" use of semaphores, the threads that do not acquire the semaphore can continue using the stale data. Some of the complexity could be wrapped in some helper function (see #1225404: Modern event based lock framework proposal or the Cache Graceful module), but here it is in raw form:

function drupal_theme_rebuild() {
  drupal_static_reset('theme_get_registry');
  cache()->invalidateTags(array('theme_registry' => TRUE));
}
function _theme_load_registry($theme, $base_theme = NULL, $theme_engine = NULL, $complete = TRUE) {
  if ($complete) {
    $lock = FALSE;
    $lock_name = "theme_build_registry:$theme->name";
    $cached = cache()->get("theme_registry:$theme->name", TRUE);
    if ($cached && $cached->valid) {
      return $cached->data;
    }
    // Stale or missing cache item.
    $lock = lock_acquire($lock_name);
    if (!$lock) {
      if ($cached) {
        // Could not acquire lock for rebuilding - use the stale cache item instead.
        return $cached->data;
      }
      if (lock_wait($lock_name)) {
        // We expect this cache item to be present - if not then we go
        // ahead and rebuild the cache ourselves. We accept a stale value,
        // because we know that it has been fresh for at least a short moment
        // immediately before the other thread released its lock.
        $cached = cache()->get("theme_registry:$theme->name", TRUE);
        if ($cached) {
          return $cached->data;
        }
      }
    }
    $registry = _theme_build_registry($theme, $base_theme, $theme_engine);
    // Only persist this registry if all modules are loaded. This assures a
    // complete set of theme hooks.
    if (module_load_all(NULL)) {
      _theme_save_registry($theme, $registry);
    }
    if ($lock) {
      lock_release($lock_name)
    }
    return $registry;
  }
  else
    ..
}

Status:Needs work» Needs review
StatusFileSize
new8.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cache-invalid-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll following the commit of #1651206: Added a cache chain implementation and #1808112: Missing method visibility, bogus phpDoc and coding style in Cache backend classes.

Oops, cache_delete_tags() should call deleteTags().

Fixed.

I added some comments to deleteXxx() explaining why it is sometimes better to call invalidateXxx().

StatusFileSize
new91.89 KB
FAILED: [[SimpleTest]]: [MySQL] 43,265 pass(es), 117 fail(s), and 0 exception(s).
[ View ]

Sorry, bad patch.

Status:Needs review» Needs work

The last submitted patch, cache-invalid-10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new94.97 KB
PASSED: [[SimpleTest]]: [MySQL] 46,323 pass(es).
[ View ]

StatusFileSize
new94.68 KB
PASSED: [[SimpleTest]]: [MySQL] 46,330 pass(es).
[ View ]

StatusFileSize
new94.06 KB
FAILED: [[SimpleTest]]: [MySQL] 48,201 pass(es), 16 fail(s), and 30 exception(s).
[ View ]

Reroll.

Status:Needs review» Needs work

The last submitted patch, cache-invalid-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new95.33 KB
PASSED: [[SimpleTest]]: [MySQL] 48,865 pass(es).
[ View ]

The test bot is happy again. Is this something that you would like to see in D8?

Status:Needs review» Reviewed & tested by the community

back to rtbc.

Assigned:c960657» catch

This one has catch written all over it.

Category:feature» task
Status:Reviewed & tested by the community» Needs review

OK I read through this again. While the tag invalidation vs. deletion still makes me a bit wary in terms of complexity, I see how it could be useful and as far as I can see there's not really any significant overhead - it's just adding an extra value to load and check against.

However this really, really needs more docs on the invalidate tags vs. delete tags use case, they're almost exactly the same now except for the wording change - i.e. why you'd want to use each one and how it fits with the $allow_invalid argument to ->get().

Moving this to a task since it's just bringing consistency to the API rather than adding a completely new feature.

StatusFileSize
new100.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cache-invalid-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I updated the patch with additional explanation in the DocBlock for the CacheBackendInterface itself and various functions. Once we start using this in code, e.g. in the theme registry, we can refer to those functions as example use-cases.

I also added invalidateAll() for completeness to match deleteAll().

Status:Needs review» Reviewed & tested by the community

back to catch

Status:Reviewed & tested by the community» Needs work

The last submitted patch, cache-invalid-15.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new100.95 KB
PASSED: [[SimpleTest]]: [MySQL] 48,948 pass(es).
[ View ]

Title:Better handling of invalid/expired cache entriesChange notice: Better handling of invalid/expired cache entries
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

OK, committed/pushed to 8.x, thanks!

This will need a change notice (or updating http://drupal.org/node/1272696 would probably be better).

Assigned:catch» Unassigned

I updated http://drupal.org/node/1272696 with relevant changes, but didn't add any new docs for the new stuff:

  • invalidate()/invalidateMultiple()
  • The new $allowInvalid parameter for get()/getMultiple()

http://drupal.org/node/1534648 should be merged into the above change notice.

Issue tags:+Needs change record

I added a description of the new invalidateXxx() methods (sorry for the delay). Comments?

Status:Active» Needs review

In http://drupal.org/node/1272696 invalidate info was added. Looks like that takes care of the task from #41.

Was #42 done? I'm not sure I understand what it needs. Maybe it's asking for a link to http://drupal.org/node/1534648 to be added to http://drupal.org/node/1272696

This has been waiting for a change notice for over 5 weeks now. Let's please get this knocked out.

@webchick

I have one drafted for http://drupal.org/node/1534648. This is my first change notice. YesCT helped me look over this one:

Method cache->flush() becomes cache->deleteAll()

<?php
// D7
cache('mymodule')->flush();cache('page')->flush();
// D8
// Clear the cache for the module
cache('mymodule')->deleteAll();cache('page')->deleteAll();

With cache tags being implemented. The new method has been renamed to cache_invalidate_tags() from cache_invalidate(). No parameters have been changed. It still accept an array.

<?php
// D7
cache_invalidate(array('content' => TRUE));
// D8
cache_invalidate_tags(array('content' => TRUE));

Title:Change notice: Better handling of invalid/expired cache entriesBetter handling of invalid/expired cache entries
Priority:Critical» Normal
Status:Needs review» Fixed
Issue tags:+#SprintWeekend

New cache API looks good. I made sure it includes nicely revised content of Cache tag support is added. Kicking this sucker out of queue.

Issue tags:-Needs change record

Removed tag.

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

Issue summary:View changes

Fix missing /ul> tag