Convert prefix cache clears to cache tags, then remove support for them

#1: cache-delete-prefix-1.patch queued for re-testing.

I could only reproduce one of the test failures (the one in Drupal\system\Tests\Theme\ThemeTest) - and for some reason only when running tests using run_tests.php, not when using the UI. This has been fixed now. Let's try the other one (in Drupal\locale\Tests\LocaleTranslationTest) one more time.

Some thoughts from a review of the #5 patch if and when it is re-rolled.

+++ b/core/lib/Drupal/Core/Utility/CacheArray.phpundefined
@@ -71,6 +72,11 @@ abstract class CacheArray implements ArrayAccess {
   /**
+   * A tags array to pass to cache()->set().
+   */
+  protected $tags;

Missing '@var array'

+++ b/core/lib/Drupal/Core/Utility/CacheArray.phpundefined
@@ -71,6 +72,11 @@ abstract class CacheArray implements ArrayAccess {
+  /**
    * A bin to pass to cache()->set() and cache()->get().
    */

Ibid.

+++ b/core/lib/Drupal/Core/Utility/CacheArray.phpundefined
@@ -92,10 +98,13 @@ abstract class CacheArray implements ArrayAccess {
    *   The cid for the array being cached.
    * @param $bin
    *   The bin to cache the array.
+   * @param $tags
+   *   The tags to specify for all cache items.

Can we add type hinting to this docblock since you are adding another variable. Also this variable need '(optional) to start description as well as what happens in the default case.

+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.phpundefined
@@ -31,9 +33,10 @@ class ThemeRegistry extends CacheArray {
-  function __construct($cid, $bin) {
+  function __construct($cid, $bin, $tags) {
     $this->cid = $cid;

This needs a docblock describing what the @param variables being passed to the constructor are.

The test failure in LocaleTranslationTest seems to be caused by #1791920: Create new translations:// directory for tests, though I don't understand why that problem only causes a failure with the patch for this issue and not on a clean git checkout (anyone?). But in order to make the test bot happy, this patch includes the patch for #1791920.

@Lars Toomre: This patch addresses all your comments.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -714,11 +714,13 @@ abstract class TestBase {
+    $this->translation_files_directory = $this->public_files_directory . '/translations';
...
+    file_prepare_directory($this->translation_files_directory, FILE_CREATE_DIRECTORY);

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -632,6 +632,7 @@ abstract class WebTestBase extends TestBase {
+    variable_set('locale_translate_file_directory', $this->translation_files_directory);

Seems this is slipped in accidentally...

Otherwise look really great!

Those lines were a fix for #1791920: Create new translations:// directory for tests. These were included intentionally in order to the patch pass on the test bot (see comment #8). The fix for #1791920 was committed a few minutes ago, so here is a clean patch.

As mentioned over in #1791920, the test failure only occurred when running the two tests "Translation export" and "String translate, search and validate" in one batch, not when they were run independently. One possible reason for this (only an unverified theory) is that the static class variable DatabaseBackend::$tagCache is not cleared between each test run. A fix for this problem is included in the patch for another cache API improvement, #1774332: Better handling of invalid/expired cache entries.

Anyway, this patch should pass the test bot now.

@c960657: Sorry I hadn't read that, stupid me. Code-wise this is RTBC, but needs to be re-rolled obviously...

Thanks for the reviews and the RTBC. Here is a reroll.

It it allowed to update existing change records, or should we create a new one for this? It seems that this change is relevant to add in New, cleaner cache API.

Quickly updated the old changelog over at [#1272696]. Not sure if I nailed it. (Was my first ever change notice. /me hides) :P

On it.

This doesn't cover field_info yet (/core/modules/field/lib/Drupal/field/FieldInfo.php)

Okay that should cover field_info now too. Dunno if that was missed in the first place or added in the mean time. Probably the latter.

The latest additions look fine.

Cool. Let's get this commited asap. We are currently pushing forward quite fast in the whole Cache API world and could use this to unblock other issues.

Looks like this was done http://drupal.org/node/1272696/revisions/view/1832888/2378858 Thanks, fubhy!

Couple of questions:

+++ b/core/includes/entity.inc
@@ -83,7 +84,7 @@ function entity_get_info($entity_type = NULL) {
+      cache()->set("entity_info:$langcode", $entity_info, CacheBackendInterface::CACHE_PERMANENT, array('entity_info' => TRUE));

Shouldn't we have added a tag for 'language' => $langcode here and elsewhere?

+++ b/core/includes/theme.inc
@@ -332,7 +333,7 @@ function _theme_load_registry($theme, $base_theme = NULL, $theme_engine = NULL,
+    return new ThemeRegistry('theme_registry:runtime:' . $theme->name, 'cache', array('theme_registry' => TRUE));

Why didn't we tag these with 'theme' => $theme->name?

+ more questions identical to these two, but for other cache item key name ($cid) parts in this patch.

@29: Are we clearing anywhere using those, more granular tags? If not, then that is correct. We can add that of course if we ever need to clear on that more granular scale.

Hm.

1) Is there a significant overhead per (additional) tag in terms of performance?

2) I'd think and assume that the better "categorized" cache items of core are, the more opportunities might arise for us in core (and/or contrib) to improve the cache invalidation? In other words, if I was able to say "Everything that relates to language X is (guaranteed to be) tagged with 'language' => 'X', then I can perform a very granular but at the same time very effective tag invalidation for language X only across all cache bins. (Real world example for that would be to add or edit a string translation for language X.) [Whereas language is just one of many possible examples.]

3) Separate issue: Thinking further, I almost wonder whether the effective $cid couldn't be auto-generated, in order to significantly simplify the function parameters for get() and set(); e.g.:

$cid = 'entity_info'; + $tags = array();
    == 'entity_info';

$cid = 'entity_info'; + $tags = array('language' => 'X');
    == 'entity_info:b326b5062b2f0e69046810717534cb09';
 // == md5(json_encode(array_multisort($tags)))

cache()->set('entity_info', array('language' => 'X'), $info [, CacheBackendInterface::CACHE_PERMANENT] ));
// Making $tags the second argument to encourage usage.

1) Is there a significant overhead per (additional) tag in terms of performance?

That depends on the specific cache backend. For the database backend I think the overhead is small (the invalidate value for each tag is fetched only once per request).

2) [...] In other words, if I was able to say "Everything that relates to language X is (guaranteed to be) tagged with 'language' => 'X'

Instead of “relates to” I suggest “depends on”. If a cache item depends on some specific piece of data, we should invalidate the cache item when the underlying data changes.

A theoretical example: The cache keys could be table names. If we calculate a value by doing a "SELECT * FROM a JOIN b ON ..." we tag the cache item with "a" and "b". Whenever we write to a table (INSERT/UPDATE/DELETE), we delete all cache items tagged with the table name (this could even be done by the database layer).

This would additionally allow us to make an API for declaring cache dependencies, i.e. that invalidating one tag recursively invalidates other tags. Basically the page cache for node/123 depends on node 123. This node may have a declared dependency on its author, user 456, so calling ->invalidateTags(array('user' => 456)) would trigger a call to ->invalidateTags(array('node' => 123)) tag and thus the page cache item for that page will be invalidated.

3) Separate issue: Thinking further, I almost wonder whether the effective $cid couldn't be auto-generated

Your suggestion requires that you need to know all tags in order to call ->get(). This is not always the case, e.g. in the example provided in the documentation for ->set().

This is important issue that I, as a modest observer, would prefer not lag from a lack of core developer attention. How can I help move this issue forward?

@Lars: Which issue are you speaking off? The patch in this issue was commited already. :)