Setting cache tags can be tricky: use strings instead of nested arrays to improve DX

I'd be OK with this.

The idea for allowing a prefix + array was for things like array('node' => array_keys($nodes));. Now that we have getCacheTag() on entities, that use case doesn't really exist as much, and the 'flattening' of cache tags has come up in a couple of places already as an issue.

I'm not sure right now, if you have cache tags like 'node' => array(1, 2, 3), do we support to clear all them?

I'll also leave znerol a ping, he has written a redis cache backend, want to make this is not a problem for him.

I'm not sure right now, if you have cache tags like 'node' => array(1, 2, 3), do we support to clear all them?

Nope, it's just convenience for setting them. They get stored/treated as individual tags in the implementations.

Re #3: The redis module has a copy of flattenTags() which converts the ones containing an array to strings. After this patch we simply can remove this method and everything will be fine.

Maybe we should keep an implementation of flattenTags so people can use it if they are not doing $entity->getCacheTags()/working with entities?

In general, this seems to simplify things a lot. Only not sure about some places that are now using merging that doesn't seem necessary.

1. +++ b/core/includes/common.inc
   @@ -3152,8 +3117,7 @@ function drupal_render_cache_set(&$markup, array $elements) {
   -  $cache_tags = $elements['#cache']['tags'] ?: array();
   -  $cache_tags['rendered'] = TRUE;
   +  $cache_tags = Cache::mergeTags($elements['#cache']['tags'] ?: [],  ['rendered']);
   

   This looks strange, why are we doing that conditional? if tags is not set, wouldn't this cause a notice?

2. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -193,7 +193,11 @@ public function rebuild(array $definitions) {
   -    Cache::invalidateTags(array('menu' => $affected_menus));
   +    $cache_tags = [];
   +    foreach ($affected_menus as $affected_menu) {
   +      $cache_tags[] = 'menu:' . $affected_menu;
   +    }
   +    Cache::invalidateTags($cache_tags);
   

   How often does this happen? Is it worth to build something like Cache::buildCacheTags($prefix, array $ids) ?

3. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
   @@ -72,7 +73,7 @@ public function render(HtmlFragmentInterface $fragment, $status_code = 200) {
   -      $tags['rendered'] = TRUE;
   +      $tags = Cache::mergeTags($tags, ['rendered']);
          $page->setCacheTags($tags);
   

   Here we don't do it conditionally.

4. +++ b/core/modules/block/tests/modules/block_test/block_test.module
   @@ -27,7 +28,7 @@ function block_test_block_view_test_cache_alter(array &$build, BlockPluginInterf
   -    $build['#cache']['tags'][\Drupal::state()->get('block_test_view_alter_cache_tag')] = TRUE;
   +    $build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], [\Drupal::state()->get('block_test_view_alter_cache_tag')]);
   

   Do we really need to make it so complicated everywhere? It might be merged anyway somewhere, so why bother?

5. +++ b/core/modules/book/src/BookManager.php
   @@ -451,7 +451,7 @@ public function deleteFromBook($nid) {
   -    \Drupal::cache('data')->deleteTags(array('bid' => $original['bid']));
   +    \Drupal::cache('data')->deleteTags(array('bid:' . $original['bid']));
   

   Wrong cache tag deletion. A few times in here.

6. +++ b/core/modules/comment/src/Tests/CommentDefaultFormatterCacheTagsTest.php
   @@ -101,17 +102,16 @@ public function testCacheTags() {
   -    $this->assertEqual($build['#cache']['tags'], $expected_cache_tags, 'The test entity has the expected cache tags when it has comments.');
   +    sort($expected_cache_tags);
   +    $this->assertEqual($build['#cache']['tags'], $expected_cache_tags); //, 'The test entity has the expected cache tags when it has comments.');
   

   nice trick with the comment ;)

7. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -426,10 +426,10 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -      Cache::invalidateTags(array('theme' => $values['theme']));
   +      Cache::invalidateTags(array('theme:'));
   ...
        }
   

   Missing something here.

8. +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsTestBase.php
   @@ -14,7 +14,7 @@
    abstract class PageCacheTagsTestBase extends WebTestBase {
   -
   +protected $dumpHeaders = TRUE;
   

   not sure if this is supposed to stay, needs to be formatted properly then.

1. Yes, it was the same logic, but the logic was strange, I didn't understand what the conditional was for? It was not a notice-safe (empty/isset) safe check, so the only case where it could fire if tags is set to FALSE/NULL/empty string and I'm not sure why we support that? If the array tags would not be set, then it would trigger a notice?

Anyway, looking at the interface, I see that you removed the check, so looks fine to me now.

2. I'm not sure, but as @catch said, this was the main reason we went with the current structure in the first place, to avoid code like that. Having a helper method for that would be a nice counter-argument if someone says the old API was easier for those use cases. Agreed that just three use cases is not much.

4.
Now we have:

 $build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], [\Drupal::state()->get('block_test_view_alter_cache_tag')]);

And we could also do

$build['#cache']['tags'][] = \Drupal::state()->get('block_test_view_alter_cache_tag');

Which seems a lot easier to me and merging there seems pointless, as a) it is only test code and we know that it is unique and b) If it would not be, then it could also get duplicated in a different part of the render array, so why bother to make it unique?

