DomainAccessCheck prevents menu tree render arrays from being cached

How can I reproduce this issue for testing?

I think changing '->setCacheMaxAge(0)' to '->addCacheableDependency($domain)' should be enough; cache dependencies will do the rest.

Steps to reproduce:
1) Create a custom menu.
2) Add menu link to a node.
3) Add menu block to the page.
4) Optionally, install/enable renderviz module and ensure block max-age is 0 (should be -1 normally).

Resolving the menu link access will lead to resolving the node_view access check which in turn calls '\Drupal\domain\Access\DomainAccessCheck::access()'.

Please try this patch drclaw.

Thanks. I've been on vacation and will take a look next week.

I don't understand what I'm looking for here. renderviz doesn't seem to be returning useful information.

See attached.

Hi @agentrickard,
Sorry I wasn't clear enough.

Try checking the page source HTML; renderviz adds some debugging comments in a way similar to how Twig debug does. I don't think obtaining max-age via renderviz js is possible.

You should see max-age 0 without a patch and -1 + cache metadata (most notably domain record context) with patch applied.

Thanks! That's an awesome tool.

I am consistently seeing the cache tag applied to the first loaded domain (in this case two.example.com) and then appearing the same when one.example.com is loaded.

This is identical on both one.example.com and two.example.com for an anonymous user.

<!--{"contexts":["user.permissions"],
"tags":["config:domain.record.two_example_com","node:1","node:2","config:system.menu.my-menu"],
"max-age":-1}-->

I wonder if this approach would also work?

I think the above url.site approach will work. I _think_ the other fails because we don't set unique cache contexts per domain since the url.site core tag handles it for us.

I would rather stick with adding domain as a cache dependency and here's why.
You never know what 3rd party custom module may do. Imagine someone wants to replace the active domain based on permissions or user roles or phase of the moon; the 'url.site' approach will not work since the developer may extend domain entity and expect cache metadata to bubble properly.

However, I think we should investigate why you're getting the cache tag from a domain which is different to active one.
Did you really access the site using a different domain (e.g. url.site) or did you just change the default domain? That may be a different bug; could you please write simple steps to reproduce? I'm going on a vacation for 2 weeks but I could have a look after I'm back.

Just thought. In any case, simply adding a context is not enough since anything rendering within a domain should depend on a domain settings. So we need the 'config:domain.record.name' cache tag in any case which is normally supplied by cache dependency.

I'm switching domains as anon user via the Domain Switcher block.

Steps to reproduce:

1) Install Domain and create five domains.
2) Create a custom menu.
3) Create and add menu link to a node.
4) Add menu block to the page.
5) Enable renderviz
6) Enable the Domain Switcher block
7) Give anon users access to the Domain Switcher block
8) Click from domain to domain and check the output of renderviz

Not sure about the $domain dependency for 3rd parties. url.site provides the same context and is recommended for use with Domain Config.

This is all on a standard profile installation of Drupal 8.6.

The patch failed to apply using composer. Here is a re-roll without changes.

Apart from that the patch solve a significant performance issue on a production site.

In case there are no open tasks like tests etc here, I'd RTBC this patch.

Thanks!

Sorry, looking at this again, we still have to validate that the proper cache tags are being applied. Comments #8 and #13 are why we haven't committed this yet.

Can you confirm that caching is being applied as expected?

I can't reproduce the cache tag / renderviz issue. I'm tempted to commit this.

Committed.