Add CCK field widget and formatter for issue references

Instead of trying to treat a simple int field as if it were a node reference to an issue, why not just do this:

#243543: nid text field widget?

Allow the issue node reference to just take an int nid as input (which is how it's stored, anyway) and then you get all the power of node references.

Formatting it like #1175704: Add CCK field widget and formatter for issue references seems better to me than formatting it like http://drupal.org/node/648218 -- right?

Well, if it was a real node reference, it'd appear like Add CCK field formatter for issue numbers, not like http://drupal.org/node/1175704. So yeah, I guess this patch could still be useful as a way to format node references that point to issue nodes to add the #xxx and the color-coding, strike-through, etc. But, I'd still strongly encourage #648218: Make API changes in Drupal core be nodes to stores these fields as real node refs, not plain ints. IMHO, the difference between this:

Add CCK field formatter for issue numbers

and:

#1175704: Add CCK field widget and formatter for issue references

is just icing on the cake, not the cake itself.

Good points... So the features we want here are:

a) [required] Widget: enter the NID (rather than entering the node title in an auto-complete or select). So this could be a numeric field or a noderef field...

b) [highly desirable] Field: Have it be a noderef field, so you can use views relationships etc.

c) [highly desirable] Formatter: display the NID and the node title:
#648218: Make API changes in Drupal core be nodes

d) [desirable] Formatter: display the coloring/strikethrough for issue formatting that we're all used to: #648218: Make API changes in Drupal core be nodes

I was just going to say the same, I'd also like to see what kind of widget we could add that would limit this to issue nodes with possible autocomplete (for issues only, would still support nid).

Note that I'm doubly interested in this, since it's something that could be reused by the related issues stuff (even though that's not using CCK).

I'm not as concerned about autocomplete for the issue nodes. I think devs can copy/paste or type in the issue number (and correct it if the preview shows the wrong issue). We already do that when typing [ # 12345 ] in comments.

As far as limiting to particular content types, noderef already does this in its existing widgets, so if that can be adopted, good. (at least it would validate the nid)

Agreed - I don't think we need it to be a noderef for this project. I don't see anything we can't do with an integer field. I'll give this a try and report back shortly, thanks!

I'd really rather see this as a node reference field, you're going to end up validating this data at some point and it sure would make alot more sense to validate it on input rather than on output (it'll confuse alot less people that way).

Here's a first pass at a widget and autocomplete for issues. The autocomplete is very fluid, and takes issue numbers OR titles and uses a more pleasing syntax than the default CCK. The formatter is adapted to work with nodereference instead of integer fields as well.

The autocomplete also includes a generic callback in addition to the one used by the nodereference widget, that could be used elsewhere (I intended it for use with the related issues module, and I could move that callback to that module if necessary).

I'll try to setup a demo of how this works, and I'd love a detailed code review.

This is great!

I did find one bug. I tried to link to issue #1136130: Regression: Reinstate WATCHDOG_* constants and document why they are necessary., which, at least on my test box, has title:
Don't duplicate watchdog level constants
I typed in "Don't dupli..." and it found the issue. So far so good.

But when I went to save the API change node (which has this autocomplete noderef field on it), I got an error:

Issues: title mismatch. Please check your selection.

And in the field, the issue title was showing up as:

#1136130: Don't duplicate watchdog level constants

So it looks like there is an entity double-decoding or double-encoding problem. Other than that, it seems to be working fine...

Ahh, I was reusing the value for the key, which was already run through check_plain. That doesn't work since the key will be used exactly as is. This ever so slightly reworks the logic in the autocomplete function to be slightly more agnostic.

That fixed the problem I identified in #13 - seems to be working fine on my dev site!

This is working very well for us on my demo site. Can we get it committed and deployed to d.o?

Here's a revised patch that moves the generic callback out (and into pi_related module) and cleans up some code comments.

I'd like to here from another project* maintainer before committing this.

Does this patch need testing too, or is it functionally the same?

It'd be good to test this patch, but it mostly just removes some functionality (that wasn't being used by this feature, only in pi_related).

I just deployed the new patch from #17 on http://jhodgdon-drupal.redesign.devdrupal.org/, and it is working fine for auto-complete widget and formatter. The bug I identified in #13 is also still fixed.

As far as I'm concerned, this patch is ready to go (but I haven't reviewed the code).

