[1.0.x] Comment Notify Node Author

Thank you for applying!

Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,>DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status will not be changed by this application; no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the branch to review and the project name.

To the reviewers

Please read How to review security advisory coverage applications, Application workflow, What to cover in an application review, and Tools to use for reviews.

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications, even to leave a comment similar to the following one. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• It is also preferable to wait before using a CLI tool to report what needs to be changed, especially because the comment left from Code Review Administrators suggests to use PHP_CodeSniffer. Before that, manual reviews should be done.
• Reviewers should not copy-paste the output of a CLI tool. They should use a CLI tool only once per application. When they do that, they should later verify the code has been correctly changed; this means, for example, that adding a documentation comment that is not correct just to avoid to get a warning/error is not a correct change that should be reported in a further comment.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues.

1. Fix phpcs issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml cna

FILE: cna/cna.module
--------------------------------------------------------------------------------
FOUND 78 ERRORS AND 4 WARNINGS AFFECTING 66 LINES
--------------------------------------------------------------------------------
  1 | ERROR   | [x] Missing file doc comment
  4 | ERROR   | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\comment\CommentInterface.
  7 | WARNING | [ ] Format should be "* Implements hook_foo().", "* Implements hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements hook_foo_BAR_ID_bar() for
    |         |     xyz-bar.html.twig.", "* Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.", or "* Implements hook_foo_BAR_ID_bar() for block templates."
  7 | ERROR   | [x] Doc comment short description must end with a full stop
  8 | ERROR   | [x] There must be no blank lines after the function comment
 11 | ERROR   | [x] Opening brace should be on the same line as the declaration
 12 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
 13 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
 14 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 8
 15 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 16 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 17 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 18 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 19 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 20 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 20 | ERROR   | [x] Space found before semicolon; expected "".";" but found ""." ;"
 21 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 21 | ERROR   | [x] TRUE, FALSE and NULL must be uppercase; expected "TRUE" but found "true"
 22 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 22 | WARNING | [ ] Line exceeds 80 characters; contains 82 characters
 22 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 22 | ERROR   | [x] Comments may not appear after statements
 23 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 24 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 25 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 26 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 28 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 29 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 8
 30 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
 34 | WARNING | [ ] Format should be "* Implements hook_foo().", "* Implements hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements hook_foo_BAR_ID_bar() for
    |         |     xyz-bar.html.twig.", "* Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.", or "* Implements hook_foo_BAR_ID_bar() for block templates."
 35 | ERROR   | [ ] Doc comment short description must end with a full stop
 35 | ERROR   | [ ] Doc comment short description must be on a single line, further text should be a separate paragraph
 36 | ERROR   | [x] There must be no blank lines after the function comment
 39 | ERROR   | [x] Opening brace should be on the same line as the declaration
 40 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
 41 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
 42 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 8
 42 | ERROR   | [x] Space found before object operator
 42 | ERROR   | [x] Space found after object operator
 43 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 44 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 45 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 46 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 47 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 48 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 48 | ERROR   | [x] Space found before semicolon; expected "".";" but found ""." ;"
 49 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 49 | ERROR   | [x] TRUE, FALSE and NULL must be uppercase; expected "TRUE" but found "true"
 50 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 50 | WARNING | [ ] Line exceeds 80 characters; contains 82 characters
 50 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 50 | ERROR   | [x] Comments may not appear after statements
 51 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 52 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 53 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 54 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 56 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 57 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 8
 58 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
 65 | ERROR   | [x] Opening brace should be on the same line as the declaration
 66 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
 67 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 8
 68 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 69 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 70 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 72 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 12
 73 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 75 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 12
 76 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 78 | ERROR   | [x] Case breaking statement indented incorrectly; expected 10 spaces, found 12
 79 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
 86 | ERROR   | [x] Opening brace should be on the same line as the declaration
 87 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
 88 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 8
 89 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 8
 90 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 8
 90 | ERROR   | [x] TRUE, FALSE and NULL must be uppercase; expected "NULL" but found "null"
 90 | ERROR   | [x] TRUE, FALSE and NULL must be uppercase; expected "TRUE" but found "true"
 91 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 8
 93 | ERROR   | [x] Array indentation error, expected 10 spaces but found 8
 94 | ERROR   | [x] Array indentation error, expected 10 spaces but found 8
 96 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 76 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

2. FILE: cna.routing.yml

  requirements:
    _permission: "administer cna configuration"

"administer cna configuration" permission is not defined in the module.

Wrong machine name of permission. It should be "administer cna configuration"

administer stop broken link in body:
  title: 'Administer CNA Configuration'
  description: 'Access to configure CNA settings.'

cna.settings:
  path: "/admin/config/system/cna"
  defaults:
    _form: '\Drupal\cna\Form\CNAConfig'
    _title: "Comment notify node author"
  requirements:
    _permission: "administer cna configuration"

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

cna.module

$message['body'][] = t(':message', [':message' => $params['message']]);

That code does not show the translation of $params['message']; it shows it as it is because placeholders in translatable strings are the only part that are not translated. The only way to make the message translatable is to use code similar to the following one.

