display project name on the project issue to link filter

My 2 cents:

My preference would be that the project short name is displayed when referring to an issue that belongs to another project's issue queue. When referring to an issue with module B from an issue in the queue for module A, the link would be "#12345 [B]: Issue Title". For references outside of the issue queues (for example from a handbook page), the short project name could always be displayed.

However, my understanding of the filtering system (used to render the link) is that it does not have context. It formats the issue link, but doesn't know for which node/issue/comment it is doing so. I guess this means the only options are A) never display the short project name, or B) always display the short project name.

Going with B) would mean an additional call to node_load for each rendered link, as we'll need to fetch the project data. It would also mean that the links would grow even longer, especially if the target issue is assigned to a user.

Personally I wouldn't mind seeing this change...

I think this would be really useful for places like http://drupal.org/community-initiatives/git/feature-list where many modules issues are listed.

Cool, thanks for working on this via a patch!

A) If you use the Drupal.org testing distro, you'll get a nice test site with Project* and devel. That would have helped you find the answer to this question:

+  $project_name = 'somevalue'; //FIXME get the right value

;) But, I'll help you out:

+  $project_name = $node->project['uri'];

B) I think it'd be visually more consistent if we went with "#nid [short_name]: title", so these need re-ordering:

+    $title = "[$project_name] #$node->nid-$comment_number: $node->title";

...

+    $title = "[$project_name] #$node->nid: $node->title";

C) Some project short names are really long. :( and in a lot of places, this is just going to be noise (and annoying). i agree with pasqualle in #2 in that i'd much rather see some opt-in syntax as with #367423: Make "assigned to" on issue links opt-in (by appending '@'). Reusing @ as in @P seems wrong. Or maybe he meant that [#367423@P] would result in something like this:

#367423 [project_issue]: Make "assigned to" on issue links opt-in (by appending '@') Assigned to: dww

(since both '@' and 'P' are present, you see both).

To get the output he gave, you'd just write this: [#367423P] which would yield:

#367423 [project_issue]: Make "assigned to" on issue links opt-in (by appending '@')

I'm not sold on 'P', but maybe that's best for this. As a 20+ year UNIX user, '~' comes to mind as the "home" for an issue. So, you could use [#nid~] if you wanted the home listed, [#nid~@] if you wanted both, etc. But that's probably just cryptic and weird to most people. ;) I guess if we're going to use a single character to toggle this, 'P' is most self-documenting for project name.

Of course, adding conditional support for the project name is going to need a bigger patch, since you need to change not just the theme function, but the filter itself in project_issue_filter() and project_issue_link_filter_callback().

After a quick irc chat, dww and I end up having this possibilities:

I am hoping to write a new patch soon.

Here a new version of the patch.

A snapshot about how these look like with a real example based on the possibilities proposed in #5

Looking great! Glad the drupalorg_testing profile is helping you get this working. Hurray for that. ;)

A few minor issues and I think this is real close:

A) Please use proper capitalization and punctuation on code comments, e.g. this:

+    // avoid the project name when not needed since it needs extra queries

Should be this:

+    // Avoid the project name when not needed since it needs extra queries.

B) Given all the combinations now of modifiers, the theme function is getting very complex, with lots of nesting and duplication. For example:

  if (!empty($username) && !$include_assigned) {
    // We have an assigned user, but we're not going to print it next to the
    // issue link, so include it in title. l() runs $attributes through
    // drupal_attributes() which escapes the value.

    // avoid the project name when not needed since it needs extra queries      
    if ($include_project_name) {
      $attributes = array('title' => t('Project: !project_name, Status: !status, Assigned to: !username', array('!project_name' => $project_name, '!status' => project_issue_state($node->project_issue['sid']), '!username' => $username)));
    }
    else {
      $attributes = array('title' => t('Status: !status, Assigned to: !username', array('!status' => project_issue_state($node->project_issue['sid']), '!username' => $username)));
    }
  }
  else {
    // Just the status.
    if ($include_project_name) {
      $attributes = array('title' => t('Project: !project_name, Status: !status', array('!project_name' => $project_name, '!status' => project_issue_state($node->project_issue['sid']))));
    }
    else {
      $attributes = array('title' => t('Status: !status', array('!status' => project_issue_state($node->project_issue['sid']))));
    }
  }

I think it'd be a lot better to build up an $title_strings array one at a time, and then finally implode() the array for the actual $attributes array. Something like this:

  // Figure out what strings to include in the title attribute when you hover
  // over the link.
  $title_strings = array();

  // Since it requires a whole separate node_load(), only include the project
  // name if the user asked for it.
  if ($include_project_name) {
    $title_strings[] = t('Project: !project_name', array('!project_name' => $project_name));
  }

  // Always include the issue status.
  $title_strings[] = t('Status: !status', array('!status' => project_issue_state($node->project_issue['sid'])));

  if (!empty($username) && !$include_assigned) {
    // We have an assigned user, but we're not going to print it next to the
    // issue link, so include it in title. l() runs $attributes through
    // drupal_attributes() which escapes the value.

    $title_strings[] = t('Assigned to: !username', array('!username' => $username));
  }

  // Finally, construct the attributes array for the link.
  $attributes = array('title' => implode(', ', $title_strings));

We should do the same thing for the $title itself, for the same reason. Make sense? I'll leave that for you. ;)

Thanks!
-Derek

p.s. Actually, I'm not sure it makes any sense to include the project name in the title attribute, especially if we're only doing it if it's already being printed out. The title attribute is just some hints about stuff if you want to hover, and in this case, you're not getting any new information. Here, the project name is really just noise in the hover tip. So, the above code block shouldn't really be used, now that I think about it. ;) It's really just here to demo how I think we should handle constructing the actual text of the link itself.

See also #532144: Issue reference filter creates content that isn't always accessible for more on the woes of the title attribute.

Thanks for the feedback.

I tried to simplify the title construction and removed the project name from the tooltip.

the last mentioned issue about accessibility is related, but I think it would be better to patch that independently(aka I am really not sure about what to do, I had a quite similar ask in vote up/down and is still pending).

Perfect!

Now all it needs are SimpleTests... ;)

Thanks!
-Derek

Bad luck for me now I think :-p #877562: PHP 5.3 compatibility for tests

Here the patch with the test.

In the other side, what is the preferred way to get the input filter id after creation? (aka avoid hardcoding the id input format number).

Tagging for the D7 Upgrade sprint; this one should probably go along for the ride.

This isn't a blocker for the D7 port. Untagging.

Oh, how time goes by and things go out of sight!