Problem/Motivation

Comments threading fails on comment fields attached to custom entities as it assumes that comments are always attached to nodes when validating references, even if the "node" module is not installed (it's not a hard dependency and that is totally fine).

When trying to save a reply to a comment, you get the following:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'db.node_field_data' doesn't exist: 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 (:db_condition_placeholder_0); Array ( [:db_condition_placeholder_0] => 3 ) in Drupal\Core\Entity\Query\Sql\Query->result() (line 271 of core/lib/Drupal/Core/Entity/Query/Sql/Query.php).
Drupal\Core\Database\StatementWrapper->execute(Array, Array) (Line: 937)
Drupal\Core\Database\Connection->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 (:db_condition_placeholder_0)', Array, Array) (Line: 512)
Drupal\Core\Database\Query\Select->execute() (Line: 271)
Drupal\Core\Entity\Query\Sql\Query->result() (Line: 83)
Drupal\Core\Entity\Query\Sql\Query->execute() (Line: 382)
Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection->validateReferenceableEntities(Array) (Line: 133)
Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraintValidator->validate(Object, Object) (Line: 196)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints(Object, '000000004162599c000000002f23063f', Array) (Line: 148)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object) (Line: 158)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object, Array, 1) (Line: 100)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate(Object, NULL, NULL) (Line: 93)
Drupal\Core\TypedData\Validation\RecursiveValidator->validate(Object) (Line: 132)
Drupal\Core\TypedData\TypedData->validate() (Line: 489)
Drupal\Core\Entity\ContentEntityBase->validate() (Line: 188)
Drupal\Core\Entity\ContentEntityForm->validateForm(Array, Object)
call_user_func_array(Array, Array) (Line: 82)
Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object) (Line: 275)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'comment_api_comment_form') (Line: 118)
Drupal\Core\Form\FormValidator->validateForm('comment_api_comment_form', Array, Object) (Line: 588)
Drupal\Core\Form\FormBuilder->processForm('comment_api_comment_form', Array, Object) (Line: 320)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 48)
Drupal\Core\Entity\EntityFormBuilder->getForm(Object) (Line: 268)
Drupal\comment\Controller\CommentController->getReplyForm(Object, Object, 'comments', '3')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 158)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

In this case, "api_comment" is a comment type attached to a custom entity.

Steps to reproduce

* Create a comment type where the "Target Entity Type" is your custom entity.
* Create a custom entity with a comments field attached to it, where the "comment_type" key is the newly created comment type.
* Allow comments and replies.
* Visit the custom entity page and make a comment, it should work.
* Now, reply to that comment, you'll get the error described above.

Proposed resolution

Check if the "node" module is enabled before making additional SQL-joins and node-related checks.

Remaining tasks

Provide a patch for this.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Do no check "node access" in comments when the "node" module is not even enabled.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fjgarlin created an issue. See original summary.

fjgarlin’s picture

Attaching patch to check if the module exists before running the code.

The patch just wraps the node-related code in a "if" statement that checks if the "node" module is enabled:

if ($this->moduleHandler->moduleExists('node')) {
  ...
}
fjgarlin’s picture

Title: Comment module uses node-related tables even if node module is not installed » Do not check node_access if node module is not installed
larowlan’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Bug Smash Initiative

Wow, I can't believe we haven't found this before.

Can you add replying to \Drupal\Tests\comment\Functional\CommentNonNodeTest to cover this functionality

Great work

fjgarlin’s picture

I guess there aren't that many sites not using the "node" module.

Thanks for pointing me to the right test. I added a test case for threading. The test fails without the fix (expected) and works with the fix (expected too).

Attaching the new patch with the test.

fjgarlin’s picture

Needed to pull latest changes on the branch. Re-rolled the patch.

fjgarlin’s picture

