Remove backwards compatibility layer for cache API

Sub, will help when I'll have some spare time.

Why is this critical?

Okay; wasn't trying to be argumentative; just wanted to hear the rationale. Thanks.

I was able to get the installer working together with #1275808: Use new cache bin naming in hook_flush_cache and porting a last remaining cache_get_multiple() call (which is I think the only one in core and was probably forgotten because of that).

I also updated a few remaining mentions of cache_get(), cache_set(), cache_get_multiple() with something I found appropriate, please review. The interdiff shows them.

Leaving at needs work, change to needs review once the linked issue is commited.

Fixed the trailing whitespace :)

+++ b/modules/field/field.attach.inc
@@ -634,7 +634,7 @@ function field_attach_load($entity_type, $entities, $age = FIELD_LOAD_CURRENT, $
-    $cache = cache_get_multiple($cids, 'cache_field');
+    $cache = cache('cache_field')->getMultiple($cids);

If I read the patch correctly, this should now be cache('field') ?

Powered by Dreditor.

Absolutely.

Marking to needs review now that #1272686: convert existing cache_* calls to the new cache()-> API and #1275808: Use new cache bin naming in hook_flush_cache are in.

We're still not quite there yet:

Two issues:

1. In menu_cache_clear(), the following line will need to be converted to the new syntax, possibly with the addition of a helper function.
   

       drupal_register_shutdown_function('cache_clear_all', 'links:' . $menu_name . ':', 'cache_menu', TRUE);
   

2. In CacheTestCase (modules/simpletest/tests/cache.test), $default_bin is set to 'cache_page', not 'page'.

Would a possible solution for the cache_clear_all() problem be changing

function cache_clear_all() {
  if (module_exists('block')) {
    // Clear the block cache first, so stale data will not end up in the page
    // cache.
    cache('block')->expire();
  }
  cache('page')->expire();
}

to something on the order of (please ignore likely syntax errors)

function cache($bin = 'cache')->flush() {  // Idea is to flush all caches, and ignoring invalid $bin names
    if ($bin == 'all') {                             
      if (module_exists('block')) {
        // Clear the block cache first, so stale data will not end up in the page
        // cache.
        cache('block')->expire();
      }
      cache('page')->expire();
    }
    else {
      // existing implementation of cache()->flush();
    }
}

It would permit all ooccurrences of cache_clear_all();
to be replaced by cache('all')->flush();

Just a quick thought off the top of my head. No idea how it would impact the code otherwise. If this isn't the solution, well, sorry I spoke up. :)

+++ b/includes/cache.inc
@@ -4,10 +4,28 @@
+ * For example to use 'MyCustomCache' for the''page' bin. Set the following
+ * variables:

double quotation mark here instead of white space

+++ b/includes/cache.inc
@@ -237,7 +80,7 @@ interface DrupalCacheInterface {
+   * text or as serialized data. Will automatically return
    * unserialized objects and arrays.

line breaks too early here

Re-rolled #20, reformatting comments in includes/cache.inc in response to #21 and similar concerns. No code changes.

Why is patch in #22 so much larger than #20? I suspect that some extra stuff was included rather than are-roll with small text changes.

Why is patch in #22 so much larger than #20? I suspect that some extra stuff was included rather than are-roll with small text changes.

Patch #22 is larger because it corrects errors in the comments that were not corrected by patch #20, in order to make the documentation meet current coding standards.

Here's the interdiff, with annotations:

diff -u b/includes/cache.inc b/includes/cache.inc
--- b/includes/cache.inc
+++ b/includes/cache.inc
@@ -1,15 +1,15 @@
 <?php
 
 /**

Remove the redundant word "correct" so the summary fits in 80 characters.

- * Factory for instantiating and statically caching the correct class for a cache bin.
+ * Factory for instantiating and statically caching the class for a cache bin.
  *
  * By default, this returns an instance of the DrupalDatabaseCache class.

Wrap at 80 characters.

- * Classes implementing DrupalCacheInterface can be used instead, either
- * as a default for all cache requests, or for specific bins.
+ * Classes implementing DrupalCacheInterface can be used instead, either as a
+ * default for all cache requests, or for specific bins.
  *
  * Configuration of cache bins should be done in settings.php
  *

Correct usage of quotation marks.

- * For example to use 'MyCustomCache' for the''page' bin. Set the following
+ * For example, to use "MyCustomCache" for the "page" bin, set the following
  * variables:
  *
  * @code
@@ -21,8 +21,9 @@
  * @endcode
  *
  * Additionally, you can register your cache implementation to be used by

Correct usage of quotation marks.
Correct run-on sentence error.
Expand "e.g." to "For example".
Add a blank line before the code example.

- * default for all cache bins by setting the variable 'cache_default_class' to
- * the name of your implementation of the DrupalCacheInterface, e.g.
+ * default for all cache bins by setting the variable "cache_default_class" to
+ * the name of your implementation of the DrupalCacheInterface.  For example:
+ *
  * @code
  * $conf['cache_default_class'] = 'MyCustomCache';
  * @endcode
@@ -30,6 +31,7 @@

Add a blank line before the @return line.

  * @param $bin
  *   The cache bin for which the cache object should be returned, defaults to
  *   'cache'.
+ *
  * @return DrupalCacheInterface
  *   The cache object associated with the specified bin.
  */
@@ -60,10 +62,10 @@
 }
 
 /**

Coding standards require the summary to begin with a third-person present indicative verb.

- * Interface for cache implementations.
+ * Provides the interface for cache implementations.
  *
  * All cache implementations have to implement this interface.

Correct a subtle error in grammar.

- * DrupalDatabaseCache provides the default implementation, which can be
+ * DrupalDatabaseCache provides the default implementation, which may be
  * consulted as an example.
  *
  * @see cache()
@@ -79,38 +81,40 @@
   function __construct($bin);
 

Coding standards require the summary to begin with a third-person present indicative verb.
Wrap at 80 characters.

   /**
-   * Return data from the persistent cache. Data may be stored as either plain
-   * text or as serialized data. Will automatically return
-   * unserialized objects and arrays.
+   * Returns data from the persistent cache. Data may be stored as either plain
+   * text or as serialized data. Will automatically return unserialized objects
+   * and arrays.
    *

Add a blank line before the @return line.

    * @param $cid
    *   The cache ID of the data to retrieve.
+   *
    * @return
    *   The cache or FALSE on failure.
    */
   function get($cid);
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Return data from the persistent cache when given an array of cache IDs.
+   * Returns data from the persistent cache when given an array of cache IDs.
    *

Add a blank line before the @return line.

    * @param $cids
    *   An array of cache IDs for the data to retrieve. This is passed by
    *   reference, and will have the IDs successfully returned from cache
    *   removed.
+   *
    * @return
    *   An array of the items successfully returned from cache indexed by cid.
    */
    function getMultiple(&$cids);
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Store data in the persistent cache.
+   * Stores data in the persistent cache.
    *
    * @param $cid
    *   The cache ID of the data to store.
    * @param $data
    *   The data to store in the cache. Complex data types will be automatically

Wrap at 80 characters.

-   *   serialized before insertion.
-   *   Strings will be stored as plain text and not serialized.
+   *   serialized before insertion. Strings will be stored as plain text and not
+   *   serialized.
    * @param $expire
    *   One of the following values:
    *   - CACHE_PERMANENT: Indicates that the item should never be removed unless
@@ -123,7 +127,7 @@
   function set($cid, $data, $expire = CACHE_PERMANENT);
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Delete an item from the cache.
+   * Deletes an item from the cache.
    *
    * @param $cid
    *    The cache ID to delete.
@@ -131,7 +135,7 @@
   function delete($cid);
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Delete multiple items from the cache.
+   * Deletes multiple items from the cache.
    *
    * @param $cids
    *   An array of $cids to delete.
@@ -139,7 +143,7 @@
   function deleteMultiple(Array $cids);
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Delete items from the cache using a wildcard prefix.
+   * Deletes items from the cache using a wildcard prefix.
    *
    * @param $prefix
    *   A wildcard prefix.
@@ -147,34 +151,34 @@
   function deletePrefix($prefix);
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Flush all cache items in a bin.
+   * Flushes all cache items in a bin.
    */
   function flush();
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Expire temporary items from cache.
+   * Expires temporary items from cache.
    */
   function expire();
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Perform garbage collection on a cache bin.
+   * Performs garbage collection on a cache bin.
    */
   function garbageCollection();
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Check if a cache bin is empty.
+   * Checks whether a cache bin is empty.
    *
    * A cache bin is considered empty if it does not contain any valid data for
    * any cache ID.
    *

Describe both possible return states, for clarity.

    * @return
-   *   TRUE if the cache bin specified is empty.
+   *   TRUE if the cache bin is empty; FALSE otherwise.
    */
   function isEmpty();
 }
 

