Page MenuHomePhabricator

Audit inline comments doesn't account for synthetic authors
Closed, ResolvedPublic

Description

The PhabricatorAuditInlineComment class is implementing PhabricatorInlineCommentInterface interface, which tells, that it supports synthetic authors (e.g. ones, that are used in Diffusion for showing lint messages in form of inline comments). However the getMarkupFieldKey method is generating markup key based on inline comment id, which is absent in case if comment is added by synthetic author. At the end all inline comments from synthetic author are rendered with same content (because markup field key is the same).

I recommend changing

public function getMarkupFieldKey($field) {
  return 'AI:'.$this->getID();
}

to

public function getMarkupFieldKey($field) {
  // We can't use ID because synthetic comments don't have it.
  return 'AI:'.PhabricatorHash::digest($this->getContent());
}

As it is done for Differential Diff Inline Comments.

Event Timeline

aik099 raised the priority of this task from to Needs Triage.
aik099 updated the task description. (Show Details)
aik099 added a project: Audit.
aik099 added a subscriber: aik099.

This report is probably correct and the remedy is likely also correct, but getting lint into Diffusion is completely in shambles so I'm actually not immediately sure how to reproduce it, at least in a reasonable / moderately-convenient way.

The way coverage and lint are stored needs a rewrite (referenced in T9524). And MarkupInterface is likely to get removed at some point anyway. I'm just going to put this into a "Future" column for now.

In https://github.com/aik099/phabricator/commits/jenkins-integration branch I've:

  • collecting lint messages as part of build process
  • storing them in new commit_property table (similar to differential implementation at the time)
  • when rendering inline comments adding fake inline comments with lint messages

If you'd like you can see implementation in there for a possible reproduction recipe.

epriestley claimed this task.

Mooted by changes in T13513.