Generate comment subjects from parent subject (re:...)

Was using this module in Drupal 4.7 and now using it in Drupal 5, it would be really good if it was part of drupal 6 core.

I agree. Please make this into drupal core.

no longer applies

+1 for adding this feature to the core

+1 for adding to core

Sounds good.

+1 for core

+1 This should definitely be a core feature. As I think it's just the natural way of subject titles.

+1 Don't understand why this feature is not in core

The reason this isn't in core is because no-one reviewed the patch when it was posted. If it's not tested, it can't be put into core.

If you'd like to help out with that, it first needs a re-roll against the latest Drupal 6 development snapshot to take into account changes in the comment module since the patch was originally posted. This can be done with minimal php knowledge in most cases.

+1, and it has a recent patch. Come on...

Please add this to 6, it seems "common sense" for inclusion.

Not three months after feature freeze it isn't.

If any one of the people who posted "+1" or "please add this" on this thread, had actually tested the patch and posted if it worked instead, it might be in already. If you test it now, and it still works, it might get in early for D7.

I don't think any of us is using the 6.x beta so we couldn't test it out for that. As for me, when I posted earlier, I had tested it with the 5.x version and it work wonderfully but I couldn't say the same about the 6.x code. So here's hoping somebody with a 6.x beta or a 7.x beta tries it out.

One more thing, this issue to me is more of a bug than a feature so maybe if we think about it like that, maybe we can fit this in one of the 6.x revisions.

Crimson, it takes about five minutes to install 6.x beta on nearly any system. For instructions on testing patches, go to http://drupal.org/patch

You can get the latest dvelopment snapshot from here: http://drupal.org/node/97368

catch

IF someone like you, who "know how things work here", had spent two minutes back in April posting the information that the patch "HAD" to be tested by us clueless ones AND that the results "HAD" to be posted here AND if you had posted the links to "instructions for testing patches" AND where to get the dEvelopment snapshot THEN I don't believe you would have felt the "NEED" to make post #16 AND the patch would probably be a part of the core now.

Instead of what-ifs, in the interest of not breaking what is there, we focus on fixing bugs, not adding features in 6.x. This is how Drupal's development always worked. Thanks for your contributions and all the best on getting this ready for 7.x and getting it reviewed.

KeithDaniels: I've been a drupal user for 2 1/2 years - only the past 6-12 months have I got involved in contributing regularly. My frustration on the numerous +1s and 'must be in cores' is because I spent the other 2 years doing much the same thing, and it had no effect (plus I didn't know how things worked and played version tennis a few times as well). I'd be happy to continue this conversation via my contact tab, but otherwise we should probably let this issue get back on topic.

Just wanted to say that I would find it nice to have this within the core

Subscribing.

Unless I'm mistaken, and please correct me if I'm wrong - I'm new to core patch testing, but the patch should be rolled from under Drupal root (http://drupal.org/patch/create). Also, because of the back-and-forth about versions is #25 a patch against Drupal HEAD?

Patch applied cleanly after editing the path to comment.module.

Received error message when replying to a comment.

notice: Undefined property: stdClass::$title in /var/www/drupal-cvs/modules/comment/comment.module on line 1373.

Steps to reproduce:

Checkout Drupal HEAD
Apply patch in #25
Turn on automatic subject for story content type
Create story node
Make a comment, leaving automatic subject in place
Click the reply link on this new comment
Error message

#29 patch applies cleanly and works correctly but the node_load to get the node title isn't necessary because the parent node object is loaded at the top of comment_form.

Here's ahoeben's #29 patch edited to use $node. I didn't do a full reroll but it still applies.

Tested comment to node and comment as reply to another comment and they all work correctly. Looks good.

+1 for core

I'm new, so do you replace the code in comment.module or add that code to comment.module?

amnion - take a look at http://drupal.org/patch

also - if you try this out, post back to say if you like how it works or not. Then it stands a higher chance of being committed to core.

There's a few minor code style issues (capitalize comments, end them in a period).

Also, you might look around and pick a form description that matches the form of other 'enable/disable' operations. Off the top of my head, I think most of them say "Enable or disable..." rather than a short question fragment.

Changed "Automatic comment subject" to "Default comment subject" with options "None" and "Inherit from parent" on the comment settings form.

Also fixed some remaining code style issues.

+1

Tested patch #41 against HEAD and I confirm that it does as described. In some situations the added behaviour is wanted (apparently by a lot of people, judging by the +1 crowd), so I opt for adding this to D7.

Using "Default comment subject" with mentioned options introduced by #41 has my preference, as it is much more to the point than the previous description.

