HAL RelationLinkManager caches and returns entity type definition object instead of id

Also related, this one has the same problem as TypeLinkManager used to have: #2567133: getTypes in TypeLinkManager doesn't work with the null cache back end

This changes the return value, but I can't imagine that anyone relies on this, and based on what I found, we only seem to care about field names?

Our installer actually broke in 8.3 somehow because we have entity type objects with "loaded" StringTranslation objects (that's my guess at least) that then get serialized including their service and then it explodes with a cannot serialize container exception.

I wonder whether this actually could cause some fatals, which might be a reason for this to be critical.

Here is some fun code:

  protected function getRelations($context = []) {
    $cid = 'hal:links:relations';
    $cache = $this->cache->get($cid);
    if (!$cache) {
      $this->writeCache($context);
      $cache = $this->cache->get($cid);
    }
    return $cache->data;
  }

Durpal, the only system which writes to the cache, and then reads from it before its doing anything else.

Here is some test coverage for this issue.

We had a fatal in our install profile, starting with 8.3, not sure what makes the difference, not sure about critical though.

But yeah, all this is very Durpal indeed.

Note: #4 is a test only patch ... feel free to take it.

This code has not been changed in eons, why is this causing problems *now* all of a sudden?

IOW: why is this major? Why is this not normal or even minor? How can this cause problems? What are the STR to even cause problems?

#4 has tests (Thanks @dawehner!), we just need to merge it together.

It's IMHO major because it can result in serialization exceptions under some cases, see #3. In our case it is during installation, called by default_content. Hard to reproduce, not sure why it only happened for us and only since 8.3, but it does and when it does then there is no workaround.

Just combined the two patches together.

+++ b/core/modules/hal/tests/src/Kernel/HalLinkManagerTest.php
@@ -51,14 +71,32 @@ public function testGetRelationUri() {
-    $link = $relation_manager->getRelationUri('node', 'page', 'foobar', ['hal_test' => TRUE]);
+    $link = $relation_manager->getRelationUri('node', 'page', 'foobar',
+      ['hal_test' => TRUE]);
...
-    $link = $relation_manager->getRelationUri('node', 'page', 'foobar', ['rest_test' => TRUE]);
+    $link = $relation_manager->getRelationUri('node', 'page', 'foobar',
+      ['rest_test' => TRUE]);

@@ -68,7 +106,8 @@ public function testHalLinkManagersSetLinkDomain() {
-    $this->assertEqual($link, 'http://example.com/rest/relation/node/page/field_ref');
+    $this->assertEqual($link,
+      'http://example.com/rest/relation/node/page/field_ref');

Automatic IDE changes? I think we want to revert these.

Then this is RTBC.

Thanks @Ginovski!

Unrelared

+++ b/core/modules/hal/src/LinkManager/RelationLinkManager.php
@@ -128,7 +128,7 @@ protected function writeCache($context = []) {
-              'entity_type' => $entity_type,
+              'entity_type' => $entity_type->id(),

+++ b/core/modules/hal/tests/src/Kernel/HalLinkManagerTest.php
@@ -59,6 +79,22 @@ public function testGetRelationUri() {
+    $internal_ids = $relation_manager->getRelationInternalIds($link);
+
+    $this->assertEquals([
+      'entity_type' => 'node',
+      'bundle' => 'page',
+      'field_name' => 'field_ref'
+    ], $internal_ids);

This results in an API change :(

Something somewhere could be relying on \Drupal\hal\LinkManager\RelationLinkManagerInterface::getRelationInternalIds() actually returning an entity type object.

I think we have to care about BC here.

Maybe we should cache the ID but on get load the entity type to preserve the API. But also add an entity_type_id key to the array and deprecate entity_type.

Sorry. BC is hard.

Hm. The documentation fairly clearly states that it returns ids:

   * @return array
   *   An array of typed data ids (entity_type, bundle, and field name) keyed
   *   by corresponding relation URI.

I think it's more likely that someone runs into errors because they use it as documented. But fine, I'll think of something, will require rewriting some of that code, will also improve it so the caching isn't so weird.

Also, core only seems to ever use the field names from this method, which is why nobody notices this I guess.

Came up with the idea of adding entity_type_id and only then noticed that @alexpott suggested exactly that. I guess that's the only option that makes sense when keeping BC.

Not sure how to document that a single key of the return value of a method is deprecated.

Also not sure about 8.3, but this is a bug that is causing fatal errors for us in our installer, as mentioned, I'm not 100% clear why exactly.

It is a bit sad that this patch now fixes two things at the same time (the return value and the caching problem), but I guess this is how it is.

Committed 4ad11bc and pushed to 8.4.x. Thanks!

I credited @dawehner, @Wim Leers and myself for review comments.

I think this should be backported to 8.3.x but I'm going to ask other committers first.

diff --git a/core/modules/hal/src/LinkManager/RelationLinkManagerInterface.php b/core/modules/hal/src/LinkManager/RelationLinkManagerInterface.php
index 1b5c37d..8ba8c94 100644
--- a/core/modules/hal/src/LinkManager/RelationLinkManagerInterface.php
+++ b/core/modules/hal/src/LinkManager/RelationLinkManagerInterface.php
@@ -32,9 +32,9 @@ public function getRelationUri($entity_type, $bundle, $field_name, $context = []
    *   Relation URI (or IANA link relation type) to transform into internal IDs.
    *
    * @return array
-   *   Array with keys 'entity_type_id', 'bundle' and 'field_name'. For backwards
-   *   compatibility, the entity_type key returns the full entity type object,
-   *   this will be removed before Drupal 9.0.
+   *   Array with keys 'entity_type_id', 'bundle' and 'field_name'. For
+   *   backwards compatibility, the entity_type key returns the full entity type
+   *   object, this will be removed before Drupal 9.0.
    */
   public function getRelationInternalIds($relation_uri);
 

80 chars - fixed on commit.

diff --git a/core/modules/hal/src/LinkManager/RelationLinkManager.php b/core/modules/hal/src/LinkManager/RelationLinkManager.php
index dfda71f..282dfde 100644
--- a/core/modules/hal/src/LinkManager/RelationLinkManager.php
+++ b/core/modules/hal/src/LinkManager/RelationLinkManager.php
@@ -111,7 +111,7 @@ protected function getRelations($context = []) {
     if (!$cache) {
       $data = $this->writeCache($context);
     }
-     else  {
+    else {
       $data = $cache->data;
     }
 

Spacing - fixed on commit.

I just had the same error on another install profile, when trying to install in german.

The patch in #18 seems to have been tested against 8.3.x, so setting back to RTBC as the patch applies there as well.

Thanks for the coding standard fixes, I was apparently a bit lazy. Might make sense to cherry-pick the commit then :)

In discussion with @xjm about backporting the patch @xjm pointed out that the docs weren't clear about how this change was not a significant API break. I've tried to fix this on #2877593: Improve \Drupal\hal\LinkManager\RelationLinkManager::getRelations() documentation

So given that storing entity types in the cache is resulting in failed installs I've backported this fix to 8.3.x

publish the change record