Fatal errors while loading/building orphaned comments

+++ b/core/modules/rdf/rdf.module
@@ -234,9 +234,14 @@ function rdf_comment_storage_load($comments) {
     $entity = $comment->getCommentedEntity();
-    $comment->rdf_data['entity_uri'] = $entity->url();
+    if ($entity) {

Note: you can save a line here with : if ($entity = $comment->getCommentedEntity()) {
and if ($parent = $comment->getParentComment()) {

Thanks, although i don't think this qualifies as major, the test is pretty contrived, you're adding a storage handler method to deliberately corrupt the comment. Is there a way to trigger this from standard install, from the ui?

I think an exception is the right thing here, otherwise there'd be no way to know that you have orphaned comments.

If we really don't want to throw the exception though, we should at least trigger_error() with E_USER_WARNING.

Discussed with @xjm, @cilefen, @amateescu, @fago, @berdir and @plach. We agreed that this is not major because there's no clear path to orphaned comments. @roderik is this a theoretical issue or did it happen on a real site for you?

I can confirm this is not a theoretical issue. This is an actual problem. I migrated a website to Drupal 8 from Drupal 7 and there are some orphaned comments without parent comments and there are many pages that fail with a fatal error and 500 response code. There are many cases in which this can happen.

I had to modify core modules in order to make website usable. I think this is a major issue for websites that experience similar issues as we had.

This looks like this is test of rolling back a full migration using the UI to fail in #16 at #2687849: Add back rollbacks on migrate_drupal_ui. Since this is blocking a Major, changing to Major and tagging as blocker.

The error is

1) Drupal\Tests\migrate_drupal_ui\Functional\d6\MigrateUpgrade6Test::testMigrateUpgradeExecute
Exception: Error: Call to a member function url() on null
rdf_comment_storage_load()() (Line: 244)

Another reroll here. This is actually a reroll of 19 again as I couldn't get 33 to apply to any commit from 8.8.x, even one that was identical (aside from commit hash) to one that worked for 8.6.x. Not sure what I was doing wrong. This reroll required some manual adjustments as well to make it compatible with the current source. I have tested it and it still seems to fix the issue at least.

Disregard that last patch - I generated it before committing a fix for a typo which causes a fatal error. Here's a working one.

Triaging this as part of the Bug Smash Initiative.

I followed the steps to reproduce that are provided by maisplia in the IS of the related issue, #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted and can confirm that it is possible to create orphaned comments via the UI. I began with a minimal install, 9.1.x, and created three comments on a custom node type, 1 by uid 1 and the other two by an authenticated user. Then deleted the comment field instance. Looking in the db there are 3 entries in comment, comment__comment_body and comment_field-data and 1 row in comment_entity_statistics. After the deletion the page loaded without a visible error for either user. There were no error message at /admin/reports/dblog either.

This issue was to fix fatal errors when loading orphaned comments and my test was not able to generate errors. Therefor, I believe this is fixed in 9.1.x.

Repeated the same test as above with 8.8.x and 8.9.x . Again no errors. So, this has been fixed.

In the related issue #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted masipila did not mention errors when reloading pages with orphan comments. And I am sure he would have - he is very thorough. That was created in Sep 2017 so I suspect that this has been fixed for a while, by what issue I don't know.

Because it is close to bedtime I am not closing this now. I will come back tomorrow afternoon and review.

Ah, another day and I realized I had not tested with nested comments, which is what the IS clearly says. I completely missed it! Yes, deleting a parent comment does cause fatal errors. Sorry for the noise everyone.

And while here I decided to reroll.

thanks for reviving this!

1. +++ b/core/modules/comment/comment.module
   @@ -503,14 +503,14 @@ function comment_preview(CommentInterface $comment, FormStateInterface $form_sta
   +  $build = array();
   

   this should use [] syntax, shows how old this patch is 😂

2. +++ b/core/modules/comment/comment.tokens.inc
   @@ -200,9 +200,13 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
   +            else {
   

   🧐 nit: we could break in the previous if and avoid an else here

3. +++ b/core/modules/comment/comment.tokens.inc
   @@ -219,10 +223,14 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
   +            $replacements[$original] = $title;
   ...
   +            $replacements[$original] = '[not found]';
   

   similarly here

4. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -31,17 +31,19 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +      return ($operation != 'view' || !$commented_entity) ? $access : $access->andIf($commented_entity->access($operation, $account, TRUE));
   

   Should we return access denied if the commented entity is gone?

5. +++ b/core/modules/comment/src/CommentLazyBuilders.php
   @@ -139,7 +139,7 @@ public function renderLinks($comment_entity_id, $view_mode, $langcode, $is_in_pr
   +      $links['comment'] = $commented_entity ? $this->buildLinks($entity, $commented_entity) : array();
   

   same here re [] vs array()

6. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -164,7 +167,9 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +        && $commented_entity->getEntityTypeId() === 'node'
   +        && $commented_entity) {
   

   won't the call to ->getEntityTypeId here fail if $commented_entity is falsey? ie we need to check $commented_entity before we check the entity type, not after

Hi @larowlan
Made changes as you suggested please review.
#45.4 is still pending I am no getting this.

Setting to NR for the testbot.

Been dabbling on this for the past week and the other day I finally realized why I am not making progress. The short answer is that when I run any Functional Comment test that uses \Drupal\Tests\comment\Functional\CommentTestBase::postComment the test fails on this line
$this->assertFieldByName('op', t('Save'), 'Save button found.');.

I am working on HEAD, on a fresh minimal install.

45.3 There was another case where this could be applied, the 'parent' case.
45.4. That makes sense here but not yet changed. But is it something for a followup so an admin can delete orphans?

+++ b/core/modules/comment/src/CommentLazyBuilders.php
@@ -146,7 +146,7 @@ public function renderLinks($comment_entity_id, $view_mode, $langcode, $is_in_pr
-      $links['comment'] = $this->buildLinks($entity, $commented_entity);
+      $links['comment'] = $commented_entity ? $this->buildLinks($entity, $commented_entity) : [];

Instead of checking after each return of buildLinks this patch changes buildLinks to return an empty array if the commented_entity does not exist.

Many changes to the new test and there are more changes here converting array() to [] and other coding standards. So sorry for the noise.

There are changes in comment, getAuthorName and getAuthorEmail that seem totally unrelated to orphans. I could be totally wrong but I want to see what test failures this causes. I would do this locally but there is something about this patch that plays havoc with my setup making this practically impossible to work on.

Similarly with the changes to access logic. The problem is not the access to the comment but that it is an orphan. Let's see what happens if the changes to CommentAccessControlHandler.php are removed.

OK, trimmed this back to the bare minimum and with a fail patch. This removes the changes to files, CommentViewBuilder.php, CommentInterface.php, comment.module and comment.tokens.inc.

Thanks to pameeela another duplicate has been found, #2565247: Fatal error: Call to a member function url() on null. Added as related and adding contributors

The duplicate #2565247: Fatal error: Call to a member function url() on null has a slightly different change to rdf.module and it needs to be checked which change is preferred.

Found what looks like another duplicate of this issue #3044158: Orphaned comments Error: Call to a member function getOwner().

That has a one line change to hasParentComment() and no test. Checking in hasParentComment seems like a better place to check for an orphan, so, let's remove the fix here and test with that fix.

	-    return (bool) $this->get('pid')->target_id;
		+    return (bool) $this->get('pid')->target_id && !empty($this->get('pid')->entity);

Haha, There is no need to change Comment at all. The change in rdf.module will allow the test to pass.

I am reviewing this today, as part of DrupalCon Global.

Looking at the table of patches, I see that the early ones were about 5 KB, then there were a few around 20 KB, and now we are down to 7.64 KB, most of which is tests. Good job!

I have not tested manually, but the fix is simple and looks right. Most or all of my suggestions are related to comments.

1. The patch to the rdf module is small. Here is the whole thing:

    +++ b/core/modules/rdf/rdf.module
    @@ -237,12 +237,13 @@ function rdf_comment_storage_load($comments) {
         $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(TRUE)->getGeneratedUrl();
    +    if ($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 ($parent = $comment->getParentComment()) {
    +      $comment->rdf_data['pid_uri'] = $parent->toUrl()->toString(TRUE)->getGeneratedUrl();
         }

   That comment applies to toString(TRUE) both places that it is used. Putting the comment inside the if block makes it look as though it only applies to the first one.

   The rest of this point is out of scope for this issue, unless it is worth adding a follow-up issue and an @see comment. The combination toString(TRUE)->getGeneratedUrl() asks for the bubbleable metadata and then throws it away. It was added, along with the comment, in #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it. It was discussed in Comment 39 of that issue and following comments, so it did not slip in by accident. It is also mentioned in the Proposed resolution section of the issue summary.

   I find this very confusing. If we do not ask for the bubbleable metadata and then throw it away, how does it "leak"? Does it somehow get "used up" when we ask for it? Is it really getCommentedEntity() and getParentComment() that are creating this unwanted cache metadata?

   According to Comment 40 on that issue, #2626298: REST module must cache only responses to GET requests independently fixes this metadata problem. Maybe we no longer need this ugly work-around (IMO).

2. This is a really nice explanation. I commend you for not saying "because **** happens". I just want to polish it a bit.

    +++ b/core/modules/comment/tests/src/Functional/CommentOrphanedTest.php
    @@ -0,0 +1,98 @@
    +<?php
    +
    +namespace Drupal\Tests\comment\Functional;
    +
    +/**
    + * Tests operations on orphaned comments.
    + *
    + * Drupal core will officially never orphan comments. An orphan comment is
    + * comment with a nonexistent parent comment or commented entities.
    + *
    + * This test exists despite the above because it's expected that in time someone
    + * will try to be smart and delete old entities/comments from a large database,
    + * a PHP process will die at exactly the wrong time, or whatever exotic event
    + * happens that makes comments with invalid parent/entity references exist. When
    + * this happens, a site should not start crashing during basic operations like
    + * loading/displaying a node page. Practice has shown that it's fairly easy to
    + * inadvertently write code that makes Drupal crash during loading/rendering
    + * orphaned comments; this test will draw attention to that code in Drupal core.

   Why CommentOrphanedTest instead of OrphanedCommentTest?

   Consider shortening "will officially never" to "should never". Maybe switch the order of the two sentences.

   Consider "Experience" instead of "Practice". Start a new sentence instead of using a semicolon. I think it should be "such code" instead of "that code".

3.   +  /**
     +   * Modules to install.
     +   *
     +   * Install all modules that implement hook_comment_storage_load().
     +   *
     +   * @var array
     +   */
     +  public static $modules = ['comment', 'comment_test', 'node', 'rdf'];

   Why install comment_test? (Answer: because it defines CommentTestSqlEntityStorage.)

   The comment makes it look as if all four of those modules implement hook_comment_storage_load(). Maybe something like "Install node, comment, comment_test, and all modules that implement hook_comment_storage_load()".

4. My personal preference is to use the imperative (as here, "Test"), but isn’t the Drupal standard to use the present tense ("Tests")?

    +  /**
    +   * Test loading/deleting/rendering orphaned comments.
    +   */
    +  public function testDeleteOrphanedComment() {

5. If you prefer to use service() all the time, that is fine, but we could use Drupal::entityTypeManager() instead:

    +    $manager = \Drupal::service('entity_type.manager');

6. Thanks for this comment!

    +    // We only know how to orphan comments on SQL based entity storage.
    +    $storage = $manager->getStorage('comment');
    +    if (in_array('Drupal\Core\Entity\Sql\SqlEntityStorageInterface', class_implements($storage))) {

   I would rather exit early if $storage does not implement the interface.

   Is this any different from testing $storage instanceof Drupal\Core\Entity\Sql\SqlEntityStorageInterface? If we had a use statement, then we could simplify this to $storage instanceof SqlEntityStorageInterface.

7.   +      $this->drupalGet('node/2');
     +      $this->drupalGet('node/' . $this->node->id());

   Do we have to navigate away from $this->node before reloading it? Are we sure that $this->node->id() is not 2?

   Out of scope: do the Comment tests do this enough that it is worth adding a drupalGetNode() helper method? (I count 21 times, including this patch.)

8. This line needs a comment: we are now invalidating the entity field, having previously invalidated the user and parent.

    +      $ids = $storage->orphanComments();

   But orphanComments() does not return anything, so now $ids is NULL.

    +      // Load, render (standalone without commented entity) and delete comments,
    +      // implicitly testing that nothing breaks when the commented entity is not
    +      // available. This does not have a current practical application (there
    +      // are no standalone 'comment pages') but it ensures some decoupling of
    +      // the comment view/render code, which is a good thing in itself.
    +      $entities = $storage->loadMultiple($ids);
    +      $storage->delete($entities);

   Since $ids is NULL, this loads all comments, right? Is that the intention?

   Either use something more specific than $entities, like $comments, or else eliminate the intermediate variable.

9.   +++ b/core/modules/comment/tests/modules/comment_test/src/CommentTestSqlEntityStorage.php
     @@ -0,0 +1,61 @@
     +   * @param array $comment_ids
     +   *   IDs of comments to orphan. If not provided, one comment will be taken
     +   *   semi randomly.
     ...
     +   */
     +  public function orphanComments(array $comment_ids = [], array $fields = []) {

   It is not semi random: it invalidates the most recent comment (max cid), and we rely on that.

   The first time we call this, we provide [] as the first argument. I would say "If empty" instead of "If not provided".

P.S.

The class comment in #75.2: "An orphan comment is comment with ...". Add "a" before or after the line break.

@benjifisher, thanks for the review.

Now that the unnecessary changes have been removed it is easy to see that this patch fixes rdf_comment_storage_load so that is handles NULL returns from getCommentedEntity and getParentComment. The interface documents that those methods can return NULL so rdf should be doing that. Therefor I made a decision to move this patch to #2978320: [backport] rdf comment storage load should not load NULL comments and continue work there. And I started that but didn't get back here to leave a comment until now.

However, there is still a need to make sure that loading an orphaned comment doesn't cause a fatal. Exploring that can continue here. For example, are there other cases where getCommentedEntity and getParentComment do not properly handle NULL?

And now what about the review in #75 and #76? To make reviews easier I made the recommended changes for the points in #75 and #76. If the tests pass this patch needs to move to #2978320: [backport] rdf comment storage load should not load NULL comments. Then, I think this needs to be postponed, until that issue is committed.

After removing the comment field and navigating to the comment, we saw website encountered error
PHP error is seen in dblog as well

@samiullah:

Can you describe more explicitly what you did and what the results were? I can guess what you mean by "the comment field" that you removed, but I would rather not have to guess. What error messages did you get? (If you paste the error message into a comment, then others who run into the same problem will be able to find this issue with their favorite search engine.)

I think what you are describing is a different bug, but I am not sure. If it is different, then we should have a separate issue for it.

@quietone:

If you want to move the patch and the discussion to the other issue, that is fine with me. I like the way you addressed #75.6. Several other points from that review look as if they have not been addressed (3, 4, 8, 9).

Closed #3044158: Orphaned comments Error: Call to a member function getOwner() as a duplicate, adding credit for HiMyNameIsSeb, dionsj, and mgp_novicell. Since this is still a work in progress here is the fix suggested from that issue.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -344,7 +344,7 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
-    return (bool) $this->get('pid')->target_id;
+    return (bool) $this->get('pid')->target_id && !empty($this->get('pid')->entity);

I haven't really been happy with this and today and spent time reworking the test. The new and improved version creates two comments and the second one is orphaned. It uses a data provider that changes one of the properties (entity_id, cid, or pid) of comment 2 so that it is orphaned. Then, when running the test caused failures each failure was fixed in turn. So, now I am sure there are no out of scope changes.

Still to do in #79, the final paragraph.

Fixing tests and fix coding standards.

#75

3. comment_test does not need to be installed, this just uses the test storage class. So, maybe that file needs to move somewhere else.
4. Comment changes and uses 'Tests'.
8. That bit of code reworked. It now loads the two comments created in the test individually.

    // Test rendering the two comments.
		    $ids = [1, 2];
		    $entities = $comment_storage->loadMultiple($ids);
		    foreach ($entities as $entity) {
		      $built = $this->buildEntityView($entity, 'full', NULL, TRUE);
		      $renderer->renderPlain($built);
		    }

9. Forgot to do this one.

Setting NW for 9 and maybe 3.

This address #75.9. The comment storage test class is now in the new test, CommentOrphanedTest, because that is the only place it is used. The comments in that class are updated.

Uploaded the wrong patch - ignore #88.

This address #75.9. The comment storage test class is now in the new test, CommentOrphanedTest, because that is the only place it is used. The comments in that class are updated.

Note that this patch includes the fix from #3163924: buildEntityView calls resetCache with an array of ids not entities.

Actually, the comment storage test class is also used in #2978320: [backport] rdf comment storage load should not load NULL comments, so moved it outside the CommentOrphanedTest.php

Actually, I just rewrote the test in #2978320: [backport] rdf comment storage load should not load NULL comments and it no longer uses comment storage.

Still, this is ready for review.

1. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -80,9 +80,11 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode) {
   +      !$commented_entity
   

   Nit: I would fine is_null($commented_entity) a little easier to follow.

2. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -140,10 +142,12 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +      $entity_type_id = $commented_entity ? $commented_entity->getEntityTypeId() === 'node' : NULL;
   
   @@ -164,7 +168,7 @@ public function buildComponents(array &$build, array $entities, array $displays,
   -      if ($attach_history && $commented_entity->getEntityTypeId() === 'node') {
   +      if ($attach_history && $entity_type_id) {
   

   This variable is very confusing. It sounds like it's an entity type id, but actually it's either boolean or NULL. Could it be simple a boolean called $is_a_node?

3. +++ b/core/tests/Drupal/Tests/EntityViewTrait.php
   @@ -61,7 +61,7 @@ protected function buildEntityView(EntityInterface $entity, $view_mode = 'full',
   -      $render_controller->resetCache([$entity->id()]);
   +      $render_controller->resetCache([$entity]);
   

   Presumably this isn't mean to be in the patch.

@jonathanshaw, thanks.

1. Fixed.
2. Fixed, the variable is now $is_node.
3. Ah, yes that is the fix from #3163924: buildEntityView calls resetCache with an array of ids not entities, let's try without that. That requires changing the call to buildEntityView. Made a fail patch to be sure this change works. The file 'interdiff.txt' is between the fail patch and the patch.

You've done a fantastic job of sticking with this unglamorous issue for a long time, quietone. Sorry to raise doubt when you're so close to the end ...

1. +++ b/core/modules/comment/tests/src/Functional/CommentOrphanedTest.php
   @@ -0,0 +1,95 @@
   +namespace Drupal\Tests\comment\Functional;
   ...
   +    $this->drupalLogin($this->webUser);
   +    $comment = $this->postComment($this->node, 'Comment 1 body', 'Comment 1 subject');
   ...
   +    $this->drupalGet('comment/reply/node/' . $this->node->id() . '/comment/' . $comment->id());
   +    $this->postComment(NULL, 'Comment 2 body', 'Comment 2 subject', TRUE);
   

   I don't see that this needs to be a functional test. The UI seems to be only being used to set up the comments, and that can be better done via the API. The actual assertions are all via the API. Kernel tests are so much faster, and robust to UI changes.

2. +++ b/core/modules/comment/tests/src/Functional/CommentOrphanedTest.php
   @@ -0,0 +1,95 @@
   +    $ids = [1, 2];
   ...
   +    $ids = [1, 2];
   +    $entities = $comment_storage->loadMultiple($ids);
   

   Unimportant nit: Do we need to commit to the ids? Could we just use loadMultiple() to get all comments?

3. +++ b/core/modules/comment/tests/src/Functional/CommentOrphanedTest.php
   @@ -0,0 +1,95 @@
   +    $comment_storage_test->delete($entities);
   +    $entity_manager->clearCachedDefinitions();
   +    $entity_manager->getDefinition('comment')->setStorageClass(get_class($comment_storage));
   

   I'm not sure you need this; isolation between scenarios of a data provider is pretty good.

4. +++ b/core/modules/comment/tests/src/Functional/CommentTestSqlEntityStorage.php
   @@ -0,0 +1,60 @@
   +class CommentTestSqlEntityStorage extends CommentStorage {
   

   If you make the test a kernel test, you may not even need the complexity of this storage handler. It's usually possible to make invalid data with dysfunctional references via the entity API, validation is opt-in.

5. I think an exception is the right thing here, otherwise there'd be no way to know that you have orphaned comments.
   If we really don't want to throw the exception though, we should at least trigger_error() with E_USER_WARNING.

   I'm not sure we've really dealt with this comment of @catch in #18. Would it make sense to log a warning if a comment has a bad reference?

6. I suspect the IS is pretty outdated and there have been some twists and turns, an update would help the committer.

@jonathanshaw, thanks again.

95.
1. The reason this is a functional test is because a) I was working from what was here, b) there has been no suggestion in this issue that the test should be a kernel test and c) I don't know how to use the Entity API to make orphan comments. If that is the desired direction here then either a) I need to find support for learning how to do that or b) someone else will have to finish this.
2-5. TODO when a decision is made on the type of test.

6. The IS was updated in #41 on 23 June 2020. Can you be more specific about what needs to be updated?

Edit: improvements

Updated IS based on my understanding of your work, hope I got it right.

I don't know how to use the Entity API to make orphan comments.

You're right, it's not that simple. Comment:preSave() wille error if you try to save a comment with a reference to an invalid parent via the entity API. You do need to tickle the database directly like the orphanComments method does.

But I don't see why the orphanComments method needs to be on a storage handler rather than just part of the test case; I can't see that getting committed as is.

Maybe this would this work in a kernel test:

public function testOrphan($property) {
  $nodeStorage = $this->entityTypeManager->getStorage('comment');
  $node = $nodeStorage->create(['title'=>'test'])->save();

  $commentStorage = $this->entityTypeManager->getStorage('comment');
  $comment1 = $commentStorage->create([
    'subject' => 'test',
    'comment_body' => 'test',
    'uid' => '1',
    'entity_id' => $node->id(),
    'entity_type' => 'node',
    'comment_type' => 'default'
  ])->save();
  $comment2 = $commentStorage->create([
    'subject' => 'test',
    'comment_body' => 'test',
    'uid' => '1',
    'entity_id' => $node->id(),
    'entity_type' => 'node',
    'comment_type' => 'default'
    'pid' => $commen1->id(),
  ])->save();

  $this->orphanComments($property);
  $commentStorage->resetCache();
  $nodeStorage->resetCache();

  $comments = $commentStorage->loadMultiple();
  foreach ($comments as $comment) {
     $built = $this->buildEntityView($comment, 'full', NULL);
     $renderer->renderPlain($built);
  }

   $node = nodeStorage->load($node->id());
   $built = $this->buildEntityView($node, 'full', NULL);
   $renderer->renderPlain($built);
}

protected function orphanComments($property) {
   ...
}

@jonathanshaw, thanks you. After I thought about it some more I was thinking of doing the same thing you suggest.

This patch removes the functional test and the test storage class and replaces it with a Kernel Test. Because this is a new test a fail patch is added.

Namespace error.

Nice.

1. +++ b/core/modules/comment/tests/src/Kernel/CommentOrphanTest.php
   @@ -0,0 +1,178 @@
   +  public function orphanComments(array $comment_ids = [], array $fields = []) {
   

   The signature and logic of this method are now significantly more complex than they need to be. We either need to make a strong case for why this is desirable, or simplify it.

2. There is also @catch's comment in #18:
   

   I think an exception is the right thing here, otherwise there'd be no way to know that you have orphaned comments.
   If we really don't want to throw the exception though, we should at least trigger_error() with E_USER_WARNING.

   I suggest that in CommentViewBuilder we check for invalid references and simply log a warning if one is found. I don't think this needs test coverage, that seems like overkill.

Otherwise RTBC.

1. Simplified it so much that the helper method no longer exists.
2. Ah, where to add this, exactly? What is the message?

Simplified it so much that the helper method no longer exists.

That interdiff made me chuckle.

Regarding #18, I've become uncomfortable with the precedent. If we systematically tested for valid references every time we worked with an entity reference and logged invalids, we'd add lines of code to thousands of places all over Drupal. If people wanted to be warned about invalid entities on their site, it would be easy to make a module for that.

OK #106 is a good point. It's not really different from other entity references where we're quite permissive.

Committed 9beb71e and pushed to 9.1.x. Cherry-picked to 9.0.x and 8.9.x, Thanks!

This broke HEAD on 8.9.x:
https://www.drupal.org/pift-ci-job/1803301

The $modules test property was made protected in D9, but should be public for D8.

#112. Here is an 8.9.x patch.

Sorry for noise, I requeued for 8.9 instead.

Whoops thanks everyone. 8.9.x is green and a one word change, so committed/pushed to there now.

Nope... https://www.drupal.org/pift-ci-job/1803553

Remove return type declaration.

The void return type thing is unfortunate, never easy to remember when doing backports unless you explicitly test on PHP 7.0.

Committed 78cbebb and pushed to 8.9.x. Thanks!