$message = t("Comment has been updated on %node_title by %username." ['%node_title' => $node_title, '%username' => $username]);

    \Drupal::logger('Comment notify node author')->notice(t('Sent eMail notification for comment with :subject
        to the site administrator eMail address :nodeAuthorEmailAddress', [
          ':nodeAuthorEmailAddress' => $to,
          ':subject' => $subject,
        ]));

The first argument passed to notice() must be a literal string.

/**
 * Implements hook_comment_insert().
 *
 * Responds to insertion of new comment.
 */
function cna_comment_insert(CommentInterface $comment) {

That function is a hook_ENTITY_TYPE_insert() implementation.
There is no need to add a long description to hook implementations.

/**
 * Send mail function.
 *
 * This function handles the process of sending an email message.
 */

The long description is just repeating what the short description already said. The long description is not mandatory, but the parameters and the return value descriptions must be present, if the function has parameters or returns a value.

src/Form/CNAConfig.php

    $this->messenger()
      ->addMessage($this->t('The configuration options have been saved.'));

The parent method already shows a message. It is sufficient to call it.

There are other modules for sending notifications when a node or a comment is created, like the Notify module. At least the project page should give a list of similar modules and describe the difference with them. (A list of notification modules can be obtained by searching for Notify.)

I am changing priority as per Issue priorities.

I am changing priority as per Issue priorities.

What I reported in my previous comment has not been changed.

Furthermore, the following changes are also necessary.

cna.module

$message = "Comment has been posted on " . $node_title . ", by " . $name . ".";

Everything that is shown in the user interface must be translatable. $message is not translatable, or it would not be a concatenation of strings.

$message['body'][] = t('Link to the comment :linkToComment', [':linkToComment' => $params['url']]);

The correct placeholder for URL starts with :. It must also be wrapped on double quotation marks, as reported on FormattableMarkup::placeholderFormat().

:variable: Return value is escaped with \Drupal\Component\Utility\Html::escape() and filtered for dangerous protocols using UrlHelper::stripDangerousProtocols(). Use this when using the "href" attribute, ensuring the attribute value is always wrapped in quotes

    \Drupal::logger('Comment notify node author')->notice(t('Sent eMail notification for comment with :subject
        to the site administrator eMail address :nodeAuthorEmailAddress', [
          ':nodeAuthorEmailAddress' => $to,
          ':subject' => $subject,
        ]));

The channel ID is normally the module machine name. It is not a message title, nor does it contains spaces.
eMail is misspelled. Either it is email or e-mail.
Placeholders starting with : are used when dangerous protocols needs to be filtered out, which is not the case of $subject. In the case of email addresses, see my previous note.

cna.routing.yml

cna.settings:
  path: "/admin/config/system/cna"
  defaults:
    _form: '\Drupal\cna\Form\CNAConfig'
    _title: "Comment notify node author"
  requirements:
    _permission: "administer cna configuration"

It does not seem the title is correct for a settings form.

I have reviewed the code and it looks like the reported issues have been fixed.

However, I've noticed a few small things.

In cna.module, line 10:

use \Drupal\Core\Render\Markup;

Usual convention is to remove the leading `\` in use statements, as you've done in the other use statements above it. However, it doesn't look like Markup type is used at all, so this use statement could be removed altogether.

Next, the if statement on line 18:

if ($comment) {

This hook is only called when a comment is inserted, with the comment as a parameter, so $comment should always have a value, so this if statement is unnecessary and will always evaluate TRUE.

Finally, you use getCommentedEntity() to get the node being commented on. However, this returns a FieldableEntityInterface. However, as not all entities have owners, this interface does not include the getOwner() method. This will cause most IDEs to be unhappy about the following lines (23 and 49):

$to = $node->getOwner()->getEmail();

I think this could be clearer. First, it seems to assume a node, but doesn't explicitly check for this. Casting to a node type could be one way to resolve this. It could also be possible to make this module more generic by allowing for the possibility of other entity types.

I think it would be worth tweaking the code to make it clear what node types are supported, and make sure the variable is of a type that has the getOwner() method.

I like the improvements in cna.module, but I think there are still a couple of issues in both the cna_comment_insert and cna_comment_update functions.

You currently check if the type is a node, which is good, and set the $node_title and $to, if it is a node. However, there is no else condition, so it will continue to attempt the email sending with those variables unset.

For the node title, you could use the label() function instead of the title property. I believe this is recommended practice according to this change record (though it's rather old, so I'm not 100% sure it hasn't changed). label() is generic to all entities, while title is specific to nodes, so you no longer need that in the if statement.

Rather than checking if the entity type is a node, I recommend checking if its an EntityOwnerInterface. That will mean the module will work for any entities with an owner.

If it isn't, it means the entity doesn't have an owner, so there's no one to send an email to, so you should skip the email sending altogether.

You could use something like:

if (!($node instanceof EntityOwnerInterface)) {
  // Entity has no owner, so no need to send email.
  return;
}
$to = $node->getOwner()->getEmail();

I am changing priority as per Issue priorities.

manual review:
cna_comment_update(): $node_title can be undefined when the entity is not a node. Same for the insert hook.

Otherwise looks good to me!

Thanks for your contribution, Gowthamraj!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.