Generate comment subjects from parent subject (re:...)
ahoeben - April 23, 2007 - 20:58
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | won't fix |
Description
"Technically speaking the subject of most comments will be the post itself. So the comment subject line often ends up being something like, "Agreed" or "Another thought", which doesn't really mean much. However the comment subject line is used in the comment block and several other places in Drupal where comments are listed. If it is not enabled, Drupal will use the first few words of the post as the subject line." (from a Lullabot article).
A module exists that generates comment subjects from parent comments. The attached patch makes this functionality an option in comment.module.
| Attachment | Size |
|---|---|
| comment_subject_re.patch | 2.25 KB |
#1
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.
#2
I agree. Please make this into drupal core.
#3
no longer applies
#4
rerolled patch (though it does not seem too different)
#5
+1 for adding this feature to the core
#6
+1 for adding to core
#7
#8
Sounds good.
+1 for core
#9
+1 This should definitely be a core feature. As I think it's just the natural way of subject titles.
#10
+1 Don't understand why this feature is not in core
#11
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.
#12
New patch against the status quo.
#13
keeping up with CVS
#14
+1, and it has a recent patch. Come on...
#15
Please add this to 6, it seems "common sense" for inclusion.
#16
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.
#17
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.
#18
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
#19
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.
#20
I am not mixing myself in the 6.x or 7.x argument, but here's a rerolled patch against the status-quo (6.x RC1-ish) and with improved wording on the preference page.
#21
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.
#22
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.
#23
Just wanted to say that I would find it nice to have this within the core
#24
Subscribing.
#25
Rerolled against current status quo. Slightly improved wording.
#26
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?
#27
I will try to find out how to do that using tortoisecvs.
#25 is against revision 1.617, which is currently HEAD.
#28
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
Thanks for testing the patch. Turns out there was some 'rot' in my code, sorry 'bout that. Here's a new patch that underwent some more testing on an actual install than the previous one(s).
I have hand-edited the patch to point to the proper path to the modified file, hope it works this way.
#30
mariuss, camus, xjm, rays, alexandreracine, crimson, cyberpunk, codepoet, edoc, l8a,
If you really want this in core, now's a good time to review this patch... Even though the patch is against HEAD, it applies fully to Drupal 6.0 so it is relatively easy to test this against your current (dev-)site. The longer you wait, the more effort it will cost to test this (because in time you will likely need to set up a new HEAD-based site to test the patch).
#31
#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.
#32
Thanks, good catch. Just in case that last empty line causes someone to think this patch does not apply cleanly, here's a rerolled patch.
Some more tested scenarios: editing a comment and clearing the subject line (leaving the subject empty). In the latter scenario, the first couple of words are taken from the comment body, like it used to be.
Tested with and without comment subject field enabled.
So what does it take to get this RTBC?
#33
+1 for core
#34
Phosphoric, please test the patch, so I can mark it 'reviewed & tested by the community'.
#35
I'm new, so do you replace the code in comment.module or add that code to comment.module?
#36
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.
#37
Update against HEAD
#38
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.
#39
Thanks for looking at the patch. Here's a new one:
* capitalised comment
* ended comment with period
* removed question mark to improve description.
#40
#41
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.
#42
+1
#43
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.
#44
Please make this part of drupal core.
#45
@dmage: I suggest you test and review the patch aswell, because the more people test it, the sooner it gets into core.
#46
@43, not syncing the titles is a good thing IMO.
#47
That's really needed...
#48
@47:
Please test the patch and report back here, otherwise it will never get into core.
#49
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.
#50
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.
#51
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
#52
Thanks for updating the patch to head.
The original intend of the patch was to have comment subjects behave like email discussions. Your email client is smart enough NOT to add nested/chained "Re:" prefixes; in long discussions you would end up with comment titles that only exist of "Re: Re: Re: Re:" etc.
Could you explain why you feel they should be chained? As far as I can think of, the purpose it would serve is to show nesting of the comment. However, the way comments are graphically presented provides a much better cue for that nesting.
#53
-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.#54
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 :)
#55
+1 for core
#56
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).
#57
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.
#58
About the "Set 'Re: [parent title]' as comment title by default. " suggestion; I personally don't like the [] pseudo code as an explanation. Perhapse something more like "Suggest email-like comment titles."?
#59
+1 for core - using it now on my 5.7 site.
#60
I tested patch #57 against HEAD and it works like a charm.
Nice addition to the comment functionality!
#61
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
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.
#62
@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.
#63
@macgirvin: the text you have an issue with is outside the scope of this patch. It is the current text for a very different option to show or hide the form for a comment subject. The patch we're talking about here puts a default comment into that field.
@flobruit: even if the comment subject is not shown, the option to use a re:-style subject is still relevant; even if the subject field is disabled, a comment subject is still stored in the database (and shown in blocks and possibly other places too), and some people will want this stored comment to be in the re:-style of this patch. Hiding the option when the subject form is disabled is not a good idea perse.
See this issue for the original comment_subject module.
#64
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.
#65
Rerolled patch against current status quo, with slightly different wording.
#66
Seems like a worthwhile functionality.
Patch looks generally good. I didn't test.
Points for improvement:
*
$subject = t('Re:') . " $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);+
#67
+1 from me, if that's still an issue (update the module description if not)
#68
rerolled and slightly modified
@nedjo:
* The reason I use t('Re:') instead of using a placeholder for $subject is that I use the same localised string to test if the parent subject does not already start with 'Re:'. If I would use t('Re: !subject', array('!subject' => $subject)), there would be two strings that need translation.
* I think I got all code standard issues
* Added description and code comment.
@codepoet:
slightly modified the wording on the module page to say that the patch needs testing (instead of +1s)
Pending feedback on t('Re:') . ' ' . $subject vs t('Re: !subject', array('!subject' => $subject)) this is still 'needs work', but this is a very minor issue. So please review!
#69
what if you used a RTL language?
#70
I have no idea. How do email applications handle RTL languages?
#71
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.
#72
I get
whee replying to comments with this patch.
Robin
#73
+1 for core functionality
#74
@Robin Monks:
Are you sure that is caused by my patch? The line that is giving you the error is part of comment_save, which has very little to do with the issue at hand. Recently comment.module (specifically the comment_save call) was updated for a patch for the new database layer.
Did you just get the latest version of comment.module, or did you get a full cvs checkout of Drupal?
#75
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
#76
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.
#77
@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.
#78
@Dries: One could argue that comment subjects don't make sense in an online discussion forum (see the quote from that lullabot article in the opening post). Yet we still have them in comment.module. This patch uses a known 'pattern' to (optionally) make the comment subject line behave more like people have come to expect from email.
Having said that, thanks for looking at the issue so quickly after it went 'the-state-formerly-called-RTBC'.
#79
+1 for adding to core
#80
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.
#81
> Сomment headers become unintelligible.
Excuse me for staying a proponent of this patch, but do you find subjects in email messages unintelligible too?
Like your email application, this patch prevents 'Re: Re: Re: ...' type subjects from happening. Does the addition of one 'Re:' make a subject unintelligible? Hardly.
#82
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.
#83
I know that Dries shot this patch down, but here's a reroll against head anyway. Also includes a fix for parent subjects that are too long (#302025: Subject cannot be longer than 64 characters).