HAL links are broken if diffferent domains, protocols or ports are used in multisite or multi-domain setup

Adding related tags. It would be great if we add test for this as well.

Breaks default_content #2928952: Invalidate the type links cache after altering the link domain

Test

forgot to remove test tag.

This looks super vulnerable to cache thrashing ie. multiple clears of the cache on the same request. Should the cid have the link domain as part of it instead?

I don't know about "super" because I think it generally just get called settings up services but I guess is could basically nullify the cache making it more or less just overhead if it gets called on every run. So much for the trivial solution I guess...

Couldn't apply the patch but the idea is interesting. Changed it up a bit and fixed the tests. Seems to mostly address Alex's concerns with out a seismic shift in the way we cache hal information.

To add a little extra thrash protection I compare the domain to the previous one as well. So if some code is just continuously setting the same domain we're protected.

Resolved one coding standard issue with an interdiff.

I don't have a good enough understanding of HAL type links and caching to intelligently review the code - but, it fixed a problem for me where after a drush site-install with our custom entity and *slightly* customized REST endpoints, a PUT of our entity would fail with "Type https://example.ddev.site/rest/type/our_entity/our_entity does not correspond to an entity on this site" unless we did an extra drush cr first. One thumb up.

+++ b/core/modules/hal/src/LinkManager/TypeLinkManager.php
@@ -99,6 +99,29 @@ public function getTypeUri($entity_type, $bundle, $context = []) {
+    // If we're changing domains we may need to invalidate the cache. Look and
+    // see if there are any old domains in the cache and if so invalidate.
+    if ($old_domain != $domain) {
+      $cache_values = $this->cache->get('hal:links:types');
+      if ($cache_values) {
+        foreach (array_keys($cache_values->data) as $bundle_uri) {
+          if (strpos($bundle_uri, $old_domain) === 0) {
+            $this->cache->invalidate('hal:links:types');
+            break;
+          }
+        }
+      }
+    }
+    return $this;

wouldn't it be easier to just make the link domain part of the cache key, aka making it a poormans-cache context? would just work then.

Also, The cacheability of the resulting page also doesn't correctly include the cache tag yet if the setting is currently empty.

#15 fixed my issue of import with default_content_deploy but but only using latest release (1.0-beta2 from april 15th) default_content_deploy 8.x-1.0-beta2
and including --uri parameter to my drush command to avoid this

vagrant@devd8:/var/www/drupalvm/d8vm/rd8.vbox.local/public_html$ drush dcdi --force-override
In ContentEntityNormalizer.php line 268:
 Type http://default/rest/type/block_content/basic does not correspond to an entity on this site.  using 

using

drush dcdi --uri=http://mysiteurl.com --force-override

instead works.

Tested on core 8.8.5

One Drupal installation can serve multiple domains, protocols or ports. The current implementation in core regarding the cache is simply wrong. Also the approach in the proposed patch is wrong and is not robust against race conditions.

Here's a failing patch first.

I think berdir's proposal from #18 is the correct way to fix the issue without any race conditions or performance loss.

But as mentioned before, the current implementation must be considered to be broken. Multisite or multi-domain setups will fail.
Contrib modules like Default Content, Default Content Deploy, Domain Access and other are currently affected.
In core language negotiation by domain is incompatible for example.

Relations are also affected, not just types.

Another try of a failing test.

I missed to fetch the cached values in the previous tries of a failing patch.

Now the patch to solve the issue.

+++ b/core/modules/hal/src/LinkManager/RelationLinkManager.php
@@ -155,7 +158,7 @@ protected function getRelations($context = []) {
    */
-  protected function writeCache($context = []) {
+  protected function writeCache($cid, $context = []) {
     $data = [];
 

changing the method just so we don't have to call getLinkDomain() a second time doesn't really seem worth it?

changing the method just so we don't have to call getLinkDomain() a second time doesn't really seem worth it?

I checked the sources and didn't find any other calls of this protected method. But I don't care. If you'll set RTBC I'll change it to be be 100% backwards compatible.

BTW I run into this issue because of default_content_deploy.

Pretty sure it's much more likely to be committed without that change. We try to avoid changing protected methods, keep deprecated protected methods around and so on. There are known subclasses of this, it's theoretically possible that a custom project overrides or calls it in a subclass?

Patch looks good to me, we will wait for testbot to confirm.

@berdir RTBC?

Yeah, I think this is RTBC. fixes the problem with minimal changes and has test coverage.

+++ b/core/modules/hal/src/LinkManager/TypeLinkManager.php
@@ -152,7 +152,7 @@ protected function writeCache($context = []) {
+    $this->cache->set('hal:links:types:' . $this->getLinkDomain($context), $data, Cache::PERMANENT, ['entity_types']);

Should this get a cache tag for hal.settings?

https://api.drupal.org/api/drupal/core%21modules%21hal%21src%21LinkManag...

Also doesn't look like there's test coverage for the link domain being changed in config.

Should this get a cache tag for hal.settings?

I don't think so. The cache must not be invalidated when link_domain in hal.settings changes. This was an issue before this patch!
Now the cache entry can remain regardless that the link domain changed and regardless how it changed, via settings or via API calls at runtime.

If you're argument is to save cache space when the setting changes, then we can add the tag.

Cache tags don't clear space. And yes, the configured url is basically a cache context now with this patch.

Oh good point didn't think it all the way through, so yeah when the settings are changed the cache key changes and everything will work fine.

Committed 7dabb04 and pushed to 9.3.x. Also cherry-picked to 9.2.x. Thanks!