Just found another bug where it could return more than the designated number of items. Silly me.

Is it easy to fix? Is there something I can do to help? I'd really love to get the API changes thing deployed, but it's waiting on this issue.

This should be better, and has better comments.

The new patch is up and working fine on my test site. Still haven't reviewed the code, just verified that the widget and formatter appear to be working fine.

x-post of some sort?

sorry, I definitely did not mean to change the status!

Subscribe.

Sorry, I don't really know CCK well enough to give a really proper review, but here's some stuff I found.

+++ b/includes/autocomplete.inc
@@ -71,3 +71,59 @@ function project_issue_autocomplete_user_issue_project($uid, $string = '') {
+/**
+ * Autocomplete callback for nodereference widget, returns '#NID: Title' for
+ * both key and value.

+++ b/project_issue.module
@@ -2158,3 +2176,156 @@ function project_issue_project_promote_sandbox($project) {
+ * Additional process function to set validation and autocomplete path for the
+ * project_issue_nodereference_autocomplete widget. This process runs after the
+ * default, nodereference_autocomplete_process().
...
+ * Provide a formatter for nodereference fields that uses
+ * theme_project_issue_issue_link.

<pedantic stupid crap>
The standard is for comments to wrap at 80 chars, and if more than that's required, it's on a separate line.
</pedantic stupid crap>

+++ b/includes/autocomplete.inc
@@ -71,3 +71,59 @@ function project_issue_autocomplete_user_issue_project($uid, $string = '') {
+    $matches["#$nid: ". $title] = "#$nid: ". check_plain($title);

+++ b/project_issue.module
@@ -2158,3 +2176,156 @@ function project_issue_project_promote_sandbox($project) {
+    $value = '#'. $nid .': '. db_result(db_query(db_rewrite_sql('SELECT n.title FROM {node} n WHERE n.nid = %d'), $nid));

Is it worth it to sanitize $nid here?

+++ b/includes/autocomplete.inc
@@ -71,3 +71,59 @@ function project_issue_autocomplete_user_issue_project($uid, $string = '') {
+      $placeholders = db_placeholders($values);
+      $sql = "SELECT n.nid, n.title FROM {node} n WHERE n.status = 1 AND n.type = 'project_issue' AND n.nid NOT IN ({$placeholders}) AND n.title LIKE '%%%s%%'";

Just as a minor style quibble, you might want to put db_placeholders($values) inside the query ala http://api.drupal.org/api/drupal/modules--block--block.module/function/b... so the next person to review this doesn't go OMG! SQL INJECTION?!

+++ b/project_issue.module
@@ -2158,3 +2176,156 @@ function project_issue_project_promote_sandbox($project) {
+  $field_key  = $element['#columns'][0];

<pedantic nonsense>
there's an extra space before =
</pedantic nonsense>

+++ b/project_issue.module
@@ -2158,3 +2176,156 @@ function project_issue_project_promote_sandbox($project) {
+  // Add our custom autocomplete callback
...
+  // Unset the default validate
...
+      // Attempt to validate non-autocomplete syntax

<boring pedantic nonsense>
Comments should end in a period. :P
</boring pedantic nonsense>

+++ b/project_issue.module
@@ -2158,3 +2176,156 @@ function project_issue_project_promote_sandbox($project) {
+function project_issue_nodereference_autocomplete_validate($element, &$form_state) {

This function could use a few more comments to explain what's going on. A lot of that instantiation stuff, for example, is unclear, and wherever there's a regex, a comment is always a good idea. ;)

+++ b/project_issue.module
@@ -2158,3 +2176,156 @@ function project_issue_project_promote_sandbox($project) {
+        form_error($element[$field_key], t('%name: title mismatch. Please check your selection.', array('%name' => t($field['widget']['label']))));
...
+          form_error($element[$field_key], t('%name: the value is not a valid issue id.', array('%name' => t($field['widget']['label']))));
...
+          form_error($element[$field_key], t('%name: the value is not a valid issue title.', array('%name' => t($field['widget']['label']))));

Unless you know something I don't, that looks like an improper use of t(). t() should only go against static strings, because otherwise there's no way to extract translations.

+++ b/project_issue.module
@@ -2158,3 +2176,156 @@ function project_issue_project_promote_sandbox($project) {
+      'label' => 'Issue link styled with status metadata',
...
+      'label' => 'Issue link styled with status metadata and assignee',

Should these be wrapped in t()?

+++ b/project_issue.module
@@ -2158,3 +2176,156 @@ function project_issue_project_promote_sandbox($project) {
+  $project_issue_node = node_load($element['#item']['nid']);
+  if ($project_issue_node->type != "project_issue") {
...
+  $project_issue_node = node_load($element['#item']['nid']);
+  if ($project_issue_node->type != "project_issue") {

Neither of these actually check to see if $project_issue_node was successfully returned. Might not be a bad sanity check to add.

+++ b/project_issue.module
@@ -2158,3 +2176,156 @@ function project_issue_project_promote_sandbox($project) {
+/**
+ * Theme a node reference field with a metadata about the referenced issue node.
+ */
+function theme_project_issue_formatter_issue_id_assigned($element) {

Same PHPDoc as the function above, but this one shows assigned to information after it.

Powered by Dreditor.

Here's a redo of the above patch, to address webchick's comments in #28. I haven't tested it yet... will make time to do that tomorrow...

Changes from the previous patch:
- Fixed up the comments throughout (docblocks and in-code).
- Sanitized input and/or added comments as to why it's not needed.
- Added some comments and some additional validation in project_issue_nodereference_autocomplete_validate().
- Wrapped two literal strings in t().
- Added check to see if node was loaded in theme_project_issue_formatter_issue_id() and the following function.

One thing I didn't address was "Unless you know something I don't, that looks like an improper use of t()" regarding this string not being static:

t('%name: the value is not a valid issue id.')

It looks fine to me... so I didn't make a change. There are no variables there that I can see, just a % placeholder?

Sorry, that comment wasn't in relation to that part of the string, but in relation to this:

'%name' => t($field['widget']['label'])

http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/t/7 recommends against t($variable) unless you know what you're doing. However, I "overheard" mikey_p saying that he basically copy/pasted this out of CCK, so it's likely this is an instance where t($variable) is appropriate.

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+function project_issue_widget(&$form, &$form_state, $field, $items, $delta = 0) {
+  switch ($field['widget']['type']) {
+    case 'project_issue_nodereference_auto':
...
+      break;
+  }
+  return $element;

$element is unset unless 'type' is the expected.

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+    $nid = intval($element['#default_value'][$field_key]);

Use regular (int) casting here.

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+  // Unset the default validate.
+  foreach ($element[$field_key]['#element_validate'] as $key => $value) {
+    if ($value == 'nodereference_autocomplete_validate') {
+      unset($element[$field_key]['#element_validate'][$key]);
+    }
+  }

In one line:

unset(array_search($element[$field_key]['#element_validate'], 'nodereference_autocomplete_validate'));

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+  // Set our custom validate callback.
+  array_unshift($element[$field_key]['#element_validate'], 'project_issue_nodereference_autocomplete_validate');

Any reason for why it needs to come first? (array_unshift)

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+  if (!empty($value)) {

!empty() excludes "0". Should probably be $value !== ''.

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+    if (!empty($matches)) {
...
+      else {
+        // Sanity check: we don't expect to get anything but numbers and
+        // strings here, but just in case, throw an error as it's not valid.
+        form_error($element[$field_key], t('%name: the value is not a valid issue title or node ID.', array('%name' => t($field['widget']['label']))));
+    }
+  }
+  form_set_value($element, $nid, $form_state);

Odd indentation for the sanity check, syntax error?

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+      $n = node_load($nid);

$n is a bit cryptic.

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+      else if (!empty($title) && $n && trim($title) != trim($n->title)) {
+        form_error($element[$field_key], t('%name: title mismatch. Please check your selection.', array('%name' => t($field['widget']['label']))));
+      }

I don't understand why we're attempting to validate the title. The issue title - even when manipulated - should not have an effect?

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+      // Autocomplete syntax didn't work, so try to match a node ID.
+      if (is_numeric($value)) {
+        if (!$nid = db_result(db_query("SELECT n.nid FROM {node} n WHERE n.type = 'project_issue' AND n.nid = %d", $value))) {
...
+      // That didn't work either, so try matching just a node title.
+      elseif (is_string($value)) {
+        if (!$nid = db_result(db_query("SELECT n.nid FROM {node} n WHERE n.type = 'project_issue' AND n.status = 1 AND n.title = '%s'", trim($value)))) {

Missing db_rewrite_sql().

+++ b/project_issue.module
@@ -2186,3 +2204,177 @@
+ * Themes a node reference field with status information.
+ */
+function theme_project_issue_formatter_issue_id($element) {
+  $project_issue_node = node_load($element['#item']['nid']);
+  if (!$project_issue_node || $project_issue_node->type != "project_issue") {
+    return $element['#item']['nid'];
+  }
...
+function theme_project_issue_formatter_issue_id_assigned($element) {
...
+  if (!$project_issue_node || $project_issue_node->type != "project_issue") {
+    return $element['#item']['nid'];

In the negative case, we output the 'nid' value without any bells and whistles?

I'd either expect at least a check_plain(), or ignore the value altogether, t('n/a').

mikey_p: your turn this time...

#30 - agreed, even though CCK/noderef is doing it, t() should not be called there, so I took that out.

#31 - addressing these issues in a new patch...

One note: In the validation function for the widget, the user could have conceivably entered a node title, a node ID, or else the autocomplete javascript could have supplied #NID: title. We're checking for validation of any of those options. I think the code comments reflect that...

I think I've addressed everything else - thanks for the thorough review sun!

The patch above is *not* working any more. Looking into it...

Small problem with the order of args in array_search - it all works fine now on my test site, with this patch (sorry, this came from bzr on my test box, so it has the wrong number of path segments for a git patch, hope that is OK?).

Alright, this looks good to go now.

Thanks y'all, this is looking really close and *really* useful and cool!

There are a few trivial code style bugs in some of the PHPDoc blocks (which I've fixed in the attached patch).

However, the real reason I'm setting this to 'needs work' is that I don't understand something about project_issue_nodereference_autocomplete_validate() as it sits in this patch:

    // First try to match with auto-complete syntax ('#NID: TITLE').
    if (preg_match('/^#(\d+):\s*(.*)$/', $value, $matches)) {
      // Find the node and verify that the title matches, and that it's the 
      // right node type.
      list(, $nid, $title) = $matches;
      $node = node_load($nid);
      if (!$node || $node->type != 'project_issue') {
        form_error($element[$field_key], t('%name: Not a valid issue ID.', array('%name' => $field['widget']['label'])));
      }
      else if (!empty($title) && trim($title) != trim($node->title)) {
        form_error($element[$field_key], t('%name: Issue ID and title did not match.', array('%name' => $field['widget']['label'])));
      }
    }
    // Autocomplete syntax didn't work, so try to match a node ID.
    elseif (is_numeric($value)) {
      if (!$nid = db_result(db_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.type = 'project_issue' AND n.nid = %d"), $value))) {
        form_error($element[$field_key], t('%name: the value is not a valid issue ID.', array('%name' => $field['widget']['label'])));
      }
    }
    // That didn't work either, so try matching just a node title.
    elseif (is_string($value)) {
      if (!$nid = db_result(db_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.type = 'project_issue' AND n.status = 1 AND n.title = '%s'"), trim($value)))) {
        form_error($element[$field_key], t('%name: the value is not a valid issue title.', array('%name' => $field['widget']['label'])));
      }
    }

In the "nid: title" case, it tries a full node_load() (and doesn't test for published)
In the nid-only case, we do a separate query and don't test n.status at all.
In the title-only case, we do a separate query and include n.status = 1 inside the WHERE

What's going on here? ;)

A) We should be consistent about enforcing that people only link to published issues... or should we? Should we let people that can view the unpublished nodes (e.g. those with 'administer nodes') reference them?

B) Why node_load() in one case and not the others? node_load() is expensive (unless it's already happening and we're hitting the cache). Is there some reason the node is likely to already be loaded if we're in the nid: title case for some reason? Please to explain.

In other news:

C) This could use some simpletests... As with other recent patches, I'm not going to block a commit over it, but I'd really appreciate tests if possible.

D) Just tried this out on a local site. Once you select this widget on the first page when adding a new noderef field to a content type, you land on the 2nd page to configure the field instance and you get to select which node types can be referenced. The widget's validation is only going to allow issue nodes, so it's kinda pointless to ask again and let admins shoot themselves in the foot by allowing other node types to be referenced. However, I don't know if it's possible for the widget to alter the instance settings form or not. Again, not a blocker, but if anyone can shed light, I'd be very happy.

Cheers,
-Derek

p.s. I agree with sun in #21:

I don't understand why we're attempting to validate the title. The issue title - even when manipulated - should not have an effect?

E) Can someone explain why we care in the #nid: title case? The noderef only stores the nid anyway. I can see we need to validate the title if that's all we've got, but once we've got the nid we're aiming for, why bother? I could even imagine this being a pain in the ass if someone re-titles the issue while you're filling out a form with one of these references.

RE #37/38 -

a) I believe all the queries have db_rewrite_sql() in them, which should be checking permissions at least, but probably yes we should only show published nodes in all cases.

b) I think we should probably rewrite the code so that it does a similar query in all cases, rather than node_load, because all we need to find for all cases is nid and title.

c) Yeah, tests, always good... Can we do tests with autocomplete? Probably not I'm guessing... so I guess only form submission with the acceptable formats and widget rendering would be testable. Shouldn't be too hard to write...

d) Agreed, it shouldn't allow you to select node types, and we should check into that...

e) Agreed that we probably don't need to validate that the title matches in the #nid: title case.

So we need a new patch. I'll see if I can make time today... mikey_p, let me know in IRC or here if you are doing this instead.

@jhodgdon: Thanks for working on this...

re: a) db_rewrite_sql() just allows other modules (e.g. node access ones) to alter the query and inject more JOINs on {node_access_grants} and friends. It doesn't force a check for n.status = 1, which is all I'm concerned about. My talk of permissions was to see if we wanted to conditionally force n.status = 1 depending on the current user, but I agree, that's weird. Let's just always check for n.status = 1.

re: c) I dunno about writing autocomplete tests. ;) I was more thinking of unit tests on project_issue_autocomplete_issues_search(), and perhaps functional tests on the validation itself (ignoring the autocomplete). However, the latter might be a huge hassle since you'd need to define a content type and CCK and all that crap. If it's easier to do all this within our testing framework than I fear, go for it. But, if it's a rat hole, just some unit tests on project_issue_autocomplete_issues_search() would be a good step in the right direction.

Thanks!
-Derek

RE item (d) from the last couple of comments:

This is coming from CCK's hook_field_settings() [from the nodereference field].

The way this addition to project_issue is currently set up, it's providing a special Issue-specific editing widget and output formatter for the nodereference field. So it cannot affect the *field* settings, since it's not providing a field. All it could do is have widget settings, which would be shown in a different part of the settings screen.

So I think we're unfortunately going to have to live with this as it is. Sorry.

I have the other items addressed (except I haven't written tests), and am attaching this patch here so that I can verify that it works on my test site (wget from d.o seems to be the easiest way for me to transfer a patch here) (anyway, someone else might want to look at it).

I'm not sure I have the time/energy to write tests today either, so I'm un-assigning this for now... will report back on whether this patch works (via quick human-driven UI test).

The logic in that last patch didn't quite work in the validation. This one works. Sorry, bzr format again (patch created from d.o test box).

Perfect. I love it:

http://drupal.org/commitlog/commit/1894/a4a7e63f31f4c9211042b7ee2138225f...

Thanks everyone! This totally rocks.

Back to "needs work" on the vain hope that someone will provide a patch to remove the "needs tests" tag. ;)