Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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');