i'm going to leave this hanging in the 6.x queue for now, because:

now that issue followups are comments, we have the option to display the comment form below the last followup. this is tremendously handy, and given the sometimes slow page loads on d.o, would probably be a productiviy boost, because it saves a page load per issue reply.

however, the comment setting for the form location is global, and on drupal.org, the comment form is set to display on a separate page... :(

attached patch adds a simple wrapper function to comment module, which allows modules to control the location of the form per node. in order to activate per-node control, simply drop the correct setting into $node->comment_form_location (most probably done in a hook_load). if that's not present, then the fallback is the global setting, so this has no impact at all on current functionality.

i've searched core (but not contrib), and nothing else even looks at that variable setting besides comment module, so the change is totally self-contained.

if we can't slip this in for 6, i'd still like to be able to run this as a patch on drupal.org, at the very least.

CommentFileSizeAuthor
#30 comment.admin_.js__0.txt656 byteshunmonk
#29 comment_settings_js_candy_0.patch5.14 KBhunmonk
#27 comment.admin_.js_.txt645 byteshunmonk
#26 comment_settings_js_candy.patch5.14 KBhunmonk
#21 drupal-HEAD.comment-settings-per-type.patch31.22 KBsun
#12 comment_settings_by_node_type_0.patch31.84 KBhunmonk
#9 comment_settings_by_node_type.patch29.97 KBhunmonk
#2 comment_form_location3.patch3.63 KBhunmonk
#1 comment_form_location2.patch3.76 KBhunmonk
comment_form_location.patch2.53 KBhunmonk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
FileSize
comment_form_location2.patch3.76 KB

here's a slightly more integrated approach. we may not want that additional code in node_load() -- if not then the original patch would be preferable.

i searched contrib and found one module that called variable_get('comment_form_location', ... -- Node comments. hopefully robert can weigh in on the possible impact of this idea.

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
FileSize
comment_form_location3.patch3.63 KB

ok, this is a _much_ cleaner approach than the first two -- this puts the entire workflow in comment module where it belongs :)

i was originally hesitant to set $node->comment_form_location in comment_nodeapi(), because any modules trying to override it in hook_load() would have their value overwritten. but, i realized that we could simply make comment module respect any existing value of $node->comment_form_location prior to setting it.

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented

the more i think about this, the more it seems that we'd want to solve this comprehensively if we're going to do it -- ie, embed all relevant comment settings in the node object, perhaps in a $node->comment_settings array. thoughts?

Dries’s picture

Dries CreditAttribution: Dries commented

I'm cool with letting this slip into Drupal 6 in favor of improving drupal.org. It's well worth the exception.

Instead of the proposed solution, I'd consider making this a setting on the node type configuration page. Can we try that approach? It looks like the better solution to me.

bennybobw’s picture

bennybobw CreditAttribution: bennybobw commented

subscribing

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented

subscribing.

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
Status: Needs review » Needs work

after talking with dww and goba a bit more about this issue, i think we've decided on an approach:

all global comment settings will be loaded into a $node->comment_settings array, which will respect any existing comment settings that have already been loaded into the node. this will allow programatic control of comment settings (which will solve many, many problems for project issue module), and will easily allow us to move these to per-node-type admin settings in the future if we choose to do so, without surrendering or greatly altering the system detailed above.

now i just need to write the patch... :)

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented

