Using the variable (http://www.drupal.org/project/variable) module within comment_notify would allow you to:

  • Reduce the size of the code using built-in function of the variable module
  • Allow users to change the mail's subjects
  • Internationalization using the i18n module

I will provide a patch to include variable declaration in the module...

Files: 
CommentFileSizeAuthor
#15 comment_notify-translation-steps-1201326-15.patch1.8 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 46 pass(es).
[ View ]
#3 variable_declaration-1201326-3.patch26.66 KBguillaumev
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch variable_declaration-1201326-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 variable_declaration-1201326-2.patch26.64 KBguillaumev
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch variable_declaration-1201326-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 variable_declaration-1201326-1.patch19.01 KBguillaumev
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch variable_declaration-1201326-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

StatusFileSize
new19.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch variable_declaration-1201326-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The patch was tested but obviously some more testing is welcome :-)

StatusFileSize
new26.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch variable_declaration-1201326-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This new patch includes the changes in patch 1 and changes in patch #47 of issue 952072

StatusFileSize
new26.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch variable_declaration-1201326-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Forgot to delete old variables in .install file...

Status:Active» Needs review

I'm concerned about adding a dependency on this since it is not very popular and would make the installation process more complex for the average user.

That said, I appreciate the benefits you laid out and of course am happy to reduce code.

Yeah this can and should be done without the dependency.

That would remove one of the main reasons to do it: reducing code.

Hmmmm :/

Hi,

Is adding a dependency really making the process more complex for the average user ? I think if someone is capable of installing the comment_notify module, he/she should be capable of installing a dependency...

Also remember that on any multilingual Drupal 7 website, you will have variable available, since it is a requirement of the i18n module...

Let's compare the installation instructions:

1. Download comment_notify
2. Untar it
3. Copy it to the right place
4. Read the installation instructions
5. Enable it

1. Download comment_notify
2. Untar it
3. Copy it to the right place
4. Read the installation instructions
5. Download something else
6. Untar it
7. Copy it to the right place
8. Read the installation instructions
9. Enable both

Yes, quite a bit more complex and I'm leaving out a lot of steps. I'm sure installing a module including dependencies is easy for you, but I'm concerned about keeping this module "lightweight" (which is one of the stated goals) and adding a dependency reduces the lightweight nature.

Up to you, you're the module maintainer... :-)

But I'm pretty sure there are other users of comment_notify who would like to change the mail's subjects and have i18n integrated...

Status:Needs review» Needs work

The last submitted patch, variable_declaration-1201326-3.patch, failed testing.

Status:Needs work» Closed (works as designed)

Marking "works as designed" until/unless this can be done as Dave says:

Yeah this can and should be done without the dependency.

I agree it would be awesome to let people modify the mail subject and have i18n more integrated, but don't want to add a dependency.

How about using hook_variable_info() as described at http://drupal.org/node/1113374?

Doh! hook_variable_info() is part of variable module. Please ignore this.

I have seen that the template is passed through t() at the following piece of the module's source code:

  $raw_values = array(
        'subject' => comment_notify_variable_registry_get('watcher_subject'),
        'body'  => comment_notify_variable_registry_get('comment_notify_default_mailtext'), //JS @todo:change this var name.
      );
      foreach ($raw_values as $k => $v) {
        $message[$k] = token_replace(t($v), array('comment' => $comment, 'comment-subscribed' => $alert), array('sanitize' => FALSE));
      }

So I guess that the approach is to send a test email so the template gets into the core translation system so then it can be translated using the translation interface. Is this correct? If so, adding a mention to this at the INSTALL.txt or at the module's documentation page would be helpful for module users.

Status:Closed (works as designed)» Active

If there is some way for comment_notify to work even with variable disabled and have added functionality for people who use the variable module then that seems great to me.

So I guess that the approach is to send a test email so the template gets into the core translation system so then it can be translated using the translation interface. Is this correct? If so, adding a mention to this at the INSTALL.txt or at the module's documentation page would be helpful for module users.

Yes, that's correct and I'd love a patch to do that.

Status:Active» Needs review
StatusFileSize
new1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 46 pass(es).
[ View ]

Here it is.

Maybe it is simply easier to add the default templates to the Translation system automatically.

Is it OK to use the translation API to see if the default templates exist and, if not (and Locale module is enabled), add them? If so, let me know and I will work out a patch.

Title:Variables should be declared for translationExplain how to translate comment notify
Status:Needs review» Fixed

Status:Fixed» Closed (fixed)

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