Coding standards require the summary to begin with a third-person present indicative verb.

 /**
- * Default cache implementation.
+ * Provides the default cache implementation.
  *
  * This is Drupal's default cache implementation. It uses the database to store
  * cached data. Each cache bin corresponds to a database table by the same name.
@@ -183,8 +187,8 @@
   protected $bin;
 
   function __construct($bin) {

Corrected grammar and tone.

-    // All cache tables should be prefixed with 'cache_', apart from the
-    // default 'cache' bin, which would look silly.
+    // All cache tables should be prefixed with 'cache_', except for the
+    // default 'cache' bin.
     if ($bin != 'cache') {
       $bin = 'cache_' . $bin;
     }
@@ -228,7 +232,7 @@
   }
 

Coding standards require the summary to begin with a third-person present indicative verb.

   /**
-   * Prepare a cached item.
+   * Prepares a cached item.
    *
    * Checks that items are either permanent or did not expire, and unserializes
    * data as appropriate.
@@ -249,9 +253,9 @@
     // If enforcing a minimum cache lifetime, validate that the data is
     // currently valid for this user before we return it by making sure the cache
     // entry was created before the timestamp in the current session's cache

Wrap at 80 characters.

-    // timer. The cache variable is loaded into the $user object by _drupal_session_read()
-    // in session.inc. If the data is permanent or we're not enforcing a minimum
-    // cache lifetime always return the cached data.
+    // timer. The cache variable is loaded into the $user object by
+    // _drupal_session_read() in session.inc. If the data is permanent or we're
+    // not enforcing a minimum cache lifetime always return the cached data.
     if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && $user->cache > $cache->created) {
       // This cache data is too old and thus not valid for us, ignore it.
       return FALSE;
@@ -326,12 +330,12 @@
 
       $cache_flush = variable_get('cache_flush_' . $this->bin, 0);
       if ($cache_flush == 0) {

Correct run-on sentence error.

-        // This is the first request to clear the cache, start a timer.
+        // This is the first request to clear the cache; start a timer.
         variable_set('cache_flush_' . $this->bin, REQUEST_TIME);
       }
       elseif (REQUEST_TIME > ($cache_flush + variable_get('cache_lifetime', 0))) {

Correct run-on sentence error.

-        // Clear the cache for everyone, cache_lifetime seconds have
-        // passed since the first request to clear the cache.
+        // Clear the cache for everyone; cache_lifetime seconds have passed
+        // since the first request to clear the cache.
         db_delete($this->bin)
           ->condition('expire', CACHE_PERMANENT, '<>')
           ->condition('expire', REQUEST_TIME, '<')
@@ -340,7 +344,7 @@
       }
     }
     else {

Correct run-on sentence error.

-      // No minimum cache lifetime, flush all temporary cache entries now.
+      // No minimum cache lifetime; flush all temporary cache entries now.
       db_delete($this->bin)
         ->condition('expire', CACHE_PERMANENT, '<>')
         ->condition('expire', REQUEST_TIME, '<')
@@ -355,7 +359,8 @@
     // often since this will remove temporary cache items indiscriminately.
     $cache_flush = variable_get('cache_flush_' . $this->bin, 0);
     if ($cache_flush && ($cache_flush + variable_get('cache_lifetime', 0) <= REQUEST_TIME)) {

Wrap at 80 characters.

-      // Reset the variable immediately to prevent a meltdown in heavy load situations.
+      // Reset the variable immediately to prevent a meltdown in heavy load
+      // situations.
       variable_set('cache_flush_' . $this->bin, 0);
       // Time to flush old cache data
       db_delete($this->bin)

I figured that if people were going to mark this patch as "needs work" because of pedantic comment formatting errors, we might as well proactively fix all of them at once.

Thanks. All of those text and documentation changes look good!

Re-rolled #22 to correct fuzz.

Sorry but zero byte patch attached in #28.

Thanks, Lars. Trying again...

Trying again...

Well, it installs locally now...

Did a grep for cache_set and cache_get -- think I caught 'em all.

I think such an Exception makes sense. I'm setting to 'needs work' because of the "0 passes" wich are deceivingly green.

"needs work" as per #39

#38: cache.patch queued for re-testing.

Still 0 passes, looking at the review log you can see that the tests *are* run but there are a number of exceptions like this one:

exception 'Exception' with message 'cache_clear_all() no longer takes arguments, use cache() instead.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/cache.inc:62
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/locale.inc(558): cache_clear_all('*', 'cache_page', true)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/locale/locale.install(14): locale_language_save(Object(stdClass))
#2 [internal function]: locale_install()
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(795): call_user_func_array('locale_install', Array)
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(448): module_invoke('locale', 'install')
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/drupal_web_test_case.php(1323): module_enable(Array, true)
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/field/tests/field.test(25): DrupalWebTestCase->setUp(Array)
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/field/tests/field.test(2622): FieldTestCase->setUp('locale', 'field_test')
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/drupal_web_test_case.php(476): FieldTranslationsTestCase->setUp()
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(367): DrupalTestCase->run()
#10 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('1', 'FieldTranslatio...')
#11 {main}FATAL FieldTranslationsTestCase: test runner returned a non-zero error code (1).

Actually, there are *hundreds* of those. And finally, MySQL is giving up and dies:

Additional uncaught exception thrown while handling exception.
Original

PDOException: SQLSTATE[HY000]: General error: 2006 MySQL server has gone away: SELECT last_prefix FROM {simpletest_test_id} WHERE test_id = :test_id LIMIT 0, 1; Array ( [:test_id] => 1 ) in simpletest_last_test_get() (line 237 of /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/simpletest.module).

Conclusion: This patch kills the test bot :)

Found a remaining cache clear all call in menu.inc:

drupal_register_shutdown_function('cache_clear_all', 'links:' . $menu_name . ':', 'cache_menu', TRUE);

And one in locale.inc:

cache_clear_all('*', 'cache_page', TRUE);

That last one was probably added by one of the recent i18n patches, not sure about the first.

I followed the cache evolution various threads since a long time ago, and read the patch once again: it's definitely RTBC now that it passes tests. If it's not included now some people may end up by commiting code based on removed functions here and it will continue to delay it indefinitely.

This patch fixes a small indentation mistake. Leaving as RTBC as it makes no other functional change.

Pong. Good idea indeed.

Followup: #1394648: The installer's cache backend no longer overrides all cache-clearing methods, which can lead to fatal errors

Sorry for changing the state, this patch is OK definitely but we have to put a priority between this patch and #1323120: Convert cache system to PSR-0, which one are we going to commit first?

Thanks a lot to David_Rothstein for raising this point.

Plus this patch does not fix the install backend, it makes its clear() method silent and risk to awake the ugly side effects we had before it has been created.

When has it been commited? Didn't see conflicts applying my patch yesterday?
Ok understood, the namespacing patch retintroduce the clear() method because it was started before, and now it doesn't create any conflicts because it creates fully new files.