larowlan’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/tests/src/Functional/CommentNonNodeTest.php
@@ -120,7 +119,7 @@ protected function setUp(): void {
+  public function postComment($entity, $comment, $subject = '', $contact = NULL) {

@@ -311,6 +310,12 @@ public function testCommentFunctionality() {
+    $comment1_2 = $this->postComment(NULL, $this->randomMachineName(), $this->randomMachineName());

Can you elaborate on why this change is needed?

Posting a comment would always be against an entity right?

fjgarlin’s picture

Status: Needs work » Needs review

The first change was needed so NULL would be accepted. The behaviour is as explained in the comments:

   * @param \Drupal\Core\Entity\EntityInterface|null $entity
   *   Entity to post comment on or NULL to post to the previously loaded page.

When the entity is passed, the test tries to get the comment page for the entity, NOT for the reply:

    // Must get the page before we test for fields.
    if ($entity !== NULL) {
      $this->drupalGet('comment/reply/entity_test/' . $entity->id() . '/comment');
    }

And that's the reason why we get the comment reply page before calling the "postComment" method:

    $this->drupalGet('comment/reply/entity_test/' . $this->entity->id() . '/comment/' . $comment1->id());
    $comment1_2 = $this->postComment(NULL, $this->randomMachineName(), $this->randomMachineName());

This is the same approach followed in "CommentThreadingTest.php".

larowlan’s picture

+++ b/core/modules/comment/tests/src/Functional/CommentNonNodeTest.php
@@ -120,7 +119,7 @@ protected function setUp(): void {
-  public function postComment(EntityInterface $entity, $comment, $subject = '', $contact = NULL) {
+  public function postComment($entity, $comment, $subject = '', $contact = NULL) {

Ah in that case, can we make it ?EntityInterface instead

Are you able to upload a version of the patch that has only the test changes, it should fail - to demonstrate the test catches the bug.

And then the full patch as well (after the test only one) so that the bot doesn't kick your issue back to needs work.

Thanks

fjgarlin’s picture

Status: Needs review » Needs work

The last submitted patch, 11: test_only-3272023-11.patch, failed testing. View results

fjgarlin’s picture

Above fail expected.

Uploading now the patch with the test AND the fix, which includes the feedback given.
Thanks for the guidance.

larowlan’s picture

joachim’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
@@ -76,18 +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'");
+      // Pass the query to the node access control.
+      $this->reAlterQuery($query, 'node_access', $node_alias);

This seems to be still assuming that the comment is attached to a node.

What if node module is present, but we're dealing with comments attached to some other entity type?

I think this needs to check $this->configuration['target_type'].

fjgarlin’s picture

That's right @joachim, that's the check that makes sense in this case. Thanks for pointing me to the right check.
Attaching a new version of the patch.

Status: Needs review » Needs work
fjgarlin’s picture

The test fails as "target_type" => "comment" for all comments, regardless of which type of entity the comments are joined to.
#13 would still be the most-valid test for now.

I can't find a way to get the current entity being worked on (ie: the comment) to be able to check where it is going to be added to. I guess this is because the comment doesn't exist yet (which explains the actual "target_type" being "comment" as that's what we are creating).

fjgarlin’s picture

The closest thing to a dynamic check I've got is:

$field_map = $this->entityFieldManager->getFieldMapByFieldType($this->getConfiguration()['target_type']);

which returns:

array:1 [
  "node" => array:1 [
    "comment" => array:2 [
      "type" => "comment"
      "bundles" => array:1 [
        "article" => "article"
      ]
    ]
  ]
]

So I am checking that the "node" key exists in the array, but I'm not 100% sure about this. It passes the test that fails before, re-uploading the new patch to see if it passes all the tests.

@joachim @larowlan - if you could review the new check that'd be great.

joachim’s picture

Status: Needs review » Needs work

Checking getFieldMapByFieldType() seems a bit flaky to me. Even if we know the name of the field we're on, multiple entity types could have that field, and they could all be comment fields!

It does look like within entityQueryAlter(), neither $this->pluginDefinition nor $this->configuration have what we need.

The $query object does know which entity type is being attempted to be referenced -- but in the context of a new child comment, that entity type is 'comment', because we're checking the parent!

Comment entities *do* know what their top-level host is -- the entity_type field on all comments in the thread is 'node', and even with a new comment, the value is set. 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.

We *could* get that passed in via $value->getFieldDefinition():

    $handler = $this->selectionManager->getSelectionHandler($value->getFieldDefinition(), $entity);

because then that would get to:

  public function getSelectionHandler(FieldDefinitionInterface $field_definition, EntityInterface $entity = NULL) {
    $options = $field_definition->getSetting('handler_settings') ?: [];
    $options += [
      'target_type' => $field_definition->getFieldStorageDefinition()->getSetting('target_type'),
      'handler' => $field_definition->getSetting('handler'),
      'entity' => $entity,
    ];
    return $this->getInstance($options);
  }

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

@larowlan what do you think?

fjgarlin’s picture

Agree with you. I wasn't happy with that check (and the results of the testing aren't happy either).

For now, #13 is still the most valid patch, but I also understand that that's only half of the issue here:

  • ✓ - if "node" module is not enabled then don't even check anything node-related (this is what #13 covers)
  • x - if the comment is attached to a node entity, then and only then, do the checks

I'll wait on @larowlan feedback to your suggestion as I'm a bit lost on that part.

joachim’s picture

The incorrect access check is not going to be a simple fix.

So, the situation at the moment for comments attached to non-nodes is:

- site with node module NOT installed: crashes
- site with node module installed: access check is broken and probably causes incorrect access

#13 will change that to:

- site with node module NOT installed: works ok, since the node access check will be totally skipped
- site with node module installed: access check is broken and probably causes incorrect access

So #13 is fixing a crash in one scenario, and leaving the other scenario the same.

Given that, we could go with #13 and file a second issue for the incorrect access check.

larowlan’s picture

Status: Needs work » Closed (duplicate)

I think this is a duplicate of #2958241: Impossible to reply to comments: commented entity considered unreferencable because CommentSelection::entityQueryAlter() joins on {node_field_data} table - in that issue they're making it work for any entity type

However, what they're lacking is tests, which you've added here

So I think the next step is to take your test, with their patch and post it over there and see if it works