well, i spent some time trying to write this patch according to the plan above, and i'm not sure sure it's going to work... :(

the problem is that there are circumstances where it's inefficient to have the fully loaded node object available to do this (think tracker page). if we go with dries' original idea, per-node-type settings, then it can be done, it's just a bit more work.

i'll try that approach next...

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
Status: Needs work » Needs review
FileSize
comment_settings_by_node_type.patch29.97 KB

it actually wasn't as bad as i thought it would be... ;)

here's what has changed:

  1. global comment settings are gone -- they are all per node type, and are edited when you edit a content type. for organizational purposes, i made a separate 'Comment settings' fieldset on the content type edit form
  2. since the global settings are gone, i rearranged the menu at admin/content/comment to eliminate the first layer of tabs -- the main tabs are now the list and approval queue for comments. i actually like this a lot better -- i've always found the location of the comment settings to be a bit out of place there anyways.

i tested things a bit, and they seem to work beautifully.

outstanding issues i know of:

  1. if somebody edits the 'type' field on the content type edit page, i think it's going to screw up the settings, because the variable name will have changed. of course this is also true for the original 'comment_'. $node->type setting that was there already. not exactly sure how to handle this yet
  2. we need an upgrade path for the current global settings. this should be pretty easy to accomplish -- load up the previous global settings, load up the existing node types, copy the values over, delete the old global settings.

setting to review so i can get some initial feedback on what i have here.

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
Title: Allow modules to set the location of the comment form. » make comment settings per node type
Priority: Normal » Critical

changing title to reflect the new direction.

DrupalTestbedBot tested hunmonk's patch, the patch passed. For more information visit http://testing.drupal.org/node/93

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
FileSize
comment_settings_by_node_type_0.patch31.84 KB

attached handles both of the outstanding concerns:

  1. looks like the node module handles updating variables when the node type changes (although it's a bit sloppy -- see http://drupal.org/node/181195 for the fix). it doesn't handle cleaning up variables when a node type is deleted, however -- so i've added support for that (which will work as soon as the breakage reported at http://drupal.org/node/116200 is fixed)
  2. upgrade path is done as described above. i tested it and it works perfectly.
  3. also cleaned up a section that was offending my coding sensibilities, by calling a variable_get() about four times in a row instead of storing the value in a variable... :)
JirkaRybka’s picture

JirkaRybka CreditAttribution: JirkaRybka commented

If we're going to have comment settings per node type, my big +1 for that.

I would like to point a few related issues though, to consider how much it affects the solution, and whether it would be sensible to include some features from these into the present-time comments clean-up:

http://drupal.org/node/175841 - Proposed to remove comment controls from end-user facing UI completely. The point is, that users should not be able to change comment display (flat/threaded and the like), because it would mean shoot-yourself-in-the-foot case, making the discussion unreadable by changing the order and so breaking question/answer relations. If we're going to change settings per node type, then it makes even more sense.

http://drupal.org/node/55277 - One of things currently broken with flat comments mode: Author-edited posts unintentionally changing location in thread. If followups are going to be comments, *and* if they are going to stay in flat mode, then this bug needs to be fixed.

http://drupal.org/node/81373 - Another one about a bug with flat mode. The point here is, that if user see comments flat, and submit replies as such, there should be no threading saved to the database. Having a hidden threading info in DB (built rather randomly from *which one* of visually identical reply-links the user clicked) leads to some ugly effects, especially allowing/denying users to edit their comments without any visible logic, and making the comments almost impossible to maintain, because deleting one comment blows unexpectedly some hardly predictable set of other comments, due to hidden threading in DB.

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented

@JirkaRybka: while i understand that these issues should probably be fixed, none of them have a direct bearing on this patch.

let's avoid scope creep here, and stay focused on the change at hand.

JirkaRybka’s picture

JirkaRybka CreditAttribution: JirkaRybka commented

OK, just thought You might be interested in that too, depending on Your use case. As for this patch, it's almost unrelated of course.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

subscribing for later review

Dries’s picture

Dries CreditAttribution: Dries commented

I did a code review of hunmonk's patch and it looks good. Crazy as it may sound, I've no comments or remarks. ;)

Note that I didn't test this patch yet.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Needs work

I've now tested the patch and it looks good except for the nested fieldsets on the node type configuration page. No nested fieldsets please!

Moving the comment settings to the node type configuration screen actually makes for a usability improvement IMO.

If we remove the nested fieldsets (should put all the settings in one fieldset, and maybe use AJAX to hide them if comments are disabled), then this patch is ready to be committed.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

subscribing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

- Anonymous commenting options are enabled although anonymous users are not allowed to post comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal-HEAD.comment-settings-per-type.patch31.22 KB

- Fixed 'Anonymous commenting options' of #20
- Changed comments fieldset in content type settings as suggested by Dries in #18 (however, without JS auto-hide - do we have some core/default way to do such stuff in D6?)

Dries’s picture

Dries CreditAttribution: Dries commented

(The auto-hide is not important, it can be a separate patch or no patch at all.)

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
Status: Needs review » Reviewed & tested by the community

latest patch looks good. tested exhaustively -- all comment settings are working, and respected per node type. i'd already tested the upgrade path before, so i know that's good.

after this lands, i can roll a quick patch to add the js candy.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I can also confirm the upgrade path is working.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks all. :)

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
Status: Fixed » Needs review
FileSize
comment_settings_js_candy.patch5.14 KB

here's the patch to add the jQuery hide/show. js file to follow...

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
FileSize
comment.admin_.js_.txt645 bytes

discussions w/ kkaefer led me to put this in a separate js file, and title as such -- it goes in /modules/comment

anders.fajerson’s picture

anders.fajerson CreditAttribution: anders.fajerson commented
Status: Needs review » Needs work

Tested and works as expected. Some minor stuff:

1. <div id=comment-advanced-settings> should be <div id="comment-advanced-settings">

2. // $Id: is missing from the new file

3. There is a minor jump when when the advanced settings are collapsed/expanded, some kind of CSS issue.

I think it's a nice touch that the settings are slided instead of just show/hide. Although I don't think fieldset within fieldset is an option we should be aware of that we miss out on the fancy "slide in place" behaviour of fieldsets.

Maybe not part of this follow-up patch but I think we should consider collapsing the collapsible fieldsets on this page, especially since it is quite common that contrib modules adds settings to this page.

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
FileSize
comment_settings_js_candy_0.patch5.14 KB

attached fixes the missing quotes. corrected js file to follow...

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
Status: Needs work » Needs review
FileSize
comment.admin_.js__0.txt656 bytes

