Shortcut/ShortcutSet entity types should use cache tags

Shortcuts are always rendered in a Shortcut Set. Just like Menu Links are always rendered in a Menu.

Therefore, it may make sense to discard Shortcuts' own cache tags entirely and just always make them refer to the Shortcut Set's cache tags.

i.e. something like this in MenuLink::postSave() could also be applied in Shortcut::postSave():

    Cache::invalidateTags(array('menu' => $this->menu_name));
    if (isset($this->original) && $this->menu_name != $this->original->menu_name) {
      Cache::invalidateTags(array('menu' => $this->original->menu_name));
    }

Also see #2241291-1: Regression: menu link-specific cache tags, for the other half of what needs to happen for #2.

In #2254865: toolbar_pre_render() runs on every page and is responsible for ~15ms/17000 function calls, we could render cache shortcuts, but only if this lands, otherwise old shortcuts would be served to the end user in the toolbar. So let's get this done.

I had to update \Drupal\system\Tests\Entity\EntityCacheTagsTestBase, because Shortcut is a bit of a special snowflake: it has no view builder, and EntityCacheTagsTestBase was assuming that every entity it'd test would have that. EntityCacheTagsTestBase was also assuming that an entity's cache tag would be based on that entity's entity type, but Shortcut and MenuLink are the special snowflakes in that regard — fortunately we now have EntityInterface::getCacheTag(), so that's easily solved.

Silly mistake.

#6 failed because of a hilarious bug in current D8 HEAD: if you go to admin/config/user-interface/shortcut/add-set, reload that page a few times (say 10 times), then you've now created 10 duplicates of each shortcut in the "default" shortcut set. But those shortcuts are not associated with anything. Because when you go to that page, it'll automatically duplicate those shortcuts, even if you don't actually create a new shortcut set!
Because of this silly behavior, no cache tags could be generated for those duplicated shortcuts. Simply duplicating those shortcuts after the shortcut set is *actually* created fixes the problem :)

This prevents better cacheability of shortcuts, and prevents #2254865: toolbar_pre_render() runs on every page and is responsible for ~15ms/17000 function calls from making a bigger improvement. Increasing priority accordingly.

Oops, #6 and #8 didn't include ShortcutCacheTagsTest… restoring that file. Plus, #2099131: Use #pre_render pattern for entity render caching broke this. Rerolled.

An updated issue summary should help with reviewing.

Thanks for the review! :)

1. No, because cache tags are of the form array('entity type' => 'entity ID'). So $a = array('shortcut' => array(1)) and array('shortcut' => array(2)); $a + $b === array('shortcut' => array(1)) which is wrong, NestedArray::mergeDeep($a, $b) === array('shortcut' => array(1, 2)), which is right.
2. Well-spotted, sir! The ->isNew() is unnecessary there. The other part is necessary though: $this->path is only set when an entity is created, not when an entity is loaded. Therefore, it's necessary to prevent this from fataling: $shortcut = entity_load('shortcut', 343); $shortcut->save();, which happens as part of \Drupal\system\Tests\Entity\EntityCacheTagsTestBase::testReferencedEntity().
   The proper solution would be to not have $this->path at all, but to always use routes in the Shortcut entity. I first considered that out of scope, but since you brought it up, I might as well fix it. Fixed.
3. The code you cite does not have the list tag (array('shortcut_sets' => TRUE), but the entity's unique tag (array('shortcut_set' => '<id>'))! The entity's unique tag is invalidated only upon update by Entity::invalidateTagsOnSave(), but we need it to be invalidated upon creation as well: when a shortcut is created, it's always associated with a shortcut set, and since a shortcut uses the associated shortcut set's cache tags, we must invalidate that tag.
4. Hm… this is something else entirely: this is how Shortcut tells Drupal to use the associated ShortcutSet's cache tags instead. I think you're confused? :)
5. No, this is correct also. Shortcut::getCacheTag() returns ShortcutSet::getCacheTag(), and Shortcut::getListCacheTags() returns ShortcutSet::getListCacheTags(). Simple forwarding. I think you thought ::getListCacheTags() is a superset of ::getCacheTag(), but it's not.
6. Haha, oops! I thought this was a supposed standard, but I don't care. Removed.

Clearly, I didn't test #13.2 sufficiently. In trying to fix it, I tried to understand the complex relationship that Shortcut has with a "path" field. Its "path" field is a computed field that is actually used for input to compute the actual values to be stored, which are then used to compute the path when editing again… I found a solution for that, but then I ran into problems with MapItem, which I suspect is buggy and may also be used incorrectly.
I spent more than three hours trying to get this to work, digging through the many layers of Entity/Field API (with many layers of "setting" and "getting" values) … and just when I was about to give up, I found #2235457: Use link field for shortcut entity, which seems to solve all of this :) A nice 3 hours wasted. I learned my lesson: I'll look for existing issues sooner next time. (In case it turns out to be useful after all: I've attached an "out of scope" interdiff.)

So this reroll:

• reverts everything from #13 except the fix for #12.6.
• does apply the clean-up for #12.2, with the caveat that this means that a shortcut entity whose path is the front page gets loaded and immediately saved, will cause a fatal, but that's up to #2242535: Saving a shortcut set with empty or <front> link fails with an error to fix (which I also discovered only now, unfortunately), and hopefully #2235457: Use link field for shortcut entity already fixes it.
• I sidestep the above problem by using the "admin" path in the shortcut cache tags test

All this slowdown is all due to out-of-scope issues. Let's focus on the issue at hand: make shortcuts use the cache tags of shortcut sets, and make sure they're invalidated correctly.

Talked to amateescu in IRC, turns out I completely misunderstood what he meant in #12.4 & #12.5. See the interdiff for what he meant :)

Much cleaner now! :)

Great! Now #2254865: toolbar_pre_render() runs on every page and is responsible for ~15ms/17000 function calls is fully unblocked :)

#23: no, that's completely independent of what this issue tackled.