Port comment_notify to Drupal 8

Is there any activity on a 8.x port currently?

I will look into this, working on https://github.com/md-systems/comment_notify until it reaches a more or less stable state. Then I would love to see it pushed to drupal.org as well.

Changing the issue to keep it understandable even when the issue is presented out of module context.

Just want to say I'm paying attention to this and hope folks will test Arla's code.

Thanks everyone for your interest and work on this issue.

The port is now functional and the test is passing for me. Tests and reviews are much welcome: https://github.com/md-systems/comment_notify

I had the idea to turn both the per-comment and per-user settings into real fields. That seems to be the D8 way. I started on a branch, see https://github.com/md-systems/comment_notify/compare/subscription-field for the general idea. Not completely sure if it's a good idea though.

Thanks for all your work!

I suggest making the port just a straight port and then things like turning settings into fields would be a followup.

Do you want to post a patch?

Here we go

Is that a false positive warning from the testbot or really something to fix?

That's the short array syntax [] which is not supported in PHP 5.3, which is what the testbot runs. Drupal 8 requires PHP 5.4 (or was it 5.5) so that is kind of a fake problem. I think you as a maintainer can edit those parameters somewhere in the project settings. Maybe you have to first create an 8.x-1.x branch, I'm not sure...

Yeah that doesn't work. With an issue version 7.x-1.x it is testing against Drupal 7.
First a maintainer needs to create a D8 branch, a release node, then the version shows up... and a patch can be tested against Drupal 8.
(Possibly additionally the automated test settings need adjustment as arild pointed out.)

I configured the project to use the new drupalci-style system to do php 5.5 and php 5.4 tests on the 7.x branch. So, we should get a fail on 5.4 and a pass on 5.5.

It might require uploading the patch fresh to get the new drupalci to pick it up?

:-) That doesn't work yet. This issue is still assigned to 7.x branch that means it checks out Drupal core 7.x.
You really need to create an 8.x branch, assign this issue to it and then upload the patch (or retest it if it still applies).

OK, ok ok

And let's see if doing this is enough :)

Same patch again

Wohoooo, and the bot starts to spread some love!

Nice start.

Lots of feedback below, we will need to priorize what should be part of a first port and what should be done later. But there are some non-trivial problems to solve sooner or later, e.g. due to the comment-as-field approach that allows to have comments on anything, not just nodes.

