Remove the multiple comment optimization from rdf.module

I suspects the current situation also has cacheability issues which are solved by reverting #687712-32: Optimize rdf.module for displaying multiple comments.

First testing; commenting later.

The thing with the current code is, it's wrong although I'm not completely sure in what way it should be fixed if it stayed in place:

+++ b/core/modules/rdf/rdf.module
@@ -224,28 +224,6 @@ function rdf_comment_storage_load($comments)
-    // The current function is a storage level hook, so avoid to bubble
-    // bubbleable metadata, because it can be outside of a render context.
-    $comment->rdf_data['entity_uri'] = $entity->toUrl()->toString(TRUE)->getGeneratedUrl();
-    if ($comment->hasParentComment()) {
-      $comment->rdf_data['pid_uri'] = $comment->getParentComment()->toUrl()->toString();
-    }

That second toString() can cause exceptions to be thrown if it's ever called from certain points in the code*. And it's entirely possible for code to indirectly call rdf_comment_storage_load() from such a point - even though Core doesn't do this. (See also: above code comment.)

However I'm doubtful that the solution is to change the ->toString() to ->toString(TRUE)->getGeneratedUrl(). (In other words, I don't trust the reasoning of the code comment above here: just because it can be called outside a render context, does not mean that cacheability metadata should just always be disregarded.)

The patch moves the code populating the RDF attributes back into rdf_preprocess_comment(), where we can just do toString() for both calls, which will never throw an exception (I assume, because a preprocess hook is always executed in a 'managed' render context?)

This ties in with the question: is the RDF metadata for 'entity_uri' and 'pid_uri' always supposed to be permanent and independent of whatever UrlGenerator logic / cache context? The unpatched code implies "yes" for the 'entity_uri' data. The patched code implies "no; take cache context into account". (I think. And I think that's right - though I'm not very experienced with this UrlGenerator / cache contexts stuff.)

So there are two open questions in my head:

• Is it necessary to re-test whether this patch decreases performance, as the original issue text suggests?
• We are now not assigning $comment->rdf_data on load. Is that OK / can we just address that in a Change Notification, or is it a backward compatibility issue and should this data stay the same until Drupal 9? (If it should stay the same... then to address the cacheability stuff / exceptions mentioned above, we should do ->toString(TRUE)->getGeneratedUrl() in both calls above, but still disregard that data in rdf_preprocess_comment(); perform the extra toString() calls instead.)

--

* Exceptions are thrown because toString(default/FALSE) bubbles up cacheability metadata in the background. EarlyRenderingControllerWrapperSubscriber::wrapControllerExecutionInRenderContext() will detect bubbled-but-unhandled metadata and throw this exception.

And those points in the code are: 'within a regular controller route' but not within a method that explicitly handles bubbled-up metadata. See e.g. definition of "early rendering" in #2450993-118: Rendered Cache Metadata created during the main controller request gets lost or discussion / link to article in #2638686: Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it.

And I'm sorry for being wordy but... it's just a lot of things to take in / think about, for a not-very-experienced core contributor. I still have to write things out, to be surer I don't make mistakes in reasoning.

Patch #7 can't be applied on 9.2.x.Needs reroll

Checking patch core/modules/rdf/rdf.module...
error: while searching for:
  }
}

/**
 * Implements hook_ENTITY_TYPE_storage_load() for comment entities.
 */
function rdf_comment_storage_load($comments) {
  foreach ($comments as $comment) {
    // Pages with many comments can show poor performance. This information
    // isn't needed until rdf_preprocess_comment() is called, but set it here
    // to optimize performance for websites that implement an entity cache.
    $created_mapping = rdf_get_mapping('comment', $comment->bundle())
      ->getPreparedFieldMapping('created');
    /** @var \Drupal\comment\CommentInterface $comment*/
    $comment->rdf_data['date'] = rdf_rdfa_attributes($created_mapping, $comment->get('created')->first()->toArray());
    $entity = $comment->getCommentedEntity();
    // The current function is a storage level hook, so avoid to bubble
    // bubbleable metadata, because it can be outside of a render context.
    $comment->rdf_data['entity_uri'] = $entity->toUrl()->toString(TRUE)->getGeneratedUrl();
    if ($comment->hasParentComment()) {
      $comment->rdf_data['pid_uri'] = $comment->getParentComment()->toUrl()->toString();
    }
  }
}

/**
 * Implements hook_theme().
 */

error: patch failed: core/modules/rdf/rdf.module:224
error: core/modules/rdf/rdf.module: patch does not apply

Rerolled patch #7 .Please check.

#7 patch not applying in drupal-9.3.x-dev
Needs to Re-roll
for ref. sharing screenshot ....

This could still be an issue.

RDF is moving out of core and into https://www.drupal.org/project/rdf moving there.