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.
Comment | File | Size | Author |
---|---|---|---|
#13 | do_not_check_node_access_if_module_not_installed-3272023-13.patch | 3.92 KB | fjgarlin |
#11 | test_only-3272023-11.patch | 1.59 KB | fjgarlin |
Comments
Comment #2
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedAttaching 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:
Comment #3
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedComment #4
larowlanWow, I can't believe we haven't found this before.
Can you add replying to
\Drupal\Tests\comment\Functional\CommentNonNodeTest
to cover this functionalityGreat work
Comment #5
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedI 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.
Comment #6
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedNeeded to pull latest changes on the branch. Re-rolled the patch.
Comment #7
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedFixing coding standards warning.
Comment #8
larowlanCan you elaborate on why this change is needed?
Posting a comment would always be against an entity right?
Comment #9
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedThe first change was needed so NULL would be accepted. The behaviour is as explained in the comments:
When the entity is passed, the test tries to get the comment page for the entity, NOT for the reply:
And that's the reason why we get the comment reply page before calling the "postComment" method:
This is the same approach followed in "CommentThreadingTest.php".
Comment #10
larowlanAh 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
Comment #11
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedTest only patch.
Comment #13
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedAbove fail expected.
Uploading now the patch with the test AND the fix, which includes the feedback given.
Thanks for the guidance.
Comment #14
larowlanI think #2958241: Impossible to reply to comments: commented entity considered unreferencable because CommentSelection::entityQueryAlter() joins on {node_field_data} table may be a duplicate here
Comment #15
joachim CreditAttribution: joachim as a volunteer commentedThis 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'].
Comment #16
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedThat'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.
Comment #18
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedThe 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).
Comment #19
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedThe closest thing to a dynamic check I've got is:
which returns:
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.
Comment #20
joachim CreditAttribution: joachim as a volunteer commentedChecking 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():
because then that would get to:
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?
Comment #21
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedAgree 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:
I'll wait on @larowlan feedback to your suggestion as I'm a bit lost on that part.
Comment #22
joachim CreditAttribution: joachim as a volunteer commentedThe 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.
Comment #23
larowlanI 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