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

guillaumev’s picture

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 :-)

guillaumev’s picture

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

guillaumev’s picture

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...

greggles’s picture

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.

Dave Reid’s picture

Yeah this can and should be done without the dependency.

greggles’s picture

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

Hmmmm :/

guillaumev’s picture

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...

greggles’s picture

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.

guillaumev’s picture

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.

greggles’s picture

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.

juampy’s picture

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

juampy’s picture

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.

greggles’s picture

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.

juampy’s picture

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

Here it is.

juampy’s picture

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.

greggles’s picture

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

Status:Fixed» Closed (fixed)

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

Pafla’s picture

Issue summary:View changes

Hi

If i do this https://www.drupal.org/files/comment_notify-translation-steps-1201326-15...

I can translate:
1 subject can be translated
1 body can be translated
But there are 2 subject & 2 body

How to fix that?

Pafla

drupalfan81’s picture

Thanks for the great work! Giving this a try tonight! I agree that having an email with the body translated but not the subject is kind of a deal breaker to send out to users. It would be like a user receiving an email from their english website with the subject line in Japanese and then the body in English. Looks bad.

Pafla’s picture

For not to be mistaken.

Notification for Author:
Subject can be translated
Body can be translated

Notification for commenter:
Subject can NOT be translated
Body can NOT be translated

In "comment_notify.inc" Line 320
$variables['watcher_subject'] = '[site:name] :: new comment on [comment:node:title]';
to
$variables['watcher_subject'] = t('[site:name] :: new comment on [comment:node:title]');
Then Subject for commenter can be translated

For subject I suggest this:
Line 314 "$variables['author_subject'] = t('[site:name] :: new comment for your post.');"
To "$variables['author_subject'] = t('[site:name] :: New comment on [comment:node:title]');"
Line 320 "$variables['watcher_subject'] = '[site:name] :: new comment on [comment:node:title]';"
To "$variables['watcher_subject'] = t('[site:name] :: New comment on [comment:node:title]');"
But still not possible to translate body for commenter

Any solution?

Pafla