If you install the module and visit the admin settings form, the enabled node types will default just to the "Article" node type. This default is provided via comment_notify_variable_registry_get(). If you look where that variable is actually used (variable: comment_notify_node_types), if it's not set, it defaults to all node types; which is good.
So, there are two problems:
1. The admin settings show a default that isn't true. By default, all nodes should be enabled, so all of them should be checked.
2. Not all functions are using comment_notify_variable_registry_get() to fetch variable values (see comment_notify_form_comment_form_alter().
I recommend using the same method of fetching variables across the module, and also changing the default in comment_notify_variable_registry_get() to be all available node types, so the admins are not deceived.
Comment | File | Size | Author |
---|---|---|---|
#6 | 1892658-comment_notify-node_type_default-6.patch | 3.2 KB | hefox |
#5 | 1892658-comment_notify-node_type_default-5.patch | 2.67 KB | hefox |
#2 | 1892658-2-same-method-all-variables.patch | 2.66 KB | kscheirer |
Comments
Comment #1
gregglesHappy to review a patch.
Comment #2
kscheirerThe attached patch fixes up the variables to all go through comment_notify_variable_registry_get(), good call. I'm not sure about defaulting to all content types though. The settings page should be accurate now, I think just article is a sensible default.
Comment #4
kscheirerHmm, this passes with flying colors locally - can you point me in the right direction?
Comment #5
hefox CreditAttribution: hefox commentedBeing that currently if not configured, all node types are enabled, changing the patch to reflect that, else likely need to add an update hook/will cause regressions
Comment #6
hefox CreditAttribution: hefox commentedMissing includes
Note module_load_include('inc', 'comment_notify', 'comment_notify'); could likely be simplified to module_load_include('inc', 'comment_notify');