I don't care that much (again) but I'm pointing it out for two reasons: a) There are other conversions where we don't explicitly merge together, it's not clear from looking at the patch when you should do it and when not, so we should have clear rules what you need to do. b) It's example where the after looks more complicated than HEAD, that's something reviewers/committers could push back?

I went through most of the changes here, it's a few patterns repeated many times.

I think I've been the only one doing code reviews here, so I'm a bit careful about RTBC'ing myself. But I'm +1 on that. So if someone else could have a look over it, then I think this can go in.

We have a number of critical overlapping/related cache tag issues, and no matter in which direction we're moving, they are going conflict badly, so best option is just to get them in one by one and then re-roll the others. In that light, I'll RTBC in the next days if nobody else does.

Looks great to me.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -112,7 +112,7 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array
+      'tags' => Cache::mergeTags($tags, []),

+++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
@@ -138,8 +138,8 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array
+      'tags' => Cache::mergeTags($tags, []),

Another an empty array just for the validation provided from mergeTags is not intuitive or obvious. Seems like a strange pattern. Shouldn't that just validate the tags?

Seems crazy to remove flattenTags to just create a follow up to add pretty much the same thing back in, but ok...

#25 looks it needs fixing.

Unless I'm missing something Cache::mergeTags() will work fine with a single array, and validateTags() is public so could be used.

validateTags() was replaced with mergeTags() as that also ensures uniqueness, I guess we could also make validateTags() do that?

Seems crazy to remove flattenTags to just create a follow up to add pretty much the same thing back in, but ok...

The method that I suggested has nothing to do with current flattenTags(), tags are already flattened, all the time, everywhere.

What I suggested is a method to build array('node:1', 'node:2') from $ids = array(1, 2), something like Cache::buildTags($prefix = 'node', $ids = array(1, 2)).

should be pretty easy to add that here, but I'm also OK with a follow-up.

Nice, thanks Wim.

It doesn't make too much difference but do you think an array_map implementation would be better? functionally no difference. So prob doesn't matter.

Do you think we should throw our own message if e.g. an object is passed in? Otherwise it will throw a (recoverable?) fatal error and we'll get our friend the Drupal 'The website has encountered an error. Please try again later.' page?

Re berdir in #28; yes, not the same is fine but it is a substitute for flatten tags as you have a helper to create the strings for you? Just using a prefix instead of an array key...

Ok, we're green again and I think good to go, if @damiankloip is happy with the interdiff from #30.

Re: the function: Yes, both result in strings, but it is very different from before because the context it is used in and who is using it is completely different. So, IMHO, we're not replacing flattenTags() with buildTags(), we're removing flattenTags() because it is no longer useful and are adding a new method for a new use case :) But that's a detail, apparently we're both happy with buildTags() as it is now.

Right the problem that both were trying to solve is the same (having to loop over entity IDs to build cache tag strings manually), but we've taken it out of the cache storage API, and discovered it's less of a problem after applying cache tags everywhere than was expected when cache tags originally went in.

Patch looks great now, committed/pushed to 8.0.x.

Thanks Wim, This is great!

looks like a copy and paste error got in here.... check out #2364555-3: Add @covers annotation, fix some --strict for PHPUnit

Re-opening this as a followup. Apparently this issue introduced a copy-paste error in core/tests/Drupal/Tests/Core/Cache/CacheTest.php, where testInvalidateTags() is really testing deleteTags().

Here's a teeny tiny patch to rectify this.

I've moved this change out of the newest patch in #2364555: Add @covers annotation, fix some --strict for PHPUnit.

committers might prefer a separate issue for this follow-up... but maybe not. the correction looks fine to me though.

The followup is fixing a bug which is why a new issue is ALWAYS preferable to doing a followup. I can't change the category because the original was a task and not a bug and that is the majority of the work. If we weren't committing major tasks right now then I have to spend time on this issue explaining why this is an exception. So people please please please open followups as new issues and put a comment on the old issue and relate the two issues.

Committed e95124d and pushed to 8.0.x. Thanks!

OK, will make followup next time. Thanks.