js file w/ ID tag. i think if we can get a jQuery guru to give this a last lookover, we'll be good to go.

kkaefer’s picture

kkaefer CreditAttribution: kkaefer commented

The JavaScript looks good to go.

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
Status: Needs review » Reviewed & tested by the community

the rest of this patch is pretty straightfoward. now that we've got a js blessing, i think it's ready.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Needs review

I applied and tested the patch, and all look fine. The only thing stopped me from committing the patch is that I don't know of a JS naming convention. I looked up all modules in core, and they don't seem to have JS files named with multiple components. It is evident that we have module.components.inc and such for PHP files, and we use dashes for CSS and theme files. But there is only one precedent I know of in core with multi component file name and it is jquery.form.js in misc. Although I remember Dries suggesting that we should rename it to use a dash, but we committed it as-is to follow the jQuery naming convention. So we need to decide on this here as it seems.

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented

for consistency with the one other multi-name js file we have in core, i'd vote for comment.admin.js

DrupalTestbedBot tested hunmonk's patch (), the patch failed. For more information visit http://testing.drupal.org/node/95

DrupalTestbedBot tested hunmonk's patch (http://drupal.org/files/issues/comment_settings_js_candy_0.patch), the patch passed. For more information visit http://testing.drupal.org/node/104

DrupalTestbedBot tested hunmonk's patch (http://drupal.org/files/issues/comment.admin_.js__0.txt), the patch failed. For more information visit http://testing.drupal.org/node/105

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 6.x-dev » 7.x-dev
Priority: Critical » Minor

Now http://drupal.org/node/182950 has been committed, I suggest we postpone further usability improvements to Drupal 7.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Version: 7.x-dev » 6.x-dev
Category: feature » bug
Status: Needs review » Needs work

I eventually found this when checking what had happened of http://drupal.org/node/171782, and IMHO the change, while very convenient overall, and valuable developerwise, carries a significant usability problem for the more general set of drupal webmasters: with the central comment mode setting gone, all types default to an in-module hardcoded value instead of being configurable at the site level as they were in previous versions and until late into the after-code-freeze D6 cycle.

The problem exists for all comment parameters with their own respective defaults: comment, comment_default_mode, comment_default_order, comments_default_per_page, comment_controls.

For this reason, while the availability of per-node-type comment formats is indeed to be kept, I think the removal of the central default setting should be rolled back, and the defaults in comment_form_alter be taken from this central setting, just like they were previously, instead of being hardcoded in the module.

Since this part of the change looks like a UI bug to me, I'm resetting to bug and D6 pending further opinions.

hunmonk’s picture

hunmonk CreditAttribution: hunmonk commented
Status: Needs work » Active

I think the removal of the central default setting should be rolled back, and the defaults in comment_form_alter be taken from this central setting, just like they were previously

this is inaccurate. the old global settings also used hardcoded default values -- the only difference now is that the hardcoded defaults are used per node type, instead of globally. i believe this is in line with how other per-node-type settings work.

while it sounds convenient to have the global overrides for comments, i think there's a good chance it could be more confusing than the setup we have now. are there any other per-node-type settings that work this way? if not, then we'd be creating an inconsistency in the admin intervace.

IMO, the real problem is that our admin interface does not allow you to view and change settings _across_ node types (ie, show me the comment settings for all node types on one page)

another approach would be to add a global settings page for all node type settings (similar to the global theme settings). i don't know how useful that would be for day to day site maintenance, but it would certainly make initial site setup easier.

since this is more of a discussion at this point, setting status appropriately.

JirkaRybka’s picture

JirkaRybka CreditAttribution: JirkaRybka commented

+1 to introducing some centralized settings page. While tuning the config, I really hate to pursue each setting over multiple per-node-type pages, and redo the same change everywhere, being never 100% sure that I didn't miss something. Being able to reset all node types to the same setting, or at least to change them all together, would be nice IMO.

But this leads us into D7 land, I'm afraid.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Yeah a big +1 to a centralised node settings page (possibly in addition to the per-node page but with same information). More and more things are set per node type, and there's more and more node types, so could do with something like that to simplify changing things all at once. There's also been whispers about module/hook registries to cut down on nodeapi calls etc. - which could again mean even more things set per node type (maybe).

mburak’s picture

mburak CreditAttribution: mburak commented

Is there any similar patch for Drupal 5?

walker2238’s picture

walker2238 CreditAttribution: walker2238 commented

Yeah, I'm with mburak. A path for version 5 would be great.

walker2238’s picture

walker2238 CreditAttribution: walker2238 commented

Hey mburak I created a simple conditional statement to simulate this feature for drupal 5.
http://drupal.org/node/303731
It works for the basic needs. Let me know if you know of any better ways.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Minor » Normal
Status: Active » Fixed

This issue is very old and was originally fixed in 2007 - so I'm marking it back to fixed, and please open new issues for any followup patches.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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