Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Working on tests for #2422353: Comment module should check that comment body field exists Ifound that this assertion is wring because path for reply is wrong
Also messages needs fix too
- $this->drupalGet('comment/reply/entity_test/comment/' . $new_entity->id());
- $this->assertNoFieldByName('subject[0][value]', '', 'Subject field found.');
- $this->assertNoFieldByName('comment_body[0][value]', '', 'Comment field found.');
+ $this->drupalGet('comment/reply/entity_test/' . $new_entity->id() . '/comment');
+ $this->assertResponse(200);
+ $this->assertNoFieldByName('subject[0][value]', '', 'Subject field not found.');
+ $this->assertNoFieldByName('comment_body[0][value]', '', 'Comment field not found.');
The problem here that this page exposes 3 comment forms, so this assertions should use needed form only
Here a screenshot of the below step $this->drupalGet('comment/reply/entity_test/comment/' . $new_entity->id()); in the current codebase:
Actual reply with 3 comment forms:
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | commentnonnodetest-2458323-9.patch | 2.71 KB | nlisgo |
| 1.patch | 1017 bytes | andypost |
Comments
Comment #1
andypostComment #3
nlisgo commentedComment #4
nlisgo commentedAs the subject and comment body fields are appearing either this is a bug or the appearance of these fields is expected behaviour. Perhaps the messages did not need fixing but the assertions did.
Comment #5
andypost@nlisgo Yes, that I exactly talking about! if you take #3 screenshot after patching you still see the 2 comment forms
The test should make sure that comments hidden when field settings changed to hidden (
$this->assertFieldChecked('edit-field-foobar-0-status-2');before the changed hunk)But the page we are using have 3 comment fields, so patch should remove unneeded fields to test properly that comments and form are hidden
Comment #6
nlisgo commentedOK, I think I know what the expected behaviour is now. I will adjust the patch accordingly.
Comment #7
nlisgo commentedI expect this to fail but would appreciate feedback on the adjustment to the tests that I believe now check the expected behaviour.
There are indeed 3 comment fields for the entity_test entity. The first comment field is of the default comment type and the other comment fields are of the type foobar.
The field settings for the first comment field have been changed to set the comment status to closed.
When a new entity is created of entity_type and we visit the entity manage screen we expect the first comments status to be set to closed.
When we visit the entity page we should not expect to see an add comment form for the first comment field.
Comment #9
nlisgo commentedI added some tests initially for debugging purposes but they are a welcome addition IMO. Basically I wanted to the visit the /entity_test/add screen and see if the comment fields open and closed state was as expected and then create an entity by submitting the form and then visiting the new entity page. These additional tests pass.
There seems to be an issue when trying to create the entity programmatically. I have not been able to recreate this issue outside of the test suite.
Comment #11
nlisgo commentedI'm fairly sure that the tests cover the expected behaviour now. Please offer any feedback, however.
I will shift my attention to trying to find out what is going on when the entity is created programmatically here.
The result is that the default value for the comments form status is ignored.
Comment #12
andypost@nlisgo test looks great, otoh having 3 comment forms on reply is UX fail.
@larowlan Here's how it looks now (with fixed path)
Do we need to file new issue - do not display other comment forms on comment reply route? Or just re-purpose current?
Comment #13
larowlanYeah on the reply route we can be smart about this.
We already remove the formatter for the active field to prevent recursion, we should do it for all fields.
Comment #14
nlisgo commentedI'm not sure if the discussion about whether we should allow multiple comments fields to be attached or displayed on a single entity belongs here. I'm pretty sure that even if I refactored the tests to only output one comments field form I could reproduce the bug.
Let's take on addressing this bug as the priority and we can open a separate issue if you desire.
Comment #15
nlisgo commentedI can confirm that the bug is still present with one comment field.
Comment #16
andypost@nlisgo When I filed the issue, I thought that need to fix path in
drupalGet()for reply form, then discovered that there's 3 comment forms. I think better to re-purpose the issue to fix display of 3 comment forms on reply page, that hunk of test looks like was for that.I'm going to to grep the code in https://www.drupal.org/sandbox/larowlan/1790736 where we originally introduced this test
EDIT can't find origin of that test, but foobar was added in #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form
EDIT2
this 2 commits introduces that and both from @larowlan
Comment #17
larowlanYes, lets focus on the incorrect url here
Comment #18
nlisgo commentedThe url appears correct now.
The url for the reply form is correct now, I believe.
Is the problem that you would not expect to see other comment reply forms (foobar and barfoo) at this url?
The commented entity appears below the comment form, are we to hide all comment fields in the commented entity?
Comment #19
andypostThinking a bit more about, suppose we just need to check that form absent for comment field that we set to closed.
And then in new issue we extend this test to make sure no other forms here
Comment #20
andypostComment #21
bfr commentedThe patch applies just fine, no need to reroll.
Comment #30
quietone commented@andypost, Thanks for that patch.
It appears that this was fixed as part of #2422353: Comment module should check that comment body field exists.
Therefore, closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.
Thanks!
Comment #31
andypost@quietone thank you for triaging