Views field "Comment: Edit link" call to comment_access() always returns false

[alanmels]

Description of the bug

If you use the same test view given in #5882 and just add Comment: Edit link (Edit link) link field as shown below:

then the website logs starts filled in with the reported comments in title.

Additional information

It's not an error or even a notice, but just a comment:

However, still (1) it is not pleasant at all that the log gets quickly flooded with the same comment if your website is actively using the corresponding views page/block. And then (2) this message is caused not by some kind of contributed module, but core itself. See https://github.com/backdrop/backdrop/blob/7cc9968d3c1f8885a9d126668b1a8c0766b8e3d6/core/modules/comment/comment.module#L1508 :

function comment_access($op, Comment $comment = NULL) {
  if ($op == 'edit') {
    // Normalize 'edit' operation to 'update'.
    $op = 'update';
    // Log to watchdog for DX. @todo add watchdog_deprecated_parameter().
    $message = 'The function comment_access() was called with the operation <em>edit</em>. This operation is now named <em>update</em>. The <em>edit</em> operation will be removed in the next major release of Backdrop.';
    $variables = array();
    watchdog('comment', $message, $variables, WATCHDOG_DEPRECATED);
  }

  if ($op == 'create') {
    return Comment::createAccess();
  }
  else {
    return $comment->access($op);
  }
}

I'm not sure how to tackle this issue as there is simply no such operation as Comment: Update link, but there is Comment: Edit link.

[argiepiano]

Hi @alanmels. Thanks much for reporting! I followed the steps:

1. Import the view you posted in Notice: Undefined index: position in views_plugin_display_block->options_form() (line 296 of ../views_plugin_display_block.inc).  #5882
2. Added the Comment: Edit link
3. Saved the View

I don't see anything in the log. Is there a step missing? Do you have to place the block somewhere and then try to access it?

[argiepiano]

OK, I found the issue. views_handler_field_comment_link_edit::render_link() makes a call to comment_access() using the $op edit instead of update. This a minor problem (which is the reason the log shows a notice instead of an error). The function comment_access() takes care of interpreting the $op edit as update. This notice is similar to a deprecation notice. The function still works. The $op edit was used in the past, and at some point it was changed to update.

However, it'd be good to fix this.

[argiepiano]

PR provided: backdrop/backdrop#4270

Please test!

[alanmels]

@argiepiano, you'll see the log if everything is turned on /admin/config/development/logging Thanks for confirming my finding. I know it's just a comment, but since it's flooding the log page would be still nice to get this fixed.

[alanmels]

Thanks for the quick fix. I've tested PR and it works as expected - the log page is no more flooded with the reported comments. It's ready to be committed from my perspective, however one could suggest that once this is being dealt to improve other related issues such as:

• since op is changing should the respective file's name be changed from views_handler_field_comment_link_edit.inc to views_handler_field_comment_link_update.inc?
• should couple more occurrences of edit in the same function be replaced with update?

function render_link($data, $values) {
    parent::render_link($data, $values);
    // ensure user has access to edit this comment.
    $comment = $this->get_value($values);
    if (!comment_access('update', $comment)) {
      return;
    }

    $text = !empty($this->options['text']) ? $this->options['text'] : t('edit');
    unset($this->options['alter']['fragment']);

    if (!empty($this->options['destination'])) {
      $this->options['alter']['query'] = backdrop_get_destination();
    }

    $this->options['alter']['path'] = "comment/" . $comment->cid . "/edit";

    return $text;
  }

[argiepiano]

@alanmels, I believe you are misunderstanding the way comment_access() works. The text shown to the user is edit because that's easiest to understand from the UI point of view, since that's what the link does (it sends you to the edit page for the comment). But the function comment_access() uses the "machine name" update as a blanket operation for anything that's related to saving an already existing entity. In this case, update is the "internal name" for the operation of saving an existing comment after being edited.

So, in my view, it's best not to change the user-facing text, nor the name of the handler. This last one would require many changes in the code in several places.

If this works for you, please mark it so with the "works for me" label.

[alanmels]

Thanks for the explanation. I agree with your arguments. I've just approved backdrop/backdrop#4270 (review), but I don't think I have enough privileges to mark this issue with "works for me" label. At least I don't see such option anywhere on my end.

[argiepiano]

The "works for me" label should be applied here - not in the PR. It's on top of this screen, under "Labels". Marking the PR as "approved" doesn't mean anything in the Backdrop community

[alanmels]

@argiepiano, once again, I do not I have enough privileges to use labels here. Per https://docs.github.com/en/issues/using-labels-and-milestones-to-track-work/managing-labels

Anyone with write access to a repository can create a label.

I do not have write access.

[argiepiano]

@argiepiano, once again, I do not I have enough privileges to use labels here

My apologies

[alanmels]

To quote from https://docs.github.com/en/issues/using-labels-and-milestones-to-track-work/managing-labels#applying-a-label

Anyone with triage access to a repository can apply and dismiss labels.

So I believe I do not have triage access to the repository either. I'm not sure if I should apply for it or if Backdrop gods will just magically give it to me. I'm totally ok without such access, though.

[bugfolder]

Code reviewed. The one-word change looks good to me. Removed the "needs testing" label based on @kiamlaluno's WFM label and previous comments.

[quicksketch]

Thanks folks! A nice easy fix. Sorry this took so long to pull in. I've merged backdrop/backdrop#4270 into 1.x and 1.26.x. Thanks @argiepiano, @alanmels, @bugfolder, and @kiamlaluno!