Impossible to reply to comments: commented entity considered unreferencable because CommentSelection::entityQueryAlter() joins on {node_field_data} table

Thanks for the bug report. If you happen to work on this, please assign this to yourself using the select box.

That sounds … pretty esoteric/extreme. Each of us setting up that same thing to reproduce this is a poor investment of our time. Please provide a failing test. If not a failing test, then at least YAML files for the relevant configuration.

If not tests, then please at least provide steps to reproduce!

Working on test coverage.

I am currently not succeeding in even creating a comment on a comment, let alone a comment on a comment on a comment. i.e. I'm getting stuck at 2 levels instead of 3.

… and when I step through it with a debugger, even the validation of comment_type fails, despite it being set to something valid. It fails because comment_types are of course config entities, yet \Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraintValidator::validate()
ends up calling \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::validateReferenceableEntities(), which only supports content entities.

Attached is my WIP test coverage patch.

I'll continue on Monday. Unassigning myself because if somebody wants to continue in the next 4 days, feel free!

I tried to compare how this works in "the UI" (i.e. using Drupal's UI rather than JSON API).

Turns out that there too there is violation error! The following violations is triggered for the entity_id field:

This entity (comment: 2) cannot be referenced.

… yet that validation error never shows up! Why? Because \Drupal\Core\Entity\ContentEntityForm::validateForm() does

    $violations = $entity->validate();
…
      $edited_fields = $this->getEditedFieldNames($form_state);
…
    // Remove violations for fields that are not edited.
    $violations->filterByFields(array_diff(array_keys($entity->getFieldDefinitions()), $edited_fields));

… and because entity_id was not edited via the form, $edited_fields includes entity_id, for which the violation was registered.

Hence the problem lies either in comment.module or the Entity Field API. And this problem applies to the REST module just as well. Moving to that component for now, to make it clear this is NOT a bug in JSON API.

There are two potential solutions here:

1. 👎 Somehow mimic the "filter away non-edited fields" behavior that the form-based logic uses (relevant code cited in #11). But how to do that in an HTTP API context? Every field is sent, therefore every field is edited. The form-based logic uses the corresponding EntityFormDisplay config entity to determine which fields's widgets are shown to the end user, and hence are editable. No such thing is possible in an HTTP API context.
   Verdict: not feasible.
2. 👍 Rather than trying to get the same symptom (no violation errors shown for Comment's entity_id field despite them being there), fix the root cause: ensure that that violation error is not created!

I figured I'd make this easier to reproduce/debug: I want this validation error to show up even when using forms. But … that's apparently easier said than done, because just pretending entity_id is an edited field is not enough! \Drupal\Core\Entity\ContentEntityForm::flagViolations() uses $form_state->setErrorByName(str_replace('.', '][', $violation->getPropertyPath()), $violation->getMessage()); to make errors show up, and guess what … because there's no form item for the given property path, that does not actually show up at all! In other words: \Drupal\Core\Entity\ContentEntityForm::flagViolations() will happily swallow validation errors.

That's what the attached patch does.

So, going with #12.2, where does that lead?

The originally reported problem is about the 3rd level of node comments (comment on comment on comment on node). But I can also reproduce it with the 2nd level of comments on any other entity type (for example comment on comment on taxonomy term). What's the common denominator between those? The fact that the Comment entity you're posting is of a CommentType which has target_entity_type_id: comment in its configuration.

If you start debugging that, you'll end up stepping through \Drupal\comment\Plugin\EntityReferenceSelection\CommentSelection::entityQueryAlter(), which does this:

    // The Comment module doesn't implement any proper comment access,
    // and as a consequence doesn't make sure that comments cannot be viewed
    // when the user doesn't have access to the node.
    $node_alias = $query->innerJoin('node_field_data', 'n', '%alias.nid = ' . $data_table . '.entity_id AND ' . $data_table . ".entity_type = 'node'");
    // Pass the query to the node access control.
    $this->reAlterQuery($query, 'node_access', $node_alias);

This is the root cause of all these problems. It results in this query:

SELECT base_table.cid AS cid, base_table.cid AS base_table_cid
FROM 
comment base_table
INNER JOIN comment_field_data comment_field_data ON comment_field_data.cid = base_table.cid
INNER JOIN node_field_data n ON n.nid = comment_field_data.entity_id AND comment_field_data.entity_type = 'node'
WHERE comment_field_data.cid IN (2)

which returns nothing, because the comment type we're trying to reference is a comment on a comment, not a comment on a node. The cited code is what adds INNER JOIN node_field_data n ON n.nid = comment_field_data.entity_id AND comment_field_data.entity_type = 'node', which is what causes only comments on nodes to be eligible.

Maybe related to this issues:

We also have a comment on comment.
Editing the parent comment goes fine (through comment/**id>$**/edit). Then I added a comment on this comment through code. Editing or viewing this last one fails.

In function form() on rule 76 in web/core/modules/comment/src/CommentForm.php the $field_name is wrong. $comment->getFieldName() does not return the name of the field through which the comment is referenced to its parent, but it returns instead the bundle name of the comment.

It's because of

\Drupal\comment\Plugin\EntityReferenceSelection\CommentSelection::entityQueryAlter

The code is:

 /**
   * {@inheritdoc}
   */
  public function entityQueryAlter(SelectInterface $query) {
    parent::entityQueryAlter($query);

    $tables = $query->getTables();
    $data_table = 'comment_field_data';
    if (!isset($tables['comment_field_data']['alias'])) {
      // If no conditions join against the comment data table, it should be
      // joined manually to allow node access processing.
      $query->innerJoin($data_table, NULL, "base_table.cid = $data_table.cid AND $data_table.default_langcode = 1");
    }

    // The Comment module doesn't implement any proper comment access,
    // and as a consequence doesn't make sure that comments cannot be viewed
    // when the user doesn't have access to the node.
    $node_alias = $query->innerJoin('node_field_data', 'n', '%alias.nid = ' . $data_table . '.entity_id AND ' . $data_table . ".entity_type = 'node'");
    // Pass the query to the node access control.
    $this->reAlterQuery($query, 'node_access', $node_alias);

    // Passing the query to node_query_node_access_alter() is sadly
    // insufficient for nodes.
    // @see \Drupal\node\Plugin\EntityReferenceSelection\NodeSelection::buildEntityQuery()
    if (!$this->currentUser->hasPermission('bypass node access') && !count($this->moduleHandler->getImplementations('node_grants'))) {
      $query->condition($node_alias . '.status', 1);
    }
  }

So we have hardcoded condition $data_table . ".entity_type = 'node' which means stuff will work only for nodes :(

Hi,

I was having problem with this. I created a custom comment type, and added a field in a custom type. Using JSONAPI gets the following error:

"errors": [
        {
            "title": "Unprocessable Entity",
            "status": "422",
            "detail": "pid.0.target_id: This entity (comment: 1) cannot be referenced.",
            "source": {
                "file": "/var/www/d8/web/core/modules/jsonapi/src/Entity/EntityValidationTrait.php",
                "line": 59,
                "pointer": "/data/attributes/pid/target_id"
            },

But thanks to nnevill I have been able to create a quick patch. This is just a temporary solution for those who has the same problem.

Related:

• https://www.drupal.org/project/jsonapi_comment/issues/3168698
• https://www.drupal.org/project/fluid_comment/issues/3094759#comment-1380...

Sorry I forgot to make the patch with the correct format.

One more issue here is that \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::reAlterQuery() called from \Drupal\comment\Plugin\EntityReferenceSelection\CommentSelection::entityQueryAlter() trying to access protected properties on query and fails

Adding a related (possible duplicate) issue that has tests already

Marked #3272023: Do not check node_access if node module is not installed as a duplicate however it has tests.

Marking as needs-reroll

Could the person who takes on the re-roll add the tests from #3272023: Do not check node_access if node module is not installed to the patch from above

Crediting those who worked on the other issue.

Rerolled patch against 9.4.x as mentioned in #33

+++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
@@ -76,19 +76,20 @@ public function entityQueryAlter(SelectInterface $query) {
+    if ($this->moduleHandler->moduleExists('node')) {
+      // The Comment module doesn't implement any proper comment access,
+      // and as a consequence doesn't make sure that comments cannot be viewed
+      // when the user doesn't have access to the node.
+      $node_alias = $query->innerJoin('node_field_data', 'n', "[%alias].[nid] = [$data_table].[entity_id] AND [$data_table].[entity_type] = 'node'");

As I said on the other issue, this approach is assuming that if node module exists, then the comment is on a node.

But those two facts are independent.

We can have:

- node module, comment on node
- node module, but comment on something else
- no node module, comment on something else

We need to check the actual host entity of the comment.

I think he meant re-rolling the patch from #24 with the tests from the mentioned duplicated issue.
#34 is just the patch that was provided in #2372023 as is.

I think he meant re-rolling the patch from #24 with the tests from the mentioned duplicated issue.
#34 is just the patch that was provided in #2372023 as is.

Yep, that's what I meant, thanks

Re-roll of #24 with tests.

Re-roll of #24 with tests and coding standards fix.

Forgot to copy the use statement in the patch. Re-trying.

Ok, I initially thought that #24 was working, but it's not.

A few things:

+.   // Get the entity_type from cid which is the first argument for nodes
+    // and second argument for different entity types
+    $arguments = array_values($query->getArguments());
+    $cid = count($arguments) > 1 ? $arguments[1] : $arguments[0];

Upon debugging some of the methods that fail, it seems that that's wrong. Sometimes the method gets even called with no arguments.

+    $join_table = $entity . '_field_data';

The code after that assumes that the table exists, but it might not, depends on the entity implementation.

Due to the above, I changed the logic in a couple of places.

First we fetch the conditions and we try to get the one that checks the "cid" value of the comment, and we get the value of the argument for it. This should work in most of the cases. Based on getting a value for "cid", we try to determine the "entity_type" that the comment is attached to via a query (like the original patch).

Then, we actually check that the produced table name does exists in the database, as "ENTITY_field_data" does not need to exists for all entity types. If that check goes through, we do the JOIN, and only if the entity type is "node", we do the "node_access" and "bypass node access" checks).

Finally, I needed to alter one of the tests in the "system" module that one could argue is opinionated. "Should a comment admin be able to view comments made on an unpublished node?". The way I see it is that the user should actually be able to view and review the comment (ie: profanity), but not the node, like a taxonomy admin should be able to view and review terms added to a node, regardless of the access to the node, for example. If we want this test unchanged, then we need to provide access to the entity_type somehow to the "entityQueryAlter" method, which I believe is a much bigger task.

Providing patch with all the above logic in place.

Same as above, fixing coding standards.

+++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
@@ -68,26 +69,55 @@ public function validateReferenceableNewEntities(array $entities) {
+    // Try to get the entity_type from the cid given in the arguments, which is
+    // usually the first argument for nodes and second argument for different
+    // entity types. Some core tests bypass this and do not pass this option,
+    // so in these cases we don't perform the additional checks and leave it up
+    // to the test.

That looks really brittle.

I came up with an idea for how to tackle this on the other issue -- see https://www.drupal.org/project/drupal/issues/3272023#comment-14484880

@joachim - if you could give me some more guidance on where/how to make that happen I could try, though I'm not sure I have enough "core" knowledge to do the rest.

I'm a bit lost on:

Going up the backtrace, I can see in ValidReferenceConstraintValidator::validate(), there is the comment entity with 'node' set as its entity_type field value.

However, that value doesn't get passed into the selection handler.

How can I debug that?

Also:

That would require us to add a new setting to the 'comments' field type, hidden from the UI and set automatically.

Not sure what this means.

Or maybe @larowlan wants to chip in on what could be next or how to approach it.

So, my basic point is that in CommentSelection.php we shouldn't be having to sniff around to figure out the host entity type. Hacking about with table names in the query is really messy. Comments *know* which host entity type they are on.

However, we don't have the host entity type in CommentSelection.php.

So, I investigated to find out how far back we have to go until we get to a point where the host entity type is known.

I used the steps to reproduce from the other issue (which could really do to be copied over here!!!!), with xdebug to see the backtrace.

So, what I found was:

> in ValidReferenceConstraintValidator::validate(), there is the comment entity with 'node' set as its entity_type field value.

However, I'm not sure either what I meant by this:

> That would require us to add a new setting to the 'comments' field type, hidden from the UI and set automatically.

because now I look at the code, I am thinking, surely field storage and field definitions know which entity type they are attached to?

There is this:

      'target_type' => $field_definition->getFieldStorageDefinition()->getSetting('target_type'),

but target_type can be ambiguous -- IIRC for fields it can mean the host, and for reference fields it's the type of entity that gets referenced.

Here is a slightly different approach. Rather than having all that "node" or "entity" access-related logic within the query, we can just call the "access" method on the entities that are returned and then filter out the ones which the user can't access. That is actually the way that "CommentController" uses to check access to the comment.

I've tackled that by overriding the "getReferenceableEntities" and "countReferenceableEntities" methods to perform that additional check. I find it pretty clean compared to all the other previous assumptions.

Let me know your thoughts.

It was a false negative as the failing test didn't make sense at all in the context. I triggered a retest and we can see it all came green. Marking as "need review".

> Rather than having all that "node" or "entity" access-related logic within the query, we can just call the "access" method on the entities that are returned and then filter out the ones which the user can't access

That's not going to scale, unfortunately. The reason we have the node_access table system is for scalability.

We’re running into this in upgrading API.Drupal.org.

It's a pity that the previous version wouldn't scale, as that was probably the cleanest and a true split between comment and node (within that file at least), but I understand.

I kept digging into what was available within the "CommentSelection" class.
* When it's a nested comment, the entity is there (available since this issue https://www.drupal.org/project/drupal/issues/2879918) so we can access the host "entity_type" from the entity via "$entity->getCommentedEntityTypeId()".
* And if it's not available, we can then get the "target_type" setting from the "entity_id" field FieldStorageDefinition.

With that, we have the table name, which should still be checked to see if it exists, and finally, if the "target_entity_type" is "node", then we do the query altering to deal with scalability issues.

Patch supplied for your review here.

Looks like you've managed to get hold of the host entity type, that's great!

1. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -76,18 +77,37 @@ public function entityQueryAlter(SelectInterface $query) {
   +      // Entity not available yet, get it from the field definition.
   +      $target_type = $this->getConfiguration()['target_type'];
   +      $fields = $this->entityFieldManager->getFieldStorageDefinitions($target_type);
   +      $target_entity_type_id = $fields['entity_id']->getSetting('target_type');
   

   I would simplify this and always use this logic, and remove the if() path. It'll always work.

2. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -76,18 +77,37 @@ public function entityQueryAlter(SelectInterface $query) {
   +    $join_table = $target_entity_type_id . '_field_data';
   

   Get the table from the entity type's method for that, rather than hardcoding the name.

   And it's a table we're going to join to, but it would be clearer to call it $host_entity_field_data_table.

3. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -76,18 +77,37 @@ public function entityQueryAlter(SelectInterface $query) {
   +    $primary = $this->entityTypeManager->getDefinition($target_entity_type_id)->getKey('id');
   

   'primary' not usually a term used in Drupal for entities. Call this $id_key maybe?

4. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -76,18 +77,37 @@ public function entityQueryAlter(SelectInterface $query) {
   +      $entity_alias = $query->innerJoin($join_table, 'n', "[%alias].[$primary] = [$data_table].[entity_id] AND [$data_table].[entity_type] = '$target_entity_type_id'");
   

   I'm not clear on what this join is going to do for non-node hosts.

   There doesn't seem to be any further condition?

   Should we be adding a condition on status if the host entity type has a 'published' key?

1. I am not sure why, but I think that we need to keep the "if/else" when we have an entity.
* In the newly added tests, the "$entity" object was correct and would give us the right target entity type value, but if we only use the "else" logic, we would weirdly get "user" as the value for the target entity type, even if the 'target_type' is "comment" and the test does NOT attach comments to users whatsoever.
* I tried as well in the project where I am having the issue and I get the same: "$entity" is populated with the right thing and the "else" path returns "user" (again, user does not have comments attached on the project).

So I've kept the if/else since that since to be working in all scenarios. For all top level comments, "$fields['entity_id']->getSetting('target_type')" is correct, and for all nested comments, where $entity is available, "$entity->getCommentedEntityTypeId()" is correct too.

2. Renaming done and getting the table name from the entity type method.

3. Renaming done.

4. I changed the direct "node_access" tag to a dynamic "$target_entity_type_id . '_access'", which in turn, when it's a "node", will be "node_access", but it will allow other entity types to alter the query as needed (to check for the published status for example, if needed). I didn't add the "published" check, because of what I just mentioned.

New patch attached.

> if we only use the "else" logic, we would weirdly get "user" as the value for the target entity type, even if the 'target_type' is "comment" and the test does NOT attach comments to users whatsoever.

That doesn't sound right to me at all. We always have a field. Something's not right there, I think.

Yup, if defo looks weird. But it's also something that is not introduced by this patch, as we are mostly reading data, not setting any values.

If you want to replicate, you can do it running/debugging CommentNonNodeTest::testCommentFunctionality, in the methods where we test threading.

As mentioned, "comment" is not attached in any way to "user", yet the following happens when creating a comment reply in a non-node entity:

$entity_type_id = $this->getConfiguration()['target_type'];   // returns "comment"
$fields = $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id);   // returns array of field definitions
$target_entity_type_id = $fields['entity_id']->getSetting('target_type');     // returns "user"

Note that in the mentioned case, the "$entity" is set to the comment that you're replying to.

> I would simplify this and always use this logic, and remove the if() path. It'll always work.
You were right all along. It was the way I was debugging things.

I was stopping the execution and dumping things, that's why I was getting that value, which was coming from the "user" field validation within the Comment form. There is no "comment" attached to a "user", but an "user" is actually making a "comment", so it makes sense. Once I let everything run, things were good.

Thanks to this debugging, I think I finally understand what this plugin is doing. It helps validate all the EntityReference fields attached to a comment and triggers the "ENTITY_access" tag (and additional logic for node for scalability).

This version of the patch addressed all your feedback from #57
Kindly review again please.

All the reported tests do pass locally and even in the reported output of the job. Re-queing tests.

Fixing the deprecation. See notes from #62 for review.

This is looking great.
Can we get a test-only patch so we see red/green

Thanks

Didn't mean to change status

Yup, not a problem. Here you go.

Above fail expected. Changing back to "Needs review".

Looks great!

Re-run the tests to be sure.

One question here

+++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
@@ -76,18 +76,33 @@ public function entityQueryAlter(SelectInterface $query) {
+    $entity_type_id = $this->getConfiguration()['target_type'];
+    $fields = $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id);

Any reason we can't assume the entity_type_id is comment here?

Can you even use this selection handler for another entity type?

> $entity_type_id = $this->getConfiguration()['target_type'];

We're supposed to be getting the host entity type here.

(This is why inline comments are useful!)

Looking at the parent class, target_type gets us the entity type *being referenced*, so yes, 'comment' in this case:

    $configuration = $this->getConfiguration();
    $target_type = $configuration['target_type'];
    $entity_type = $this->entityTypeManager->getDefinition($target_type);

    $query = $this->entityTypeManager->getStorage($target_type)->getQuery();

So the approach needs work.

Something seems very wrong here: I added a comment field to taxo terms.

This:

    $entity_type_id = $this->getConfiguration()['target_type'];
    $fields = $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id);
    $target_entity_type_id = $fields['entity_id']->getSetting('target_type');
    dsm($target_entity_type_id);

says 'node'. There should be no nodes around at all here!

Ah right, it's because the 'entity_id' field on comments probably has 'node' as its target type in its base field definition. That is then overridden per-bundle in a hook somewhere.

So, expected, but not useful here.

If we truly want the host entity type, we need the if/else that was present in #58.

If $this->getConfiguration()['entity'] is available that'll give us the target entity type. If it's not, then the above lines get it right.

    $entity = $this->getConfiguration()['entity'];
    if ($entity) {
      $target_entity_type_id = $entity->getCommentedEntityTypeId();
    }
    else {
     // Entity not available yet, get it from the field definition.
      $entity_type_id = $this->getConfiguration()['target_type'];
      $fields = $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id);
      $target_entity_type_id = $fields['entity_id']->getSetting('target_type');
    }

I thought someone further up had said we couldn't get values from the comment entity that's available here, but it turns out we can.

(Posted simultaneously with #81.)

@fjgarlin when does this situation happen?

> // Entity not available yet, get it from the field definition.

I've been testing manually with posting a new comment in a thread and a new top-level comment.

It didn't happen while testing manually (top level comment, threaded, etc), that situation only showed up in one of the tests. It was "EntityReferenceSelectionAccessTest::testCommentHandler".

Perhaps we should just stop checking anything else if we don't have the "$this->getConfiguration()['entity']".

So, if we do have it, do the checks for access, etc, but if we don't have it yet, there's nothing to check at this stage. So a kind of wrapping "if ($comment) {" after you first try to get it.

That still makes the mentioned test fail. It might be the way that the test is calling things, bypassing the way that Drupal would normally do it.

Here is a version of the patch that brings back the checking of the entity (#58), which should be available during all cases when using the comments functionality as intended, but also has the "else" part as a fallback (which also covers the way that the failing test does the checks).

It's very similar to #58, with the suggestion from @larowlan in #77 (we know it's "comment") and the namings by @joachim in #82.

I'm open to more feedback if needed.

Forgot to adapt the deprecations. See above comment for what this patch contains.

+++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
@@ -76,18 +76,40 @@ public function entityQueryAlter(SelectInterface $query) {
+      $fields = $this->entityFieldManager->getFieldStorageDefinitions('comment');
+      $host_entity_type_id = $fields['entity_id']->getSetting('target_type');

That is always going to be 'node' as we saw earlier, because it's a base field, and Comment entity doesn't set the target type on the base field definition, but entity reference has 'node' as the default in EntityReferenceItem:

> 'target_type' => \Drupal::moduleHandler()->moduleExists('node') ? 'node' : 'user',

I think we have to examine why the test is needing this logic, since doing stuff in the UI doesn't seem to.

EDIT: Ah, it's as a non-admin. And of course, I'm testing as an admin :/

OK, I think a mixed approach between the current patch and the one suggested in #50 can get us where we need to.

We can make the checks only when we have `$entity`, which makes sense, but also add an additional check of `$entity->access(...` in the method `getReferenceableEntities`, which we can just override from the parent class into the `CommentSelection` class.

I'll work on it.

This is what I mean:
* We only do the additional checks in "entityQueryAlter" when we have the comment entity, which will be in most cases. If we actually don't have the host entity there is no extra checks for access that we can't do.
* In "getReferenceableEntities", we use almost the same code that the parent class, with an additional line if ($entity->access('view', $this->currentUser)) {, which makes sure that the comment can be accessed. The only reason why we don't call the parent here and loop through results (as originally done in #50) is that we'd need to load up entities again and (as mentioned in #54) that might not scale up, so we introduce the additional "if" over the existing loop, which would be run anyway, so there is no "scaling" problem in my opinion.

With this patch, there are no assumptions or weird defaults.

+++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
@@ -76,19 +77,75 @@ public function entityQueryAlter(SelectInterface $query) {
+    if ($comment) {

I still don't understand what circumstances produce no $comment here/ [*]

But changing the logic so we don't apply any access in that situation doesn't seem right to me.

We maybe need to go back to #48 and figure out a new way to get the information in at this point.

[*] Though really, what on EARTH is an entity doing in the configuration array ANYWAY? It's hardly actual configuration is it???

    $comment = $this->getConfiguration()['entity'];

That just smells wrong.

I still don't understand what circumstances produce no $comment here

I think there are two cases that will produce the different outcomes:
* Case 1: Create a new comment: in this case, we always have a host entity to validate against. Validate is the key-word here, as the "entityQueryAlter" is called eventually from "ValidReferenceConstraintValidator::validate()", where the entity is set in this line $entity = !empty($value->getParent()) ? $value->getEntity() : NULL;
"ValidReferenceConstraintValidator" can be seen in the backtrace here:

#0 /var/www/html/web/core/modules/system/system.module(1262): Drupal\comment\Plugin\EntityReferenceSelection\CommentSelection->entityQueryAlter(Object(Drupal\mysql\Driver\Database\mysql\Select)) 
#1 /var/www/html/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(562): system_query_entity_reference_alter(Object(Drupal\mysql\Driver\Database\mysql\Select), NULL, NULL) 
#2 /var/www/html/web/core/lib/Drupal/Core/Database/Query/Select.php(484): Drupal\Core\Extension\ModuleHandler->alter('query', Object(Drupal\mysql\Driver\Database\mysql\Select)) 
#3 /var/www/html/web/core/lib/Drupal/Core/Database/Query/Select.php(509): Drupal\Core\Database\Query\Select->preExecute() 
#4 /var/www/html/web/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(271): Drupal\Core\Database\Query\Select->execute() 
#5 /var/www/html/web/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(83): Drupal\Core\Entity\Query\Sql\Query->result() 
#6 /var/www/html/web/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php(382): Drupal\Core\Entity\Query\Sql\Query->execute() 
#7 /var/www/html/web/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php(133): Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection->validateReferenceableEntities(Array) 
#8 /var/www/html/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(196): Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraintValidator->validate(Object(Drupal\Core\Field\EntityReferenceFieldItemList), Object(Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraint)) 
#9 /var/www/html/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(148): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints(Object(Drupal\Core\Field\EntityReferenceFieldItemList), '000000000000079...', Array) 
#10 /var/www/html/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(158): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object(Drupal\Core\Field\EntityReferenceFieldItemList)) 
#11 /var/www/html/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(100): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter), Array, true) 
#12 /var/www/html/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php(93): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate(Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter), NULL, NULL) 
#13 /var/www/html/web/core/lib/Drupal/Core/TypedData/TypedData.php(132): Drupal\Core\TypedData\Validation\RecursiveValidator->validate(Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter)) 
#14 /var/www/html/web/core/lib/Drupal/Core/Entity/ContentEntityBase.php(489): Drupal\Core\TypedData\TypedData->validate() 
#15 /var/www/html/web/core/lib/Drupal/Core/Entity/ContentEntityForm.php(188): Drupal\Core\Entity\ContentEntityBase->validate() 
#16 [internal function]: Drupal\Core\Entity\ContentEntityForm->validateForm(Array, Object(Drupal\Core\Form\FormState)) 
#17 /var/www/html/web/core/lib/Drupal/Core/Form/FormValidator.php(82): call_user_func_array(Array, Array) 
#18 /var/www/html/web/core/lib/Drupal/Core/Form/FormValidator.php(275): Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object(Drupal\Core\Form\FormState)) 
#19 /var/www/html/web/core/lib/Drupal/Core/Form/FormValidator.php(118): Drupal\Core\Form\FormValidator->doValidateForm(Array, Object(Drupal\Core\Form\FormState), 'comment_comment...') 
#20 /var/www/html/web/core/lib/Drupal/Core/Form/FormBuilder.php(588): Drupal\Core\Form\FormValidator->validateForm('comment_comment...', Array, Object(Drupal\Core\Form\FormState)) 
#21 /var/www/html/web/core/lib/Drupal/Core/Form/FormBuilder.php(320): Drupal\Core\Form\FormBuilder->processForm('comment_comment...', Array, Object(Drupal\Core\Form\FormState)) 

* Case 2: Query entities that can be referenced: as the above "validate" function is never called, the "entity" is never set, but the "entityQueryAlter" is still called, in this case originating from "getReferenceableEntities". So this is the case where the "$comment" is not available in that function.
See backtrace of calls here:

#0 /var/www/html/web/core/modules/system/system.module(1262): Drupal\comment\Plugin\EntityReferenceSelection\CommentSelection->entityQueryAlter(Object(Drupal\mysql\Driver\Database\mysql\Select))
#1 /var/www/html/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(562): system_query_entity_reference_alter(Object(Drupal\mysql\Driver\Database\mysql\Select), NULL, NULL)
#2 /var/www/html/web/core/lib/Drupal/Core/Database/Query/Select.php(484): Drupal\Core\Extension\ModuleHandler->alter('query', Object(Drupal\mysql\Driver\Database\mysql\Select))
#3 /var/www/html/web/core/lib/Drupal/Core/Database/Query/Select.php(509): Drupal\Core\Database\Query\Select->preExecute()
#4 /var/www/html/web/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(271): Drupal\Core\Database\Query\Select->execute()
#5 /var/www/html/web/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(83): Drupal\Core\Entity\Query\Sql\Query->result()
#6 /var/www/html/web/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php(129): Drupal\Core\Entity\Query\Sql\Query->execute()
#7 [internal function]: Drupal\comment\Plugin\EntityReferenceSelection\CommentSelection->getReferenceableEntities(NULL, 'CONTAINS')
#8 /var/www/html/web/core/modules/system/tests/src/Functional/Entity/EntityReferenceSelection/EntityReferenceSelectionAccessTest.php(102): call_user_func_array(Array, Array)
...

But changing the logic so we don't apply any access in that situation doesn't seem right to me.

We are applying the logic when we have the entity to check against (case 1). We're also checking the access when trying to call "getReferenceableEntities", which I think is the right thing (case 2)

@joachim - doesn't the above comment (#94) explain the different cases? I think the patch covers them both. Let me know your thoughts. Thanks.

Manually tested this.

Steps taken:

* Install minimal profile
* Install comment module
* Install field UI module
* Add a new comment type, with target entity-type of User
* Add a comment field to the user entity-type and choose the comment type from above
* Configure the compact user view mode to remove the comment field, this avoids recursion (see #2114887: Maximum nesting level when attaching comment field to User.)
* Add a comment, reply to that comment - works fine


* Uninstall node module, attempt to reply to a comment
* Get the error as expected

* Apply patch and try again, works

+++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
@@ -76,19 +77,75 @@ public function entityQueryAlter(SelectInterface $query) {
+  public function countReferenceableEntities($match = NULL, $match_operator = 'CONTAINS') {
+    $options = $this->getReferenceableEntities($match, $match_operator);
+    return count($options, COUNT_RECURSIVE) - count($options);

Moving to a code-path that loads the entities rather than just querying feels like it could be a performance issue.

But I checked and TermSelection is already using the full-entity load path too.

In addition, I can't find any calls to ::countReferenceableEntities outside of test code, so I don't think there is a performance issue either way.

We should open a follow up to deprecate this method for removal in D11. Added the needs followup tag.

I looked into #93 and indeed, there are instances of where a selection handler is instantiated without an entity key in $configuration, so this is a valid guard.

On that basis this is RTBC in my book, manually tested well.

xjm asked in slack if this needed an upgrade path, but it doesn't it has no impact on existing sites/data.

#3294840: Consider deprecating and removing \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface::countReferenceableEntities added follow up

I still think this line smells wrong:

    $entity = $this->getConfiguration()['entity'];

That would be "Case 1" outlined in #94, when creating a new comment.

* Case 1: Create a new comment: in this case, we always have a host entity to validate against. Validate is the key-word here, as the "entityQueryAlter" is called eventually from "ValidReferenceConstraintValidator::validate()", where the entity is set in this line
$entity = !empty($value->getParent()) ? $value->getEntity() : NULL;

Note that '$entity' was introduced here #2879918: Send entity to selection handler on validate. (also mentioned here #2875747: ValidReferenceConstraintValidator missing field entity).
Commit: https://git.drupalcode.org/project/drupal/commit/032ad72b49a7c4c8c5b907d...

What I'm iffy about is having something that is only about the current request (the $entity) inside something that's persistent (the config). It's mixing two totally different levels together.

That's unrelated to this patch though, it's the API we have at this point

For posterity, the API is \Drupal\Core\Entity\EntityReferenceSelection\SelectionPluginManager::getSelectionHandler which accepts an $entity parameter and that is in turn passed as a configuration option when instantiating the plugin.

Thanks for the details. Let's file a follow-up to tidy that up then.

Yeah I agree it should be a dedicated method/property

Opened the follow-up that @larowlan mentioned in #100: #3295649: Deprecate SelectionInterface::countReferenceableEntities.

1. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -76,19 +77,75 @@ public function entityQueryAlter(SelectInterface $query) {
   +      $host_entity_type = $this->entityTypeManager->getDefinition($host_entity_type_id);
   +      $host_entity_field_data_table = $host_entity_type->getDataTable();
   +      $id_key = $host_entity_type->getKey('id');
   +
   

   Nit - why not set $id_key inside the if, it won't be used otherwise.

2. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -76,19 +77,75 @@ public function entityQueryAlter(SelectInterface $query) {
   +      if ($host_entity_field_data_table) {
   +        // The Comment module doesn't implement any proper comment access,
   +        // and as a consequence doesn't make sure that comments cannot be viewed
   +        // when the user doesn't have access to the entity.
   +        $entity_alias = $query->innerJoin($host_entity_field_data_table, 'n', "[%alias].[$id_key] = [$data_table].[entity_id] AND [$data_table].[entity_type] = '$host_entity_type_id'");
   +        // Pass the query to the entity access control.
   

   I know this comment is just moved, but this looks like we're working around comment module not implementing entity access, in comment module - should we open a follow-up?

3. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -76,19 +77,75 @@ public function entityQueryAlter(SelectInterface $query) {
   +        if ($host_entity_type_id == 'node') {
   

   Could be a strict comparison.

New patch attached.
1. Addressed
2. Comment module implements entity access checks as far as I can see in the code. There is "CommentAccessControlHandler.php" and even within the "CommentSelection.php" we check access in "getReferenceableEntities" method. I think that the comment (and check) is there for scalability reasons based on #54 feedback.
3. Addressed

As the changes are really minimal in 1 and 3, I'm marking it as RTBC as per #100 (not sure if this is the right process).

+++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
@@ -76,19 +77,76 @@ public function entityQueryAlter(SelectInterface $query) {
+
+        // The Comment module doesn't implement any proper comment access,
+        // and as a consequence doesn't make sure that comments cannot be viewed
+        // when the user doesn't have access to the entity.
+        $entity_alias = $query->innerJoin($host_entity_field_data_table, 'n', "[%alias].[$id_key] = [$data_table].[entity_id] AND [$data_table].[entity_type] = '$host_entity_type_id'");

If comment module does indeed implement proper access and we're doing this for performance, I think the comment should be changed to reflect this.

All the access checks in the comment module are against the host entity, as the comment is a field within it.

Changed the comment to:

// The Comment module doesn't implement per-comment access, so it
// checks instead that the user has access to the host entity.

If you think there is a better suggestion please let me know.
Patch re-attached.

Comment looks much better thank you!

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

Enough changes in here I don't think we should backport to 9.4.x, so marking fixed against 9.5.x for now.

This introduced a regression, if you have a comment reference field on an entity type that is _not_ a comment itself, it fails with a fatal error.

The entity that's passed to configuration is not the entity being referenced, it's the host.

As a quickfix, it needs to have an instanceof CommentInterface, but I'm not sure if just skipping this logic then is valid. Will create an issue and patch.

Follow-up issue for this: #3332546: CommentSelection::entityQueryAlter() fails on validate when referencing entity is not a comment