A future feature (after putting this one in core) might be inheriting the subject from the parent comment when replying to a comment instead of the node. (nevermind, it already does)

I would like to mention that editing the title of the original node will throw the titles of the inherited subjects out of sync. This is not really a big deal, but something that I noticed.

Please make this part of drupal core.

@dmage: I suggest you test and review the patch aswell, because the more people test it, the sooner it gets into core.

@43, not syncing the titles is a good thing IMO.

That's really needed...

@47:
Please test the patch and report back here, otherwise it will never get into core.

Patch #41 applied to HEAD.

Works as advertised when adding comment to a node.
Does not work when replying to a comment:

( ! ) Fatal error: Call to undefined function _comment_load() in modules/comment/comment.module on line 1391

Call Stack
#	Time	Memory	Function	Location
1	0.0002	57728	{main}( )	../index.php:0
2	0.1884	6750628	menu_execute_active_handler( )	../index.php:18
3	0.1944	7059688	call_user_func_array ( )	../menu.inc:346
4	0.1944	7059688	comment_reply( )	../menu.inc:0
5	0.2025	7306664	comment_form_box( )	../comment.pages.inc:102
6	0.2025	7306664	drupal_get_form( )	../comment.module:1464
7	0.2025	7306664	call_user_func_array ( )	../form.inc:102
8	0.2025	7306664	drupal_retrieve_form( )	../form.inc:0
9	0.2026	7306664	call_user_func_array ( )	../form.inc:358
10	0.2026	7306708	comment_form( )	../form.inc:0

Additionally, the description text in the node type comment options is confusing and provides little help.
Right now it states "Set a default value for the comment subject field.", while something like "Set 'Re: [parent title]' as comment title by default." would be more helpful.

Modified patch. Replaced _comment_load() with comment_load().
Applied to HEAD. Seems to work correctly, but see remark below:

Remark:
when replying to a comment, the default subject is Re: [comment subject] when the comment that is being replied to had a custom subject. However, when the comment that is being replied had [Re: node title] as subject, the default subject is still [Re: node title], where I would have expected [Re: Re: node title].

Example:
- node title = My story
- comment 1: proposed subject is [Re: My Story]
-- comment 1a (reply to comment 1) : proposed subject is [Re: My Story], while I expected [Re: Re: My Story]

- comment 2: custom subject 'Cool story'
-- comment 2a (reply to comment 2): proposed subject is [Re: Cool story].

Question: Should replies to comments be "Re: Re: Re:" chained or not? I feel they should.

At first I too expected 'Re: Re: Re: Title', but now I'm sorta turned.

- Many e-mail programs trim to 1 Re: (or 1 Fwd: ), so that is a somewhat seasoned methode.
- The threaded structure of the comments give enough information about the level of the reply.
- If someone breaks the chain by adding a custom title to his comment, the benefit of multiple Re:'s also is gone

-1 for chains of Re: Re: Re: - when I had an email client that did that, I used to end up editing the extra ones out.

Also Set 'Re: [parent title]' as comment title by default. from #49 would be a better string.

I'm not specifically *for* chains of Re:, I'm just stating that I *expected* the chaining behaviour.

I had not noticed many e-mail clients trim Re: and Fwd: subjects, so I'll just alter my expectations :)

+1 for core

momper, the status of the patch is "needs review", you can apply the patch to an install of Drupal 7, post back the results, and massively increase the chances of it getting into core. Had you done that, you'd have found out it no longer applies, attached is a quick edit to keep up with concat spacing (untested).

I've tested #56 and it patched some displacement warnings. Attached is a patch that patches without warnings.

All functionality is still as it should.

+1 for core - using it now on my 5.7 site.

I tested patch #57 against HEAD and it works like a charm.
Nice addition to the comment functionality!

I know the story well.

I really, really hate to do this after all the frustration and missed releases and patch re-rolls but I've got to...

The question asked is

Can users provide a unique subject for their comments?

and the possible answers are 'Disable' and Enable'.

Huh? Granted I've seen much worse, but a correct answer to the question posed should be yes or no.

I'll refrain for the moment from trying to save the poor 'users', which is what we call junkies. Authors perhaps? Commenters would be a poor choice, but is still better than junkies. I don't care if you know the difference, my mom would read this and wonder why we would consider giving any special privileges to addicts.

The code itself looks OK. Give me something I can show my mom and the English teacher down the street and let's RTBC the sucker and be done with it.

@macgirvin: The question "Can users provide a unique subject for their comments?" is actually the help text displayed below the radio buttons, not the title of the field, so I think it is fine the way it is.

