Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache

Only nitpick I founf is that PageCacheIntegrationTest should have the word 'Tags' in it since it isn't a thorough test of whole page cache system.

+++ b/core/includes/common.inc
@@ -3562,6 +3562,27 @@ function drupal_prepare_page($page) {
+  if (theme_get_setting('features.main_menu') && count(menu_main_menu())) {
...
+  }

Looks like menu_main_menu() and menu_main_menu() does exactly same calculation what we do inside this if statements. Would be great, if we can avoid and make use of main. Something like this - https://gist.github.com/vijaycs85/8690309

So this patch will become below

+++ b/core/includes/common.inc
@@ -3562,6 +3562,27 @@ function drupal_prepare_page($page) {
+  if (theme_get_setting('features.main_menu') && count(menu_main_menu())) {
+    $page['page_top']['#cache']['tags']['menu'][$main_links_source] = menu_get_Links_source('main_links', 'main');
+  }
+  if (theme_get_setting('features.secondary_menu') && count(menu_secondary_menu())) {
+    $page['page_top']['#cache']['tags']['menu'][$secondary_links_source] = menu_get_Links_source('secondary_links', 'account');
+  }

Updates:
2179083-menu-cache-tag-8-reroll.patch is just reroll
2179083-menu-cache-tag-8-with-changes.patch has changes mentioned in #7

1. +++ b/core/includes/menu.inc
   @@ -1786,6 +1783,23 @@ function menu_secondary_menu() {
   +function menu_get_Links_source($name, $default) {
   

   WOW, you even can use "Links" without breaking something. Let's better name it to 'links' to stay sane.

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Cache/PageCacheIntegrationTest.php
   @@ -0,0 +1,105 @@
   +class PageCacheIntegrationTest extends WebTestBase {
   

   Is there any reason we create a new test even we do have PageCacheTest already?

TEST PREPARATION:

• Page cache maximum age ==> 86400/1 day
• Enabled the internal page cache.
• Tested basic page cache and cache busting, all good.

Warning for co-testers: because of the cache-control policy set on the outgoing responses your browser will try to fetch it from local cache till expiry reached, therefore always refresh using "control+shift+R" to assure a forced re-fetch.

WITHOUT menu_cache_tags-2179083-15.patch APPLIED:

1. Generated cached front page, created page+link (pri.links). After saving the node and hard-refresh the front-page showed tabbed link: CORRECT.
2. Frontpage refreshed in cache, renamed menu link: INCORRECT

.. stopped tests as main original problem was confirmed to me, which emphasizes why this patch is so important.

WITH PATCH APPLIED AND AFTER CLEARING CACHES:

1. Generated cached front page, created page+link (pri.links) . After saving the node and hard-refresh the front-page showed the tabbed link: CORRECT.
2. Frontpage refreshed in cache, renamed menu link: CORRECT.
3. With warmed frontpage I created a second page+link (pri.links). After saving it is expected that both the frontpage and the older page with menu link is expired: CORRECT
4. When a menu link gets disabled, all pages showing the link in the main menu bar should see it disappear from the page cache: CORRECT.
5. More direct menu link manipulation through the admin (/admin/structure/menu/item/$MLID/edit) should expire all pages showing the link: CORRECT
6. Created a new page+link where the link has another link as parent, making it not show up in the primary links naturally. Expected behavior is that this does NOT expire the parent page as long as the rendered link is not shown on it (e.g. menu block): INCORRECT
   I had "node/2" that had "Test #1" as title and the link was as tab in primary links. I created "node/4" with a link that had the tabbed link from "Test #1" as parent, which means that - as long as the menu tree isn't rendered on any of the pages - the parent page should NOT expire, as the child was not rendered on it.

FOOTER MENU + BLOCK WITH LINKS RENDERED IN THEM:

1. With three warmed pages in cache and all showing the footer menu in the footer region rendered, we expect that a change to any of the links in them, wipe all three warmed pages: CORRECT
2. With warmed pages in cache we expect a new link in the footer menu to wipe all pages: CORRECT
3. With warmed pages we expect a deleted link to wipe all pages: CORRECT

NEXT: I will take a stab at writing test cases for all documented test steps above, however, I'll timebox my effort and might decide to stop it, lets hope it'll works out.

There is one important thing that I didn't mention in my last comment:
bullet #6 causes more paths to be expired than theoretically necessary but the costs of runtime-level detection whether a sibling menu link is rendered somewhere in the HTML is barely doable. Therefore having this patch behave the way it currently does far outweighs the cost of currently common scenarios where sites run with low TTLs and expiry of many pages is still MUCH cheaper than constant full-site expiry through a way too low TTL.

In other words, its a theoretic thing I noticed but this patch is totally RTBC in my opinion.

+++ b/core/includes/common.inc
@@ -3562,6 +3562,19 @@ function drupal_prepare_page($page) {
+  // directly by the theme system.
...
 
+  // The "main" and "secondary" menus are never part of the page-level render
+  // array and therefor their cache tags will never bubble up into the page
+  // cache, even though they should be. This happens because they're rendered

+++ b/core/includes/common.inc
@@ -3562,6 +3562,19 @@ function drupal_prepare_page($page) {

Is there any way to not couple the very specific details of having specific menus on basically every page onto the drupal_prepare_page() method which feels like part of an API or at least architecture.

Yeah that issue is #1869476: Convert global menus (primary links, secondary links) into blocks, while it's ugly hard coding those menus, it's not the fault of this patch. (not committing this yet because I haven't reviewed it).

1. +++ b/core/includes/common.inc
   @@ -3562,6 +3562,19 @@ function drupal_prepare_page($page) {
   +  // array and therefor their cache tags will never bubble up into the page
   

   therefore.

   Could use a @todo to remove this when they're blocks.

2. +++ b/core/includes/menu.inc
   @@ -1786,6 +1783,23 @@ function menu_secondary_menu() {
   +  // When menu module is not enabled, we need a hardcoded default value.
   

   The default is a parameter to the function, so it's not really hardcoded?

3. +++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
   @@ -457,6 +457,66 @@ public function testBlockContextualLinks() {
   +  public function testMenuBlockPageCacheTags() {
   

   Looks like this is checking block cache tags, but not those from the secondary/main menus?

Thanks for docs fixes/re-rolls. Committed/pushed to 8.x, thanks!