convert existing cache_* calls to the new cache()-> API

Those two first patches are good to go. Thanks beejeebus!

Updated issue summary.

Updated issue summary.

aggregator module patch -- 1st contribution.

Updated issue summary.

poll module patch

Possible error: duplicate of previous patch included, as well. oops!

poll module patch

Call to cache_clear_all() with no arguments need to stay the same for now, this is why poll is failing. That was bad grepping on my part, sorry!

Patches to following include files:
menu.inc
form.inc
gettext.inc
module.inc

Updated issue summary.

Assumed assignment

The conversions are reversed.

-  cache_clear_all('locale:', 'cache', TRUE);
+  cache('locale:')->delete('cache');

This should be:

-  cache_clear_all('locale:', 'cache', TRUE);
+ cache()->deletePrefix('locale:');

-  cache_clear_all('bootstrap_modules', 'cache_bootstrap');
-  cache_clear_all('system_list', 'cache_bootstrap');
+  cache()->deleteMultiple(array('bootstrap_modules', 'system_list', 'cache_bootstrap'));

This is nearly there, could be:

cache('bootstrap')->deleteMultiple(array('system_list', 'bootstrap_modules'));

Fixes in [#12] & [#13] emplaced, I hope.

#14: menu-form-gettext-module-inc.cache-1272686-9.patch queued for re-testing.

As catch pointed out in #12, your argument ordering is incorrect.

Here is a rough conversion guide:

cache_get:

cache_get('foo', 'cache');

cache()->get('foo');

cache_get('foo', 'cache_bar');

cache('bar')->get('foo');

cache_set:

cache_set('foo', $data);

cache()->set('foo', $data);

cache_set('foo', $data, 'cache_bar');

cache('bar')->set('foo', $data);

cache_clear_all:

cache_clear_all('foo', 'cache');

cache()->delete('foo');

cache_clear_all('foo', 'cache', TRUE);

cache()->deletePrefix('foo');

cache_clear_all('foo', 'cache_bar');

cache('bar')->delete('foo');

cache_clear_all('foo', 'cache_bar', TRUE);

cache('bar')->deletePrefix('foo');

cache_clear_all('foo', 'cache_bar');
cache_clear_all('baz', 'cache_bar');

cache('bar')->deleteMultiple(array('foo', 'baz'));

module.inc stand-alone patch file.

I'm splitting the combined file submitted in #9 into individual patch files, 1 per file.

Thanks for the assist. Spent some time this afternoon reviewing the changes in includes/cache.inc and concluded the same.

gettext.inc stand-alone cache patch.

menu.inc patch. Partial conversion completed.

Still need to convert cache_clear_all() occurrances where immediately followed by drupal_register_shutdown_function('cache_clear_all'...)

Updated issue summary.

form.inc stand-alone cache patch.

revised module.inc stand-alone cache patch.
Includes correction from beejeebus in previous comment.

Updated issue summary.

path.inc stand-alone cache patch.

Updated issue summary.

registry.inc stand-alone cache patch

Updated issue summary.

Updated issue summary.

Updated issue summary.

module/block cache patch

patches submitted.

modules/comment cache patch.

Gag. Must've been that spot on my glasses. Will fix.

revised module-block cache patch.

Updated issue summary.

cache patches for:

module forum
module node
module simpletest
module path

cache patches for:
module poll
module user
module filter
module taxonomy

revised module simpletest cache patch

Updated issue summary.

End of day progress for 9/8-9/9

Summary:
Successful patches: 15 (testing passed)
Failed patches: 2

module poll 3 failures
module block 4 failures

Will be back to finish up remaining modules this weekend.

Note:
Files excluded from conversions generally ended in .css, .info, .js extensions.
Files specifically checked for conversions generally ended in .inc, .php, .install, .module extensions.

Also a bunch of the patches incorrectly use cache()->deletePrefix(...) instead of cache()->delete(...). The deletePrefix method should be used only when the previous call was of the form cache_clear_all(..., ..., TRUE) (note the TRUE in the 3rd position).

Ok. Will follow-up and re-craft. That brings up a question:

Rough conversion guide for cache_clear_all():

cache_clear_all('foo', 'cache');

        cache()->delete('foo');

cache_clear_all('foo', 'cache', TRUE);

        cache()->deletePrefix('foo');

cache_clear_all('foo', 'cache_bar');

        cache('bar')->delete('foo');

cache_clear_all('foo', 'cache_bar', TRUE);

       cache('bar')->deletePrefix('foo');

cache_clear_all('foo', 'cache_bar');
cache_clear_all('baz', 'cache_bar');

       cache('bar')->deleteMultiple(array('foo', 'baz'));

When in the form
cache_clear_all('*', cache_bar, TRUE); or cache_clear_all('*', cache, TRUE);
is the solution

       cache('bar')->deletePrefix('*');
       // or
       cache()->deletePrefix('*');

or

       cache('bar')->delete();
       // or
       cache()->delete();

or

       cache('bar')->flush();
       // or
       cache()->flush();

If more than one of those is correct, depending on the context of the code, could you clarify which case calls for which solution?

Thanks
Paul

Got it. Thanks.

path.inc revised cache patch

module menu cache patch

module system cache patch

module field cache patch for testing

module image cache patch

repaired field module cache patch

revised image module cache patch

patch roll-up & testing (cache patches)

This is looking good, spotted a few cases where deletePrefix() should be delete() or vice versa, but otherwise very close.

+++ b/includes/form.incundefined
@@ -835,8 +835,8 @@ function drupal_process_form($form_id, &$form, &$form_state) {
+        cache('form')->deletePrefix('form_' . $form_state['values']['form_build_id']);
+        cache('form')->deletePrefix('form_state_' . $form_state['values']['form_build_id']);

These two can just be straight cache('form')->delete(....). No need for prefix since there was no TRUE argument to cache_clear_all().

+++ b/includes/gettext.incundefined
@@ -53,7 +53,7 @@ function _locale_import_po($file, $langcode, $mode) {
-  cache_clear_all('locale:', 'cache', TRUE);

This one however does need to be ->deletePrefix()

+++ b/modules/simpletest/simpletest.moduleundefined
@@ -421,7 +421,7 @@ function simpletest_clean_environment() {
-  cache_clear_all('simpletest', 'cache');

This can just be ->delete()

18 days to next Drupal core point release.

Final roll-up patch I think.

I manually reviewed the patch in #54 and did not notice any errors in the conversion.

Are we going for one big patch here or multiple little ones? In particular, there appear to still be many unchanged occurrences of cache_get, cache_set, and cache_clear_all.

I have attached a list of occurrences of cache_get, cache_set, and cache_clear_all after applying the patch in #54 to a clean 8.x checkout:

I'll have to check with catch on that. Maybe he'll see this and give a holler. Will work on these in a couple of hours.

As far as the patch is concerned. I started with lots of little ones, and was told to roll them all up. So I did.

Thanks for the feedback.

I went ahead and fixed up some (but not all) of the remaining occurrences of the old API. For easier review, the attached patch has two big chunks, the first of which is #54 and the second which is new.

Still to be fixed:

./includes/common.inc:  if (!empty($cid) && $cache = cache_get($cid, $bin)) {
./includes/common.inc:  cache_set($cid, $data, $bin, $expire);
./includes/common.inc:    cache_clear_all('*', $table, TRUE);
./modules/simpletest/tests/cache.test:      cache_set($id, $this->default_value, $bin);
./modules/system/system.module:    cache_clear_all(NULL, $table);

I'm not sure what the conversion process is for $bin as a variable ... i.e. will passing $bin = 'cache_page' always still work or do we need to change the prior code to make sure that $bin is (eventually) just 'page'?

Ok, you combined your changes into my patch. Which is fine. The only hitch is I'm still new at this and paranoid about screwing things up. The new patch is double in size from the one I've been working.

As I see it, I should:

1. Download cache_conversion-1272686-57.patch
2. git reset --hard
3. git apply -v cache_conversion-1272686-57.patch

Is that right? BTW, I'm on IRC in #drupal-contribute if you need to contact directly. Query is fine.

Paul

Also, from your last comment...

From ./includes/common.inc: cache_clear_all('*', $table, TRUE);

function drupal_flush_all_caches() {
  // ...
  // previous code omitted
  // ...
  // Ordered so clearing the page cache will always be the last action.
  $core = array('cache', 'cache_path', 'cache_filter', 'cache_bootstrap', 'cache_page');
  $cache_tables = array_merge(module_invoke_all('flush_caches'), $core);
  foreach ($cache_tables as $table) {
    cache_clear_all('*', $table, TRUE);  --old code
    cache($table)->flush();               // New code
  }
  // remainder of function follows
}

works for now.
The new code should execute like this:

{
  cache('cache')->flush();
  cache('path')->flush();
  cache('filter')->flush();
  cache('bootstrap')->flush();
  cache('page')->flush();
}

in succession using the new API. As Drupal does not require a cache to exist, this may result in added cache rebuilds, but the function is drupal_flush_all_caches().

It does pose an additional question, which is the module_invoke_all('flush_caches') call. As I read it, this adds to the array $cache_tables the function 'flush_caches' points to where it exists in the list of modules contained in $core.

Overall, I think it's safe here to use the cache($table)->flush().

Regarding...

./includes/common.inc:  if (!empty($cid) && $cache = cache_get($cid, $bin)) {

$bin will resolve to either 'cache', or the value contained in $elements['#cache']['bin'] which is passed to the function as shown below:

  // ... previous stuff ...
  $bin = isset($elements['#cache']['bin']) ? $elements['#cache']['bin'] : 'cache';

  if (!empty($cid) && $cache = cache_get($cid, $bin)) {
  // ... more stuff ...

The question is whether the argument passed to $elements will always be good within the function. If $elements['#cache']['bin'] doesn't have a value, the value resolves to 'cache', which is all good. The issue is that $elements['#cache']['bin'] always needs to contain a valid cache bin if the value is present. Sounds like something to be tested for...

This also applies to:
./includes/common.inc: cache_set($cid, $data, $bin, $expire);
where

  $bin = isset($elements['#cache']['bin']) ? $elements['#cache']['bin'] : 'cache';
  $expire = isset($elements['#cache']['expire']) ? $elements['#cache']['expire'] : CACHE_PERMANENT;
  cache_set($cid, $data, $bin, $expire);

These seem to indicate that the same bin resolution applies, and items affected will persist unless $elements['#cache']['expire'] resolves to something else meaningful. Another thing to be tested, perhaps.

and for

./modules/simpletest/tests/cache.test:      cache_set($id, $this->default_value, $bin);

at least as far as resolving $bin is concerned.

I don't know simpletest enough to determine whether these are already being tested for or not, nor do I feel confident enough at this moment to write such a test if it can be written, but that's another issue.

Another roll-up.

It's almost done, whomever reviews, search for lines containing: "Proposed replacement:".

They are comments within the code, and there are only a few.

Any added feedback on whether I can make the replacements, please let me know. There are a couple of comments above #61 & #62 that outline my thoughts on the matter.

Thanks. That's a few hours of work that's ready for the grinder of full review I think.

To beejeebus, chx, crell, and bfroehle ...

Thanks loads for your help and support.

This is ready to go. Thanks folks!

This looks good. Thanks for all the work and the mentoring. Great to see. Committed to 8.x.

Continuing the conversation in #1275808: Use new cache bin naming in hook_flush_cache.