Re-rolling patch, and adding some javascript (based on the code found in user.js) to hide the "default subject" field when comment subjects are disabled.

OK, I've seen the forms in action and agree that fixing the language nits I raised in #61 is outside the scope of this issue.

#62 applied cleanly (#57 was offset slightly) and both patches appear to work as advertised. I've run through several different use cases and have no issues with either. I'll let others who have been involved with this longer resolve whether or not to hide the option per the second part of #63. If it is decided to go with #57 it probably could use a refresh but otherwise consider this a successful test/review.

Seems like a worthwhile functionality.

Patch looks generally good. I didn't test.

Points for improvement:

*

$subject = t('Re:') . " $subject";

This should use a placeholder for the $subject.
* There are several code standards spacing issues, e.g., $edit['subject']:'';.
* It looks like the prefixed subject will be used even if subjects are disabled for comments. Maybe this should be explained in the #description text for the setting.
* This would be better on separate lines as it was originally:

+    $form['subject'] = array('#type' => 'textfield', '#title' => t('Subject'), '#maxlength' => 64, '#default_value' => $subject);
+

+1 from me, if that's still an issue (update the module description if not)

what if you used a RTL language?

RFC 2047 covers non-ASCII text in message headers and it doesn't say anything about bidirectional text. I don't think it's allowed, though I suppose a directionality character could be used within an encoded word. Or, one of the encodings in RFC 1522 possibly could be used.

I get

notice: Undefined offset: 2 in /srv/www/vhosts/development.podhurl.ca/htdocs/d1/modules/comment/comment.module on line 715.''

whee replying to comments with this patch.

Robin

+1 for core functionality

You're correct, this error will happen even on a clean HEAD install. I'll file a separate issue. Sorry about that!

This patch itself causes no errors or warnings, and the comment simpletest ran fine with it. RTBC.

Robin

I vote against this patch. The fact that we are adding 'Re:' to the subjects makes no sense in an online discussion forum. Sorry, but this functionality will have to live in contrib.

@Dries: Can you elaborate on why that doesn't make sense? Personally, I don't mind using the module, but as this is a popular feature request, a longer explanation might be welcome.

+1 for adding to core

After a little usage of that patch/module, I agree with Dries. Сomment headers become unintelligible.

I think, this patch may be in core, but users need only provide the opportunity to choose what type of headers will be used by default.

Are partial sentences from the first few words of someone's post "intelligible"?

Also, most forums use "Re: " in default subjects. It's a common thing, it makes sense, and (IMO) it's a lot less ugly than some random slice from the beginning of a post.

Include in the core

I'm not sure this needs to be in core, but it'll always be installed on my site.

I always wanted to have threaded comments and comment titles in other CMSs. When I reached Drupal (I'll never regret that day) I still loved threaded comments, but was not sure about comment titles/subjects. I found using the first words in a comment as a comment title quite odd. Until I found this module (I'll never regret that day).

To my eyes, there's no way "Re: Not enough data" is less meaningful than say, "The patch isn't there, can" or "Thank you very much for your".

I wonder whether that last remark is true, because the way e-mails are displayed and used is quite different to comments on a blog, etc. E-mails are scattered in 1 interface, often unrelated to each other, while comments all have the same context: the current post & it's replies. Therefore, in the e-mail case it's wise to have some indicator which refers to the previous e-mail, such as the "Re: "-syntax. There is less of a need for that syntax in the case of comments, in fact, using subjects as small summaries of the comment can be useful.

Never-the-less, there are still cases (blocks, overviews) where using subjects to reference the parent IS a good idea. So the need to be able to choose the "Re: "-syntax is still present. But at least we still have the module to give us what we want, when we want it.

have you considered these issues:
#318438: Support for other kind of default subjects
#318443: Integration with token (new branch request)

?

now comment_subject has a 6.x-2.x branch to experiment usability with other kinds of default subjects, please review it and give feedback
even about D6 or what should apply for D7 as well (as a contrib module)

If someone feels new version should fight for D8, leave your feedback http://drupal.org/project/issues/comment_subject

@Dries: We have "reply" and "add new comment" - it make sense to me to identify replies as replies rather than new comments, especially if using the "flat" display. On a very active site that I developed, I find that most users forget to enter a subject at all. I believe that a pre-filled, but overrideable subject would work much better.

@arhak: As soon as D8 is open for patches, I would suggest updating to the latest HEAD and re-submitting the patch.

+1

there's initial patch & still needs tests to be accepted to core