1. +++ b/comment_notify.inc
   @@ -22,8 +23,8 @@ function comment_notify_get_user_notification_setting($uid) {
        if ($uid == 0) {
          $users[0] = new stdClass();
   -      $users[0]->comment_notify = comment_notify_variable_registry_get('default_registered_mailalert');
   -      $users[0]->node_notify = comment_notify_variable_registry_get('node_notify_default_mailalert');
   +      $users[0]->comment_notify = \Drupal::config('comment_notify.settings')->get('enable_default.watcher');
   +      $users[0]->node_notify = \Drupal::config('comment_notify.settings')->get('enable_default.entity_author');
   

   Why is this not using the default function below, it's the same code?

2. +++ b/comment_notify.inc
   @@ -188,15 +189,16 @@ function comment_notify_get_notification_type($cid) {
    function comment_notify_get_watchers($nid) {
   -  $cids = db_query("SELECT c.cid FROM {comment} c INNER JOIN {comment_notify} cn ON c.cid = cn.cid LEFT JOIN {users} u ON c.uid = u.uid WHERE c.nid = :nid AND c.status = :status AND cn.notify <> :notify AND (u.uid = 0 OR u.status = 1)", array(
   +  $cids = db_query("SELECT c.cid FROM {comment_field_data} c INNER JOIN {comment_notify} cn ON c.cid = cn.cid LEFT JOIN {users_field_data} u ON c.uid = u.uid WHERE c.entity_id = :nid AND c.status = :status AND cn.notify <> :notify AND (u.uid = 0 OR u.status = 1)", array(
        ':nid' => $nid,
   

   a) Another example of hardcoded assumption on node/nid. This could result in mismatches for comments on e.g. users and having users and nodes with the same ID. We probably need a bigger issue to scan for all occurences of node/nid and make it more generic.

   b) Querying storage tables in 8.x is problematic.. it's actually possible that the storage changes, e.g. because sites force storage to non-translatable.. that might be more likely than something like a MongoDB storage.

   However, it's currently not possible to do anything else, we can't do an entity query on something that's not a field.

3. +++ b/comment_notify.inc
   @@ -231,12 +233,12 @@ function comment_notify_unsubscribe_by_email($mail) {
   -  $uid = db_select('users', 'u')
   -    ->fields('u', array('uid'))
   -    ->condition('mail', $mail)
   +  $uid = db_select('users_field_data', 'u')
   +    ->fields('u', ['uid'])
   +    ->condition('mail', $mail, '=')
        ->execute()
        ->fetchField();
   

   seems like this should be easy to convert to an entity query. not sure about the others here.

4. +++ b/comment_notify.install
   @@ -10,36 +10,23 @@
    
   +  \Drupal::logger('the_shit')->debug((string) $comments_select);
   

   ;)

5. +++ b/comment_notify.install
   @@ -10,36 +10,23 @@
   -  db_update('system')->
   -    fields(array(
   -    'weight' => 10
   -    ))
   -    ->condition('name', 'comment_notify');
   

   don't drop this, use module_set_weight().

6. +++ b/comment_notify.module
   --- [site:name] team
   -function comment_notify_init() {
   -  // Add on every page - they are both very small so it's better to add
   -  // everywhere than force a second file on some pages.
   

   The comment here is lost in the new function, but I'm, not sure it's really true. Aggregation works better in 8.x.

   If we keep this, then this will need cacheability metadata or the second library will be missing/still there if those conditions change.

7. +++ b/comment_notify.module
   @@ -80,34 +39,31 @@ function _comment_notify_options() {
   +  $user = \Drupal::currentUser();
   +  if (!(\Drupal::currentUser()->hasPermission('subscribe to comments') || \Drupal::currentUser()->hasPermission('administer comments'))) {
   

   use $user.

8. +++ b/comment_notify.module
   @@ -80,34 +39,31 @@ function _comment_notify_options() {
   +  /** @var \Drupal\Core\Entity\EntityInterface $commented_entity */
   +  $commented_entity = $form_state->getFormObject()->getEntity()->getCommentedEntity();
   +
      // Only add the checkbox if this is an enabled content type
   -  $node = node_load($form['nid']['#value'] ? $form['nid']['#value'] : $form['nid']['#default_value']);
   -  $enabled_types = variable_get('comment_notify_node_types', drupal_map_assoc(array($node->type)));
   -  if (empty($enabled_types[$node->type])) {
   +  $enabled_types = \Drupal::config('comment_notify.settings')->get('node_types');
   +  if (!in_array($commented_entity->bundle(), $enabled_types)) {
        return;
      }
   

   Mismatch between being generic but the settings below then assume that it is a node.

   As a quickfix, we could support only comments on nodes, I'm nut sure how we want to make it configurable for all.. possibly based on the name of the field or so (full config entity id, e.g. node.article.comment).

   Could also store this as third party settings on comment fields.

9. +++ b/comment_notify.module
   @@ -115,115 +71,57 @@ function comment_notify_form_comment_form_alter(&$form, &$form_state, $form_id)
   +  $form['#entity_builders'][] = '_comment_notify_form_build_comment_entity';
   ...
   +  $entity->notify = $form_state->getValue('notify');
   

   those things usually just add a submit handler that does stuff directly instead of doing it indirectly through CRUD hooks. Could have possible drawbacks, like no longer working through the API like before and something else can't re-act on it anymore like before in that hook.

10. +++ b/comment_notify.module
    @@ -115,115 +71,57 @@ function comment_notify_form_comment_form_alter(&$form, &$form_state, $form_id)
    +function comment_notify_comment_validate(&$form, FormStateInterface $form_state) {
    +  $user = \Drupal::currentUser();
    

    Just a note: Assuming this can only be set through the UI then keeping this as a form-only validation should be OK.

11. +++ b/comment_notify.module
    @@ -259,84 +157,81 @@ function comment_notify_comment_update($comment) {
       module_load_include('inc', 'comment_notify', 'comment_notify');
    

    Long term goal: Get rid of API functions in manually included inc files, make it a service.

12. +++ b/comment_notify.module
    @@ -259,84 +157,81 @@ function comment_notify_comment_update($comment) {
    -  if (!($form_id == 'user_register_form' || $form_id == 'user_profile_form')) {
    -    return;
    -  }
    -  elseif ($form['#user_category'] != 'account') {
    +  if (!($form_id == 'user_register_form' || $form_id == 'user_form')) {
         return;
       }
    

    I think you can catch this more easily through the base form id user_form now (see EntityForm::getBaseFormId())

13. +++ b/comment_notify.module
    @@ -259,84 +157,81 @@ function comment_notify_comment_update($comment) {
       // Only show the node followup UI if the user has permission to create nodes.
       $nodes = FALSE;
       foreach (node_type_get_names() as $type => $name) {
    -    if (node_access('create', $type)) {
    +    if (\Drupal::entityManager()->getAccessControlHandler('node')->createAccess($type)) {
           $nodes = TRUE;
           break;
    

    Another "fun" problem to think about for the comment on non-node problem space.

    Possibly getting all fields with type comment and check create access for their host.

14. +++ b/comment_notify.module
    @@ -352,28 +247,40 @@ function comment_notify_form_alter(&$form, &$form_state, $form_id) {
    +  // Make sure the notify pseudofields are included when the entity is built.
    +  $form['#entity_builders'][] = '_comment_notify_form_build_user_entity';
    

    Same here. See how contact_form_user_form_alter does this now.

15. +++ b/comment_notify.module
    @@ -382,10 +289,8 @@ function comment_notify_user_load($users) {
       // @todo: Why would we want to load this on every user load?
       foreach ($users as &$user) {
    

    good question ;)

16. +++ b/comment_notify.module
    @@ -399,9 +304,9 @@ function comment_notify_user_cancel($edit, $account, $method) {
     function comment_notify_comment_load($comments) {
    

    this will go away if we make it a field I guess so not urgent. but if it stays, we should make it a storage load hook so that we don't need to execute those queries when loading cached comments (same for the user hook if we keep that)

17. +++ b/comment_notify.module
    @@ -444,45 +344,55 @@ function _comment_notify_mailalert($comment) {
       // No mails if this is not an enabled content type.
    -  $enabled_types = variable_get('comment_notify_node_types', array($node->type => TRUE));
    -  if (empty($enabled_types[$node->type])) {
    +  $enabled_types = $config->get('node_types') ?: [$node->getEntityTypeId()];
    +  if (!in_array($node->bundle(), $enabled_types)) {
    

    $node->getEntityTypeId() definitely doesn't make sense :) so that fall back has no test coverage. Might also be easier to understand if we'd explicitly check for an empty array instead of this trick, the result would be the same

18. +++ b/comment_notify.module
    @@ -444,45 +344,55 @@ function _comment_notify_mailalert($comment) {
    -      $message[$k] = token_replace(t($v), array('comment' => $comment), array('sanitize' => FALSE));
    +      $message[$k] = \Drupal::token()->replace(t($v), array('comment' => $comment), array('sanitize' => FALSE));
    

    Drop that t(). That's wrong in 7.x too (user provided strings like variables should be translated using i18n_variable, not t())

    the sanitize option no longer exists. Use PlainTextOutput::renderFromHtml() instead.

19. +++ b/src/Controller/DefaultController.php
    @@ -0,0 +1,25 @@
    +    module_load_include('inc', 'comment_notify', 'comment_notify');
    +    if (comment_notify_unsubscribe_by_hash($hash)) {
    +      return(t('Your comment follow-up notification for this post was disabled. Thanks.'));
    +    }
    +    else {
    +      return(t('Sorry, there was a problem unsubscribing from notifications.'));
    +    }
    

    looks generated from module upgrader, should be cleaned up. coding standard, naming, camel case, ...

20. +++ b/src/Form/CommentNotifySettings.php
    @@ -0,0 +1,172 @@
    +    $this->config('comment_notify.settings')
    +      ->setData($form_state->getValues())
    +      ->save();
    

    this would save all kinds of things like form_id into the config object. It's annoying but you should set() them explicitly.

    Having a test that saves the confirm form would show that this then violates the schema.

21. +++ b/src/Form/CommentNotifyUnsubscribe.php
    @@ -0,0 +1,61 @@
    +class CommentNotifyUnsubscribe extends ConfigFormBase {
    

    this isn't a config form.

Some notes from a quick review... :-)

1. +++ b/comment_notify.inc
   @@ -188,15 +189,16 @@ function comment_notify_get_notification_type($cid) {
   +  $cids = db_query("SELECT c.cid FROM {comment_field_data} c INNER JOIN {comment_notify} cn ON c.cid = cn.cid LEFT JOIN {users_field_data} u ON c.uid = u.uid WHERE c.entity_id = :nid AND c.status = :status AND cn.notify <> :notify AND (u.uid = 0 OR u.status = 1)", array(
   

   The left join is confusing me since i would expect to have the users table and references to it cable.

   Anyway you return $cids here... Is comment_notify here really just contain one line per comment? Otherwise this creates multiple results per comment...

2. +++ b/comment_notify.links.task.yml
   @@ -0,0 +1,9 @@
   +comment_notify.settings:
   ...
   +  base_route: comment_notify.settings
   

   A base pointing to itself? :-)

3. +++ b/comment_notify.module
   @@ -80,34 +39,31 @@ function _comment_notify_options() {
   +  $user = \Drupal::currentUser();
   +  if (!(\Drupal::currentUser()->hasPermission('subscribe to comments') || \Drupal::currentUser()->hasPermission('administer comments'))) {
   

   Use the $user var. ;-)

Check the log messages. There is some explicit remaining one... ;-)

#24:
To keep it simple, I merged the two libraries and added them in-place in the build array. I'm not familiar enough with the aggregation topic to determine if this has a non-trivial effect on performance.

Let's create followups to

• support all entity types as commentable, not only nodes
• switch from custom schema 'comment_notify' to a field on comment
• … or switch from hook_comment_load to storage load hook
• switch from custom schema 'comment_notify_user_settings' to a field on user
• move the "node_types" setting to third_party_settings on bundles
• move procedural code into service
• test coverage

Phew.

#25:
1: Yes, there’s one entry per comment. It states 1) whether that comment author should be notified of followups, and 2) whether earlier watchers have been notified of the comment.
2: Yeah, that’s actually how it works.

Sorry, forgot the interdiff.

Btw, as @Berdir made me aware, the test will not pass because the dependencies in .info.yml are read from the current code, not from the patch. Since we haven't committed the .info.yml yet, there are no dependencies to include. Bah.

Mostly agreed on the follow-ups, but support all entity types as commentable, not only nodes is problematic as it can result in weird bugs with ID overlaps and so on. If we don't fully implement it in a first step then we should probably add some stop-gaps to opt out of anything that's not a node, with a common @todo, so they're easy to find (preferably pointing to an already existing issue).

That said, before we start/decide that: I imagine quite a bit of complexity of that will go away if we do move the "node_types" setting to third_party_settings on bundles first. That will make quite a big part of the code base generic for all entities. So we should definitely work on that first, be it before or after the first commit/merge.

I guess @greggles should make the decision on what of that should be follow-ups and what part of the initial commit or merge. I'd actually recommend to do the second. the patches are useful for reviewing, but it is probably nicer to merge in the separate, smaller commits, assuming they're more or less cleaned up (can also be done in an interactive rebase to improve commit messages and so on before the merge).

+++ b/comment_notify.module
@@ -131,22 +116,17 @@ function comment_notify_comment_publish($comment) {
+ * Additional submit handler for the comment form.
  */
-function comment_notify_comment_update(CommentInterface $comment) {
+function _comment_notify_submit_comment_form(array &$form, FormStateInterface $form_state) {

Maybe we should keep the notification part itself in the hook not sure. I guess we want to trigger notifications if comments are triggered through services/API. So this should be split up.

Yeah. Moved that the mailalert part back to the update hook. Then discovered the insert hook and moved non-mailalert logic into the form submit hook. Since the submit hook is where we actually have the notify/notify_type values, that hook is about saving those values. Then the update/insert/publish hooks only has a conditional mailalert invocation.

We could commit the info.yml file to begin with, to have the patches tested correctly :)

OK, committed for now to get the info.yml fix and so that followups can be more discrete. In the commit message I gave a mention to https://www.drupal.org/u/pjonckiere who had a pull request.

In terms of what should be in this issue vs. followups, I think it would be nice to have test coverage sooner than later, but I defer to the people actually doing the work.

Ok. Given that you committed the full patch, I would say that we open follow-ups for #26 and then work on those tasks separately, see also #28 in which order I'd suggest to do that. @Arla, can you open those follow-ups and then close this?

Okay, I created followups.

Awesome, thanks Arla and thanks to MD Systems!

Just wanted to point out that #2684617: Comment Notify seems to block Create New User on Drupal 8 is an important followup item and #2684153: Invalid tokens as an item that has a workaround but is probably easy to fix in code.