Problem statement:

There are no tests to check for correct behaviour of automatically creating comment subjects based on comment body content.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because it only includes new automated tests.
Issue priority Minor
Unfrozen changes Unfrozen because it only adds automated tests.
Prioritized changes This is not a prioritized change for the beta phase.
Disruption This change is not disruptive, and it does decrease fragility.

Original bug report:

Small change to the extraction of titles from comments for sites who have the subject line turned off; the existing method would just chop at the 29th letter, this mod will chop at the first word-boundary (perl's definition) after the 29th letter.

I confess I know next to nothing about utf8, so this patch may be naive. I take the truncate_utf8() result from strip_tags() up to the 50th letter, and hunt for the subject in there.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ricabrantes’s picture

Version: x.y.z » 7.x-dev
Status: Active » Closed (fixed)

bump

ricabrantes’s picture

Status: Closed (fixed) » Active
jhodgdon’s picture

Status: Active » Needs work

This method will not work with CJK languages (Chinese, etc.) I think.
See
http://drupal.org/node/269911#comment-2795478

jhodgdon’s picture

jhodgdon’s picture

The correct way to do this would be to use the word-safe argument to truncate_utf8(), not to reinvent a new way of finding a word boundary.

Also, as a note: Comment #3 is irrelevant now, since truncate_utf8() is being fixed to work for non-Latin languages.

Anonymous’s picture

Version: 7.x-dev » 8.x-dev

Feature request, moving to 8.x-dev.

andypost’s picture

Category: Feature request » Bug report
Issue summary: View changes
Issue tags: +Needs tests, +Needs reroll

Bug needs test and new patch

samwilson’s picture

Assigned: Unassigned » samwilson

@erangakm and I are working on this.

samwilson’s picture

Issue tags: +DrupalSouth
FileSize
2.42 KB

The issue of breaking only on a word boundary is already fixed, because Unicode::truncate() is being used, so that's all ok and once the attached test is finished this issue can probably be closed.

However, there are some other issues, such as the assumption that the body field is actually present in a comment (which is doesn't have to be). That is perhaps a separate issue though.

Attached is a patch containing only a new test for the subject field truncation. It is not yet complete, becuase is not working for HTML input formats; but it's a start, we hope. We were also thinking of moving the truncation code into the Comment entity (by overriding set('body') probably) from Drupal\comment\CommentForm

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Thanx for taking that, part of assumption about "fixed body" should be fixed in #1885962: Comment tokens should use entity translation API

@samwilson would be great if you file separate issue to discover how much code supposes that comment_body field always exists

The last submitted patch, comment-title.patch, failed testing.

andypost’s picture

There's one!

@samwilson please mention the places you found

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -196,4 +196,47 @@ function testCommentInterface() {
+    $this->drupalLogin($this->adminUser);
...
+    $comment1 = $this->postComment(null, $body_text, '', true);
...
+    $this->assertEqual('Lorem ipsum Lorem ipsum', $comment1->getSubject());
...
+    $comment2 = $this->postComment(null, $body_text2, '', true);
+    $this->assertEqual('LoremipsumloremipsumLoremingi', $comment2->getSubject());

I think better to use kerneltest to make sure that comment entity subject field is populated from comment_body field if last one exists

The last submitted patch, 11: 11883-test-trucated-subject-11.patch, failed testing.

samwilson’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.49 KB

Here's a (hopefully) working test.

I've put a bit of common stuff into CommentInterfaceTest::setUp() (out of testCommentInterface()) and added two new test methods. The first tests plain text automatic subject creation; the second, HTML.

The HTML one is a bit weird, because I had to create two new input filters in order to be able to select one of them in the form (otherwise, it'd default to plain text still). Then it creates a user that's allowed to use those filter formats. Silly eh? I did have a crack at using KernelTestBase, but I'm afraid I got a bit confused with getting it all bootstrapped and things and gave up... I think the only drawback in doing it the current way is speed.

Oh, and another place the body field is assumed is in CommentForm::buildEntity(). I was going to refactor that, but I'm still a bit unsure about how that should be done, so will await advice. :)

Thanks!

andypost’s picture

That strange that tests are green, for comment body see patch in #2422353-3: Comment module should check that comment body field exists
This is exactly a place where title generated

samwilson’s picture

Why strange? The tests are passing because they're not dealing with the assumed-comment-body thing; they're just testing the matter referred to in the original post of this issue — i.e. that an automatically-created subject is truncted correctly at a word boundary.

The other is a separate issue isn't it? #2422353?

larowlan’s picture

These tests look great - just a couple of minor changes and I think this is RTBC.
We also need to add a beta-evaluation template to the issue summary to assist committers in evaluating if this is ok to include during Beta - ping me on irc if you need pointers on how to do that.

  1. +++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
    @@ -196,4 +202,82 @@ function testCommentInterface() {
    +    // can select one of them (there's probably a better way of doing this).
    

    Please remove the 'there's probably a better way', this is the right way because the tests use the testing profile, only standard profile auto-creates these for you. Great work by the way.

  2. +++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
    @@ -196,4 +202,82 @@ function testCommentInterface() {
    +    $filtered_html_format = entity_create('filter_format', array(
    ...
    +    $full_html_format = entity_create('filter_format', array(
    

    You can use FilterFormat::create() now

  3. +++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
    @@ -196,4 +202,82 @@ function testCommentInterface() {
    +    $this->assertEqual('Hello World', Comment::load(1)->getSubject());
    ...
    +    $this->assertEqual('(No subject)', Comment::load(2)->getSubject());
    

    Love it

larowlan’s picture

Title: Break Auto Comment Title on word boundary » Add test coverage for auto-comment title functionality and word-breaking

New title

samwilson’s picture

Thanks larowlan!

Here's an interdiff with those changes.

I've also had a crack at adding the Beta Evaluation template; let me know if it's ok. :-)

Status: Needs review » Needs work

The last submitted patch, 22: 11883-test-trucated-subject-22.interdiff.patch, failed testing.

samwilson’s picture

I'm not quite au fait with interdiffs, and that one just went red, so here's a full diff with the latest changes. :)

samwilson’s picture

Category: Bug report » Task
Status: Needs work » Needs review

Fixing metadata.

larowlan’s picture

@samwilson beta evaluation looks perfect - thanks again.

+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -196,4 +203,82 @@ function testCommentInterface() {
+  public function testAutoFilledSubject() {
...
+  public function testAutoFilledHtmlSubject() {

One minor amendment - because we have these in separate methods starting with test it means this test now installs Drupal 3 times (one for each method). Are you splitting them up for that reason, or are you splitting them up for readability/maintainability. If the latter, could I suggest naming these to methods doTest{x} instead of test{x} and then in the existing testCommentInterface method - call doTest{x} and doTest{y} - this gives us the separation but avoids the overhead of three Drupal installs for the one test class.

Thanks so much for pushing this issue along. A 5 digit issue nid shows how old it is.

samwilson’s picture

@larowlan that's a good point, I guess each additional test method does bring a fair bit of overhead. I'm more used to tests that are small and quick! And that run independently because they each test a separate part of the system.

That's what I'd be worried about with combining them to all use the same instance of Drupal: there's then always more of a possibility that one test is contaminated by another (and so perhaps could be a false positive... false negatives would of course not make it through). I can't see that that would be the case with these three tests, but if they're all in it seems to make it harder for people in the future to see exactly what's going on. Maybe? I don't know — that's just my opinion from the point of view of other systems I work in; I'm very much still learning the Drupal way! hehe

So if you'd rather they were combined (still in separate methods, though; for readability) then that's no worries, but I guess I'd rather keep them separate. :) What do you think?

Oh, the only thing that does get very slightly more annoying if they're in the same Drupal install is that point of knowing what the comment ID is going to be (based on the number of comments that have been added). With combined tests, that'd have to be a cumulative total (or parsed from the request URI as it is in postComment()). Not a biggie.

:) Thanks you for taking the time to give me such thorough feedback! It's a different world, this Drupal one.

larowlan’s picture

Assigned: samwilson » andypost

Lets' put it to @andypost for the final say

andypost’s picture

Status: Needs review » Reviewed & tested by the community

I'm fine with 3 tests, let's get commiter opinion

+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -196,4 +203,82 @@ function testCommentInterface() {
+    $this->assertEqual('(No subject)', Comment::load(2)->getSubject());
+
+  }

the only nitpic, could be fixed at commit time

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 11883-test-trucated-subject-24.patch, failed testing.

samwilson’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.65 KB

Rerolled the patch (and removed that extraneous newline at the end of the last test; thanks @andypost).

:-)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 98366a9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

diff --git a/core/modules/comment/src/Tests/CommentInterfaceTest.php b/core/modules/comment/src/Tests/CommentInterfaceTest.php
index 835e4d7..c775553 100644
--- a/core/modules/comment/src/Tests/CommentInterfaceTest.php
+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -271,7 +271,7 @@ public function testAutoFilledHtmlSubject() {
     $this->drupalPostForm(NULL, $edit1, t('Save'));
     $this->assertEqual('Hello World', Comment::load(1)->getSubject());
 
-    // If there's nothing other than HTML, the subect should be '(No subject)'.
+    // If there's nothing other than HTML, the subject should be '(No subject)'.
     $body_text2 = '<span></span><strong> </strong><span> </span><strong></strong> <br />';
     $edit2 = array(
       'comment_body[0][value]' => $body_text2,

Fixed on commit.

  • alexpott committed 98366a9 on 8.0.x
    Issue #11883 by samwilson, garym@teledyn.com: Add test coverage for auto...
andypost’s picture

Assigned: andypost » Unassigned

more then 10 years (since October 22, 2004)
@samwilson Thanx a lot!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.